Skip to content

[Build] fix build issues for native Windows. #4681

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

Closed
wants to merge 4 commits into from
Closed
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
10 changes: 10 additions & 0 deletions build/Utils.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,21 @@ function(macos_kernel_link_options target_name)
)
endfunction()

# Same as kernel_link_options but it's for MSVC linker
function(msvc_kernel_link_options target_name)
target_link_options(
${target_name} INTERFACE
"SHELL:LINKER:/WHOLEARCHIVE:$<TARGET_FILE:${target_name}>"
)
endfunction()

# Ensure that the load-time constructor functions run. By default, the linker
# would remove them since there are no other references to them.
function(target_link_options_shared_lib target_name)
if(APPLE)
macos_kernel_link_options(${target_name})
elseif(MSVC)
msvc_kernel_link_options(${target_name})
else()
kernel_link_options(${target_name})
endif()
Expand Down
4 changes: 3 additions & 1 deletion build/pip_data_bin_init.py.in
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ def _find_executable_files_under(dir):
for filename in os.listdir(dir):
filepath = os.path.join(dir, filename)
if os.path.isfile(filepath) and os.access(filepath, os.X_OK):
bin_names.append(filename)
# avoid def flat.exe() on windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comments should begin with a capital letter. And I suggest making this a little more general.

Suggested change
# avoid def flat.exe() on windows.
# Remove .exe suffix on windows.

filename_without_ext = os.path.splitext(filename)[0]
bin_names.append(filename_without_ext)
return bin_names

# The list of binaries to create wrapper functions for.
Expand Down
17 changes: 13 additions & 4 deletions build/resolve_buck.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import platform
import stat
import sys
import tempfile
import urllib.request

from dataclasses import dataclass
Expand Down Expand Up @@ -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"
],
),
}


Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -193,12 +200,12 @@ 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)
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)

Expand All @@ -207,6 +214,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

Expand Down
10 changes: 8 additions & 2 deletions extension/data_loader/file_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
#ifndef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

I really want to avoid _WIN32 checks in any of the code, but this is reasonable for now. The best solution would be to avoid using POSIX concepts and to just use the standard C APIs.

Suggested change
#ifndef _WIN32
// TODO: Rewrite FileDataLoader to use standard C FILE* instead of POSIX file
// descriptors, so that we don't need this _WIN32 check.
#ifndef _WIN32

Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up that #4760 merged recently, which changes this class to use pread(), which I think will break windows. It might make sense to write a separate WindowsFileDataLoader.

There would be a lot of duplicated code, though. If we needed to split into a windows-specific data loader, it might make sense to have an AbstractFileDataLoader with one pure-virtual protected method to do the actual read.

#include <unistd.h>
#else
#include <io.h>
#endif

#include <executorch/runtime/core/error.h>
#include <executorch/runtime/core/result.h>
Expand Down Expand Up @@ -55,8 +59,10 @@ FileDataLoader::~FileDataLoader() {
// file_name_ can be nullptr if this instance was moved from, but freeing a
// null pointer is safe.
std::free(const_cast<char*>(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).
// fd_ can be -1 if this instance was moved from.
if (fd_ == -1) {
return;
}
::close(fd_);
}

Expand Down
5 changes: 5 additions & 0 deletions extension/llm/custom_ops/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ if(EXECUTORCH_BUILD_KERNELS_CUSTOM_AOT)
endif()

target_link_libraries(custom_ops_aot_lib PUBLIC cpublas torch)
if(WIN32)
# There is no direct replacement for libpthread.so on Windows.
# For the Windows build, link directly against pthreadpool and cpuinfo.
target_link_libraries(custom_ops_aot_lib PUBLIC pthreadpool cpuinfo)
endif()
target_compile_options(
custom_ops_aot_lib
PUBLIC -Wno-deprecated-declarations -fPIC -frtti -fexceptions
Expand Down
21 changes: 21 additions & 0 deletions install_requirements.bat
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
@ECHO OFF

rem Copyright (c) Meta Platforms, Inc. and affiliates.
rem All rights reserved.

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
)
)

"%PYTHON_EXECUTABLE%" install_requirements.py %*

exit /b %ERRORLEVEL%
4 changes: 4 additions & 0 deletions install_requirements.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ def python_is_compatible():
print(f"Error: Unknown option {arg}")
sys.exit(1)

# Use ClangCL on Windows.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment repeats what the code does, and doesn't provide extra value. Either remove it, or (ideally) add a comment explaining why this logic is necessary. It'd also be helpful to explain what ClangCL is, since non-windows devs will not be familiar with it.

if os.name == "nt":
CMAKE_ARGS += " -T ClangCL"

# Since ExecuTorch often uses main-branch features of pytorch, only the nightly
# pip versions will have the required features.
#
Expand Down
2 changes: 0 additions & 2 deletions runtime/core/portable_type/tensor_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@

#pragma once

#include <sys/types.h> // TODO(T126923429): Include size_t, ssize_t

#include <executorch/runtime/core/array_ref.h>
#include <executorch/runtime/core/error.h>
#include <executorch/runtime/core/portable_type/scalar_type.h>
Expand Down
7 changes: 7 additions & 0 deletions runtime/platform/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,13 @@
#endif
#endif // ifndef

#ifndef _WIN32
#include <sys/types.h> // TODO(T126923429): Include size_t, ssize_t
#else
#include <stddef.h>
using ssize_t = ptrdiff_t;
#endif
Comment on lines +136 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

Your PR actually fixes the TODO, so we can remove it.

Suggested change
#ifndef _WIN32
#include <sys/types.h> // TODO(T126923429): Include size_t, ssize_t
#else
#include <stddef.h>
using ssize_t = ptrdiff_t;
#endif
// Define size_t and ssize_t.
#ifndef _WIN32
#include <sys/types.h>
#else
#include <stddef.h>
using ssize_t = ptrdiff_t;
#endif


// DEPRECATED: Use the non-underscore-prefixed versions instead.
// TODO(T199005537): Remove these once all users have stopped using them.
#define __ET_DEPRECATED ET_DEPRECATED
Expand Down
94 changes: 78 additions & 16 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

import contextlib
import os
import platform
import re
import sys

Expand Down Expand Up @@ -162,6 +163,31 @@ def write_to_python_file(cls, path: str) -> None:
fp.write("\n".join(lines) + "\n")


# The build type is determined by the DEBUG environment variable. If DEBUG is
# set to a non-empty value, the build type is Debug. Otherwise, the build type
# is Release.
def get_build_type(is_debug=None) -> str:
debug = int(os.environ.get("DEBUG", 0)) if is_debug is None else is_debug
cfg = "Debug" if debug else "Release"
return cfg


def get_dynamic_lib_name(name: str) -> str:
if platform.system() == "Windows":
return name + ".dll"
elif platform.system() == "Darwin":
return "lib" + name + ".dylib"
else:
return "lib" + name + ".so"


def get_executable_name(name: str) -> str:
if platform.system() == "Windows":
return name + ".exe"
else:
return name


class _BaseExtension(Extension):
"""A base class that maps an abstract source to an abstract destination."""

Expand Down Expand Up @@ -189,9 +215,17 @@ def src_path(self, installer: "InstallerBuildExt") -> Path:
installer: The InstallerBuildExt instance that is installing the
file.
"""
# TODO(dbort): share the cmake-out location with CustomBuild. Can get a
# handle with installer.get_finalized_command('build')
cmake_cache_dir: Path = Path().cwd() / installer.build_temp / "cmake-out"
# Share the cmake-out location with CustomBuild.
cmake_cache_dir = Path(installer.get_finalized_command("build").cmake_cache_dir)

cfg = get_build_type(installer.debug)

if os.name == "nt":
# Replace %BUILD_TYPE% with the current build type.
self.src = self.src.replace("%BUILD_TYPE%", cfg)
else:
# Remove %BUILD_TYPE% from the path.
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.
Expand All @@ -212,17 +246,35 @@ class BuiltFile(_BaseExtension):
`ext_modules`.
"""

def __init__(self, src: str, dst: str):
def __init__(
self,
src_path: str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call this src_dir since it's specifically a directory. src_path sounds like it could still refer to the path to the source file.

src_name: str,
dst: str,
is_executable: bool = False,
is_dynamic_lib: bool = False,
):
"""Initializes a BuiltFile.

Args:
src: The path to the file to install, relative to the cmake-out
directory. May be an fnmatch-style glob that matches exactly one
file.
src_path: The path to the file to install without name, relative to the cmake-out
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mention %BUILD_TYPE% here and how to use it.

directory.
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.
is_executable: If True, the file is an executable. This is used to
determine the destination filename for executable.
is_dynamic_lib: If True, the file is a dynamic library. This is used
to determine the destination filename for dynamic library.
"""
if is_executable and is_dynamic_lib:
raise ValueError("is_executable and is_dynamic_lib cannot be both True.")
if is_executable:
src_name = get_executable_name(src_name)
elif is_dynamic_lib:
src_name = get_dynamic_lib_name(src_name)
src = os.path.join(src_path, src_name)
# This is not a real extension, so use a unique name that doesn't look
# like a module path. Some of setuptools's autodiscovery will look for
# extension names with prefixes that match certain module paths.
Expand Down Expand Up @@ -397,7 +449,7 @@ def __init__(self):
self.saved_env = {}

def __enter__(self):
if os.geteuid() == 0 and "HOME" in os.environ:
if os.name != "nt" and os.geteuid() == 0 and "HOME" in os.environ:
log.info("temporarily unsetting HOME while running as root")
self.saved_env["HOME"] = os.environ.pop("HOME")
return self
Expand Down Expand Up @@ -432,8 +484,7 @@ def initialize_options(self):
def run(self):
self.dump_options()

debug = int(os.environ.get("DEBUG", 0)) if self.debug is None else self.debug
cfg = "Debug" if debug else "Release"
cfg = get_build_type(self.debug)

# get_python_lib() typically returns the path to site-packages, where
# all pip packages in the environment are installed.
Expand Down Expand Up @@ -508,6 +559,8 @@ def run(self):
item for item in os.environ["CMAKE_BUILD_ARGS"].split(" ") if item
]

build_args += ["--config", cfg]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why this is necessary.


# Put the cmake cache under the temp directory, like
# "pip-out/temp.<plat>/cmake-out".
cmake_cache_dir = os.path.join(repo_root, self.build_temp, "cmake-out")
Expand Down Expand Up @@ -545,18 +598,24 @@ def run(self):
"build/pip_data_bin_init.py.in",
os.path.join(bin_dir, "__init__.py"),
)
# Share the cmake-out location with _BaseExtension.
self.cmake_cache_dir = cmake_cache_dir

# Finally, run the underlying subcommands like build_py, build_ext.
build.run(self)


def get_ext_modules() -> List[Extension]:
"""Returns the set of extension modules to build."""

ext_modules = []
if ShouldBuild.flatc():
ext_modules.append(
BuiltFile("third-party/flatbuffers/flatc", "executorch/data/bin/")
BuiltFile(
"third-party/flatbuffers/%BUILD_TYPE%/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will %BUILD_TYPE% always be the final component of the directory path? If so, we could remove the %BUILD_TYPE% placeholder and just add it to the source path inside BuiltFile.

It's ok as-is for now, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but it does not have access to the installer in the constructor of BuiltFile.
If I understand correctly, obtaining the build configuration requires access to installer.debug.

"flatc",
"executorch/data/bin/",
Comment on lines +614 to +616
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that there are more than two params, please use kwargs for all params. Same for other BuiltFile uses below.

Suggested change
"third-party/flatbuffers/%BUILD_TYPE%/",
"flatc",
"executorch/data/bin/",
src_path="third-party/flatbuffers/%BUILD_TYPE%/",
src_name="flatc",
dst="executorch/data/bin/",

is_executable=True,
)
)

if ShouldBuild.pybindings():
Expand All @@ -570,17 +629,20 @@ def get_ext_modules() -> List[Extension]:
)
if ShouldBuild.llama_custom_ops():
ext_modules.append(
# Install the prebuilt library for custom ops used in llama.
BuiltFile(
"extension/llm/custom_ops/libcustom_ops_aot_lib.*",
"executorch/extension/llm/custom_ops/",
"extension/llm/custom_ops/%BUILD_TYPE%/",
"custom_ops_aot_lib",
"executorch/extension/llm/custom_ops",
is_dynamic_lib=True,
)
)
ext_modules.append(
# Install the prebuilt library for quantized ops required by custom ops.
BuiltFile(
"kernels/quantized/libquantized_ops_aot_lib.*",
"kernels/quantized/%BUILD_TYPE%/",
"quantized_ops_aot_lib",
"executorch/kernels/quantized/",
is_dynamic_lib=True,
)
)

Expand Down
Loading