Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows] misc fixes to enable Windows build #9198

Merged
merged 1 commit into from
Mar 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -506,13 +506,18 @@ if(EXECUTORCH_BUILD_FLATC)
-DFLATBUFFERS_BUILD_FLATLIB=${FLATBUFFERS_BUILD_FLATLIB}
-DFLATBUFFERS_BUILD_TESTS=${FLATBUFFERS_BUILD_TESTS}
-DFLATBUFFERS_INSTALL=${FLATBUFFERS_INSTALL}
-DCMAKE_BUILD_TYPE=Release
-DCMAKE_CXX_FLAGS="-DFLATBUFFERS_MAX_ALIGNMENT=${FLATBUFFERS_MAX_ALIGNMENT}"
INSTALL_COMMAND ""
BUILD_BYPRODUCTS <BINARY_DIR>/flatc
)
ExternalProject_Get_Property(flatbuffers BINARY_DIR)
set(FLATC_EXECUTABLE ${BINARY_DIR}/flatc)
if(WIN32)
# flatbuffers does not use CMAKE_BUILD_TYPE. Internally, the build forces Release
# config, but from CMake's perspective the build type is always Debug.
set(FLATC_EXECUTABLE ${BINARY_DIR}/$<CONFIG>/flatc.exe)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jathu this is the change that was needed

else()
set(FLATC_EXECUTABLE ${BINARY_DIR}/flatc)
endif()
set(FLATC_EXECUTABLE_BUILT_FROM_SOURCE YES)
endif()

Expand Down
14 changes: 11 additions & 3 deletions backends/xnnpack/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -69,35 +69,43 @@ foreach(fbs_file ${_xnnpack_schema__srcs})
)
endforeach()

if(WIN32)
set(MV_COMMAND powershell -Command "Move-Item -Path ${_xnnpack_flatbuffer__outputs} -Destination ${_xnnpack_schema__outputs}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cross-compiling for Windows, WIN32 is set in cmake, but there is no powershell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I may address this in another PR though. There are some other places in our CMake files that don't account for cross compiling for windows. So currently I prefer to address it in a follow up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mind adding this as a comment in this cmake? worthwhile to keep this info for the follow up next time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, for sure.

else()
set(MV_COMMAND mv ${_xnnpack_flatbuffer__outputs} ${_xnnpack_schema__outputs})
endif()

# 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}
COMMAND ${MV_COMMAND}
WORKING_DIRECTORY ${EXECUTORCH_ROOT}
DEPENDS flatc
COMMENT "Generating xnnpack_schema headers"
VERBATIM
)

unset(MV_COMMAND)

add_library(xnnpack_schema INTERFACE ${_xnnpack_schema__outputs})
set_target_properties(xnnpack_schema PROPERTIES LINKER_LANGUAGE CXX)
target_include_directories(
xnnpack_schema INTERFACE ${_xnnpack_schema__include_dir}
${EXECUTORCH_ROOT}/third-party/flatbuffers/include
)

set(xnnpack_third_party pthreadpool cpuinfo)
set(xnnpack_third_party pthreadpool extension_threadpool cpuinfo)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required due to this recent refactor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without it, I see a linking error complaining that pthreadpool symbols cannot be found.


include(cmake/Dependencies.cmake)

list(TRANSFORM _xnnpack_backend__srcs PREPEND "${EXECUTORCH_ROOT}/")
add_library(xnnpack_backend STATIC ${_xnnpack_backend__srcs})
target_link_libraries(
xnnpack_backend PRIVATE ${xnnpack_third_party} executorch_core xnnpack_schema
xnnpack_backend PUBLIC ${xnnpack_third_party} executorch_core xnnpack_schema
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing to PUBLIC as suggested by @swolchok

)

target_include_directories(
Expand Down
2 changes: 1 addition & 1 deletion extension/llm/custom_ops/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "^(aarch64|arm64|armv7)$")
list(APPEND _custom_ops__srcs
"extension/llm/custom_ops/spinquant/third-party/FFHT/fht_neon.c"
)
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64")
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "^(x86_64|AMD64)")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently AMD64 is synonymous with x86_64. On my Windows machine this value is AMD64.

list(APPEND _custom_ops__srcs
"extension/llm/custom_ops/spinquant/third-party/FFHT/fht_avx.c"
)
Expand Down
10 changes: 2 additions & 8 deletions install_executorch.bat
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes are lifed from #6979


"%PYTHON_EXECUTABLE%" install_executorch.py %*

Expand Down
27 changes: 20 additions & 7 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,22 +345,30 @@ def inplace_dir(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: str, modpath: str, src_dir: Optional[str] = None):
"""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`,
this class will also look for similarly-named `.dylib` files.
src_dir: The directory of the file to install, relative to the cmake-out
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes are lifed from #6979

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`,
modpath: The dotted path of the python module that maps to the
extension.
"""
assert (
"/" not in modpath
), f"modpath must be a dotted python module path: saw '{modpath}'"
full_src = src
if src_dir is None and platform.system() == "Windows":
src_dir = "%BUILD_TYPE%/"
if src_dir is not None:
full_src = os.path.join(src_dir, src)
# This is a real extension, so use the modpath as the name.
super().__init__(src=f"%CMAKE_CACHE_DIR%/{src}", dst=modpath, name=modpath)
super().__init__(src=f"%CMAKE_CACHE_DIR%/{full_src}", dst=modpath, name=modpath)

def src_path(self, installer: "InstallerBuildExt") -> Path:
"""Returns the path to the source file, resolving globs.
Expand Down Expand Up @@ -780,7 +788,12 @@ 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"
(
"_portable_lib.cp*"
if platform.system() == "Windows"
else "_portable_lib.*"
),
"executorch.extension.pybindings._portable_lib",
)
)
if ShouldBuild.training():
Expand Down
Loading