[Windows] Fix build issues using Clang-CL on Windows, add CI #121
Conversation
meta-cla
bot
added
the
CLA Signed
GregoryComer
force-pushed
the
windows-support
branch
from
f21a6bd to
d115ac6
Compare
GregoryComer
force-pushed
the
windows-support
branch
14 times, most recently
from
ded06d3 to
c060226
Compare
| @@ -49,7 +51,7 @@ endif() | |||
|
|
|||
| add_subdirectory( | |||
| ${CMAKE_CURRENT_SOURCE_DIR}/third-party/sentencepiece | |||
| ${CMAKE_CURRENT_BINARY_DIR}/sentencepiece-build | |||
There was a problem hiding this comment.
This is to resolve path length limitations in CI. Despite having long paths enabled on the system, there are limitations in the VS build tools. The extension build has a deeply nested structure which slightly exceeds the allowable length by a few characters.
| @@ -124,7 +124,11 @@ TEST_F(TiktokenTest, ConstructionWithInvalidBOSIndex) { | |||
| std::vector<std::string>{"<|end_of_text|>"}), | |||
| 1, | |||
| 0), | |||
| #if !GTEST_OS_WINDOWS | |||
| ::testing::KilledBySignal(SIGABRT), | |||
There was a problem hiding this comment.
This API is not available on Windows (due to not using signals).
GregoryComer
force-pushed
the
windows-support
branch
5 times, most recently
from
f46a3e8 to
b7e47a5
Compare
|
CI is now green and the infra issue with Windows runners is resolved, opening for review. |
| #ifdef _WIN32 | ||
| // ssize_t isn't available on Windows. Alias it to the Windows SSIZE_T value. | ||
| #include <BaseTsd.h> | ||
| typedef SSIZE_T ssize_t; |
There was a problem hiding this comment.
In hindsight, this should probably be typedefed as TK_SSIZE_T or something similar as this is a public header. I'll update this before landing
There was a problem hiding this comment.
I added a new compiler.h header for platform and toolchain specific defs, following the pattern used in ET. I was originally going to introduce a new typedef or macro for TK_SSIZE_T, but looking at other projects, it seems like just directly typedefing to size_t is common, so I've done that. If anyone has strong opinions, I'm happy to change it.
| setup( | ||
| name="pytorch-tokenizers", | ||
| version="0.1.0", | ||
| long_description=long_description, | ||
| long_description_content_type="text/markdown", | ||
| url="https://github.com/meta-pytorch/tokenizers", | ||
| packages=find_packages(), | ||
| ext_modules=[CMakeExtension("pytorch_tokenizers_cpp")], | ||
| ext_modules=[CMakeExtension("pytorch_tokenizers.pytorch_tokenizers_cpp")], |
There was a problem hiding this comment.
What is this for? Is it another windows nuance?
There was a problem hiding this comment.
From what I understand, this is the way it should be, as it handles the namespacing correctly. The extension name should be the fully qualified name of the extension module. We were doing something slightly weird in that we were updating the CMake build to tell it to dump the library into a different directory (appending to extdir). But with this change, it just works on all platforms.
GregoryComer
force-pushed
the
windows-support
branch
3 times, most recently
from
0d17887 to
50bedee
Compare
GregoryComer
force-pushed
the
windows-support
branch
from
50bedee to
5abf2a9
Compare
This PR adds support for building with Clang-CL on Windows, as well as CI to cover this.
Changes:
-Wno-attributeson Windows. MSVC and Clang-CL don't accept this option.Fixes #111.
Test Plan:
I've add CI to run native and Python unit tests on Windows. I've also verified locally that editable and non-editable installs work on both Linux and Windows.