From 038fa5307d85982b980e6d9f22d04bc34246adb2 Mon Sep 17 00:00:00 2001 From: Chao Zhang Date: Fri, 6 Dec 2024 13:35:48 +0800 Subject: [PATCH 1/3] fix windows build issue --- backends/xnnpack/CMakeLists.txt | 38 +++++++++++----- build/resolve_buck.py | 19 ++++++-- devtools/CMakeLists.txt | 53 ++++++++++++++++------ install_requirements.bat | 10 +---- runtime/platform/compat_unistd.h | 75 ++++++++++++++++++++++++++++++++ runtime/platform/compiler.h | 8 +--- runtime/platform/targets.bzl | 1 + setup.py | 21 ++++++--- 8 files changed, 175 insertions(+), 50 deletions(-) create mode 100644 runtime/platform/compat_unistd.h diff --git a/backends/xnnpack/CMakeLists.txt b/backends/xnnpack/CMakeLists.txt index ed8cf8d8e14..ab9c1214ef0 100644 --- a/backends/xnnpack/CMakeLists.txt +++ b/backends/xnnpack/CMakeLists.txt @@ -73,17 +73,33 @@ foreach(fbs_file ${_xnnpack_schema__srcs}) endforeach() # Generate the headers from the .fbs files. -add_custom_command( - OUTPUT ${_xnnpack_schema__outputs} - COMMAND - ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o - "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" - ${_xnnpack_schema__srcs} - COMMAND mv ${_xnnpack_flatbuffer__outputs} ${_xnnpack_schema__outputs} - WORKING_DIRECTORY ${EXECUTORCH_ROOT} - COMMENT "Generating xnnpack_schema headers" - VERBATIM -) +if(WIN32) + add_custom_command( + OUTPUT ${_xnnpack_schema__outputs} + COMMAND + ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o + "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" + ${_xnnpack_schema__srcs} + COMMAND + powershell -Command + "Move-Item -Path ${_xnnpack_flatbuffer__outputs} -Destination ${_xnnpack_schema__outputs}" + WORKING_DIRECTORY ${EXECUTORCH_ROOT} + COMMENT "Generating xnnpack_schema headers" + VERBATIM + ) +else() + add_custom_command( + OUTPUT ${_xnnpack_schema__outputs} + COMMAND + ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o + "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" + ${_xnnpack_schema__srcs} + COMMAND mv ${_xnnpack_flatbuffer__outputs} ${_xnnpack_schema__outputs} + WORKING_DIRECTORY ${EXECUTORCH_ROOT} + COMMENT "Generating xnnpack_schema headers" + VERBATIM + ) +endif() add_library(xnnpack_schema INTERFACE ${_xnnpack_schema__outputs}) set_target_properties(xnnpack_schema PROPERTIES LINKER_LANGUAGE CXX) diff --git a/build/resolve_buck.py b/build/resolve_buck.py index d70fe94b415..2bda69e24ba 100644 --- a/build/resolve_buck.py +++ b/build/resolve_buck.py @@ -11,7 +11,6 @@ import platform import stat import sys -import tempfile import urllib.request from dataclasses import dataclass @@ -85,6 +84,12 @@ class BuckInfo: archive_name="buck2-x86_64-apple-darwin.zst", target_versions=["3eb1ae97ea963086866b4d2d9ffa966d"], ), + ("windows", "x86_64"): BuckInfo( + archive_name="buck2-x86_64-pc-windows-msvc.exe.zst", + target_versions=[ + "bf1685c4c4ddd9de4592b5a955cb7326fd01e6c4d5f561643422bed961a17401" + ], + ), } @@ -135,6 +140,8 @@ def resolve_buck2(args: argparse.Namespace) -> Union[str, int]: os_family = "linux" elif sys.platform.startswith("darwin"): os_family = "darwin" + elif sys.platform.startswith("win"): + os_family = "windows" platform_key = (os_family, arch) if platform_key not in BUCK_PLATFORM_MAP: @@ -193,12 +200,14 @@ def resolve_buck2(args: argparse.Namespace) -> Union[str, int]: buck2_archive_url = f"https://github.com/facebook/buck2/releases/download/{target_buck_version}/{buck_info.archive_name}" - with tempfile.NamedTemporaryFile() as archive_file: + try: print(f"Downloading buck2 from {buck2_archive_url}...", file=sys.stderr) - urllib.request.urlretrieve(buck2_archive_url, archive_file.name) + # This change is based on https://github.com/posit-dev/py-shiny/issues/287 + # It works around PermissionError on Windows. This happens because urlretrieve is trying to open an already-open temporary file by name, which isn't permitted on Windows. + archive_file, _ = urllib.request.urlretrieve(buck2_archive_url) # Extract and chmod. - with open(archive_file.name, "rb") as f: + with open(archive_file, "rb") as f: data = f.read() decompressed_bytes = zstd.decompress(data) @@ -207,6 +216,8 @@ def resolve_buck2(args: argparse.Namespace) -> Union[str, int]: file_stat = os.stat(buck2_local_path) os.chmod(buck2_local_path, file_stat.st_mode | stat.S_IEXEC) + finally: + os.remove(archive_file) return buck2_local_path diff --git a/devtools/CMakeLists.txt b/devtools/CMakeLists.txt index df4bacb802f..9249dd1e459 100644 --- a/devtools/CMakeLists.txt +++ b/devtools/CMakeLists.txt @@ -157,19 +157,46 @@ file(MAKE_DIRECTORY ${_program_schema__include_dir}/executorch/devtools/bundled_program ) -add_custom_command( - OUTPUT ${_etdump_schema__outputs} - COMMAND - # Note that the flatcc project actually writes its outputs into the source - # tree instead of under the binary directory, and there's no way to change - # that behavior. - ${_flatcc_source_dir}/bin/flatcc -cwr -o - ${_program_schema__include_dir}/executorch/devtools/etdump - ${_etdump_schema__srcs} - COMMAND rm -rf ${_etdump_schema_cleanup_paths} - DEPENDS ${_etdump_schema_gen_dep} - COMMENT "Generating etdump headers" -) +# Note that the flatcc project actually writes its outputs into the source +# tree instead of under the binary directory, and there's no way to change +# that behavior. +if(WIN32) + # The binary directory for Windows will have build type like Debug/Release at the end. + set(_flatcc_bin_path ${_flatcc_source_dir}/bin/${CMAKE_BUILD_TYPE}/flatcc) +else() + set(_flatcc_bin_path ${_flatcc_source_dir}/bin/flatcc) +endif() + +if(WIN32) + add_custom_command( + OUTPUT ${_etdump_schema__outputs} + COMMAND + # Note that the flatcc project actually writes its outputs into the source + # tree instead of under the binary directory, and there's no way to change + # that behavior. + ${_flatcc_bin_path} -cwr -o + ${_program_schema__include_dir}/executorch/devtools/etdump + ${_etdump_schema__srcs} + # COMMAND powershell -Command "Remove-Item -Path " ${_etdump_schema_cleanup_paths} " -Force -ErrorAction SilentlyContinue" + COMMAND powershell -Command "if (" "'${_etdump_schema_cleanup_paths}'" "-ne '') { Remove-Item -Path " "'${_etdump_schema_cleanup_paths}'" " -Force -ErrorAction SilentlyContinue }" + DEPENDS ${_etdump_schema_gen_dep} + COMMENT "Generating etdump headers" + ) +else() + add_custom_command( + OUTPUT ${_etdump_schema__outputs} + COMMAND + # Note that the flatcc project actually writes its outputs into the source + # tree instead of under the binary directory, and there's no way to change + # that behavior. + ${_flatcc_bin_path} -cwr -o + ${_program_schema__include_dir}/executorch/devtools/etdump + ${_etdump_schema__srcs} + COMMAND rm -rf ${_etdump_schema_cleanup_paths} + DEPENDS ${_etdump_schema_gen_dep} + COMMENT "Generating etdump headers" + ) +endif() add_library( etdump ${CMAKE_CURRENT_SOURCE_DIR}/etdump/etdump_flatcc.cpp diff --git a/install_requirements.bat b/install_requirements.bat index 4cfe4b21c4b..74ec74ab2cb 100644 --- a/install_requirements.bat +++ b/install_requirements.bat @@ -7,14 +7,8 @@ rem This batch file provides a basic functionality similar to the bash script. cd /d "%~dp0" -rem Find the names of the python tools to use (replace with your actual python installation) -if "%PYTHON_EXECUTABLE%"=="" ( - if "%CONDA_DEFAULT_ENV%"=="" OR "%CONDA_DEFAULT_ENV%"=="base" OR NOT EXIST "python" ( - set PYTHON_EXECUTABLE=python3 - ) else ( - set PYTHON_EXECUTABLE=python - ) -) +rem Under windows it's always python +set PYTHON_EXECUTABLE=python "%PYTHON_EXECUTABLE%" install_requirements.py %* diff --git a/runtime/platform/compat_unistd.h b/runtime/platform/compat_unistd.h new file mode 100644 index 00000000000..c8bc4866702 --- /dev/null +++ b/runtime/platform/compat_unistd.h @@ -0,0 +1,75 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * @file + * unistd.h related macros for POSIX/Windows compatibility. + */ +#pragma once + +#if defined(_WIN32) && !defined(_WIN64) +#error \ + "You're trying to build ExecuTorch with a too old version of Windows. We need Windows 64-bit." +#endif + +#if !defined(_WIN64) +#include +#else +#include +#define O_RDONLY _O_RDONLY +#define open _open +#define close _close +#define read _read +#define write _write +#define stat _stat64 +#define fstat _fstat64 +#define off_t _off_t +#define lseek _lseeki64 + +#include // For ssize_t. +#include +// To avoid conflicts with std::numeric_limits::max() in +// file_data_loader.cpp. +#undef max + +inline ssize_t pread(int fd, void* buf, size_t nbytes, size_t offset) { + OVERLAPPED overlapped; /* The offset for ReadFile. */ + memset(&overlapped, 0, sizeof(overlapped)); + overlapped.Offset = offset; + overlapped.OffsetHigh = offset >> 32; + + BOOL result; /* The result of ReadFile. */ + DWORD bytes_read; /* The number of bytes read. */ + HANDLE file = (HANDLE)_get_osfhandle(fd); + + result = ReadFile(file, buf, nbytes, &bytes_read, &overlapped); + DWORD error = GetLastError(); + if (!result) { + if (error == ERROR_IO_PENDING) { + result = GetOverlappedResult(file, &overlapped, &bytes_read, TRUE); + if (!result) { + error = GetLastError(); + } + } + } + if (!result) { + // Translate error into errno. + switch (error) { + case ERROR_HANDLE_EOF: + errno = 0; + break; + default: + errno = EIO; + break; + } + return -1; + } + return bytes_read; +} + +#endif // !defined(_WIN64) \ No newline at end of file diff --git a/runtime/platform/compiler.h b/runtime/platform/compiler.h index bc07470387a..f17bfee30c7 100644 --- a/runtime/platform/compiler.h +++ b/runtime/platform/compiler.h @@ -100,14 +100,8 @@ #endif // (__cplusplus) >= 202002L /// Define a C symbol with weak linkage. -#ifdef _MSC_VER -// There currently doesn't seem to be a great way to do this in Windows and -// given that weak linkage is not really critical on Windows, we'll just leave -// it as a stub. -#define ET_WEAK -#else +// Building on Windows also need this. Windows build uses clang-cl compiler, which supports __attribute__((weak)). #define ET_WEAK __attribute__((weak)) -#endif /** * Annotation marking a function as printf-like, providing compiler support diff --git a/runtime/platform/targets.bzl b/runtime/platform/targets.bzl index 42bb851e2cf..68322ffe97f 100644 --- a/runtime/platform/targets.bzl +++ b/runtime/platform/targets.bzl @@ -68,6 +68,7 @@ def define_common_targets(): "log.h", "profiler.h", "runtime.h", + "compat_unistd.h", ], srcs = [ "abort.cpp", diff --git a/setup.py b/setup.py index ff1afa89bd6..b2f11f9605b 100644 --- a/setup.py +++ b/setup.py @@ -225,7 +225,7 @@ def src_path(self, installer: "InstallerBuildExt") -> Path: self.src = self.src.replace("%BUILD_TYPE%", cfg) else: # Remove %BUILD_TYPE% from the path. - self.src = self.src.replace("/%BUILD_TYPE%", "") + self.src = self.src.replace("%BUILD_TYPE%/", "") # Construct the full source path, resolving globs. If there are no glob # pattern characters, this will just ensure that the source file exists. @@ -263,7 +263,7 @@ def __init__( output is in a subdirectory named after the build type. For single- config generators (like Makefile Generators or Ninja), this placeholder will be removed. - src_name: The name of the file to install + src_name: The name of the file to install. dst: The path to install to, relative to the root of the pip package. If dst ends in "/", it is treated as a directory. Otherwise it is treated as a filename. @@ -305,13 +305,17 @@ def dst_path(self, installer: "InstallerBuildExt") -> Path: class BuiltExtension(_BaseExtension): """An extension that installs a python extension that was built by cmake.""" - def __init__(self, src: str, modpath: str): + def __init__(self, src_dir: str, src_name: str, modpath: str): """Initializes a BuiltExtension. Args: - src: The path to the file to install (typically a shared library), - relative to the cmake-out directory. May be an fnmatch-style - glob that matches exactly one file. If the path ends in `.so`, + src_dir: The directory of the file to install, relative to the cmake-out + directory. A placeholder %BUILD_TYPE% will be replaced with the build + type for multi-config generators (like Visual Studio) where the build + output is in a subdirectory named after the build type. For single- + config generators (like Makefile Generators or Ninja), this placeholder + will be removed. + src_name: The name of the file to install. If the path ends in `.so`, this class will also look for similarly-named `.dylib` files. modpath: The dotted path of the python module that maps to the extension. @@ -319,6 +323,7 @@ def __init__(self, src: str, modpath: str): assert ( "/" not in modpath ), f"modpath must be a dotted python module path: saw '{modpath}'" + src = os.path.join(src_dir, src_name) # This is a real extension, so use the modpath as the name. super().__init__(src=src, dst=modpath, name=modpath) @@ -658,7 +663,9 @@ def get_ext_modules() -> List[Extension]: # portable kernels, and a selection of backends. This lets users # load and execute .pte files from python. BuiltExtension( - "_portable_lib.*", "executorch.extension.pybindings._portable_lib" + src_dir="%BUILD_TYPE%/", # Set the src directory based on build configuration for windows. + src_name="_portable_lib.cp*", # Rename _portable_lib.* to _portable_lib.cp* to avoid _portable_lib.lib is selected on windows. + modpath="executorch.extension.pybindings._portable_lib", ) ) if ShouldBuild.llama_custom_ops(): From c5e68a2f17a0f46b9490d0b5832a0c5c32d625b0 Mon Sep 17 00:00:00 2001 From: Chao Zhang Date: Fri, 6 Dec 2024 15:00:56 +0800 Subject: [PATCH 2/3] revert xnnpack cmakelist and fix file_data_loader --- backends/xnnpack/CMakeLists.txt | 38 +++++++--------------- extension/data_loader/file_data_loader.cpp | 5 ++- 2 files changed, 15 insertions(+), 28 deletions(-) diff --git a/backends/xnnpack/CMakeLists.txt b/backends/xnnpack/CMakeLists.txt index ab9c1214ef0..ed8cf8d8e14 100644 --- a/backends/xnnpack/CMakeLists.txt +++ b/backends/xnnpack/CMakeLists.txt @@ -73,33 +73,17 @@ foreach(fbs_file ${_xnnpack_schema__srcs}) endforeach() # Generate the headers from the .fbs files. -if(WIN32) - add_custom_command( - OUTPUT ${_xnnpack_schema__outputs} - COMMAND - ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o - "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" - ${_xnnpack_schema__srcs} - COMMAND - powershell -Command - "Move-Item -Path ${_xnnpack_flatbuffer__outputs} -Destination ${_xnnpack_schema__outputs}" - WORKING_DIRECTORY ${EXECUTORCH_ROOT} - COMMENT "Generating xnnpack_schema headers" - VERBATIM - ) -else() - add_custom_command( - OUTPUT ${_xnnpack_schema__outputs} - COMMAND - ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o - "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" - ${_xnnpack_schema__srcs} - COMMAND mv ${_xnnpack_flatbuffer__outputs} ${_xnnpack_schema__outputs} - WORKING_DIRECTORY ${EXECUTORCH_ROOT} - COMMENT "Generating xnnpack_schema headers" - VERBATIM - ) -endif() +add_custom_command( + OUTPUT ${_xnnpack_schema__outputs} + COMMAND + ${FLATC_EXECUTABLE} --cpp --cpp-std c++11 --scoped-enums -o + "${_xnnpack_schema__include_dir}/executorch/backends/xnnpack/serialization" + ${_xnnpack_schema__srcs} + COMMAND mv ${_xnnpack_flatbuffer__outputs} ${_xnnpack_schema__outputs} + WORKING_DIRECTORY ${EXECUTORCH_ROOT} + COMMENT "Generating xnnpack_schema headers" + VERBATIM +) add_library(xnnpack_schema INTERFACE ${_xnnpack_schema__outputs}) set_target_properties(xnnpack_schema PROPERTIES LINKER_LANGUAGE CXX) diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp index 1d097cfd989..53696cd2e4b 100644 --- a/extension/data_loader/file_data_loader.cpp +++ b/extension/data_loader/file_data_loader.cpp @@ -17,7 +17,7 @@ #include #include #include -#include +#include #include #include @@ -71,6 +71,9 @@ FileDataLoader::~FileDataLoader() { std::free(const_cast(file_name_)); // fd_ can be -1 if this instance was moved from, but closing a negative fd is // safe (though it will return an error). + if (fd_ == -1) { + return; + } ::close(fd_); } From a571e16f2d0b6acc6c0144c850b90b422355b474 Mon Sep 17 00:00:00 2001 From: "Chao Zhang (from Dev Box)" Date: Thu, 12 Dec 2024 10:50:36 +0800 Subject: [PATCH 3/3] fix lint warning --- extension/data_loader/file_data_loader.cpp | 3 ++- runtime/platform/compiler.h | 3 ++- setup.py | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp index 53696cd2e4b..f0b01509ecf 100644 --- a/extension/data_loader/file_data_loader.cpp +++ b/extension/data_loader/file_data_loader.cpp @@ -14,10 +14,11 @@ #include #include +#include #include #include #include -#include + #include #include diff --git a/runtime/platform/compiler.h b/runtime/platform/compiler.h index f17bfee30c7..59a614f71b0 100644 --- a/runtime/platform/compiler.h +++ b/runtime/platform/compiler.h @@ -100,7 +100,8 @@ #endif // (__cplusplus) >= 202002L /// Define a C symbol with weak linkage. -// Building on Windows also need this. Windows build uses clang-cl compiler, which supports __attribute__((weak)). +// Building on Windows also need this. Windows build uses clang-cl compiler, +// which supports __attribute__((weak)). #define ET_WEAK __attribute__((weak)) /** diff --git a/setup.py b/setup.py index b2f11f9605b..128391dbdd0 100644 --- a/setup.py +++ b/setup.py @@ -663,8 +663,8 @@ def get_ext_modules() -> List[Extension]: # portable kernels, and a selection of backends. This lets users # load and execute .pte files from python. BuiltExtension( - src_dir="%BUILD_TYPE%/", # Set the src directory based on build configuration for windows. - src_name="_portable_lib.cp*", # Rename _portable_lib.* to _portable_lib.cp* to avoid _portable_lib.lib is selected on windows. + src_dir="%BUILD_TYPE%/",# Set the src directory based on build configuration for windows. + src_name="_portable_lib.cp*",# Rename _portable_lib.* to _portable_lib.cp* to avoid _portable_lib.lib is selected on windows. modpath="executorch.extension.pybindings._portable_lib", ) )