-
Notifications
You must be signed in to change notification settings - Fork 607
Fix Windows build #10946
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
Fix Windows build #10946
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10946
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New FailuresAs of commit 25a7a8b with merge base 3032398 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
sorry about the fail in |
kernels/quantized/CMakeLists.txt
Outdated
@@ -113,7 +113,7 @@ if(NOT CMAKE_GENERATOR STREQUAL "Xcode" | |||
quantized_pybind_kernels_lib DEPS portable_lib | |||
) | |||
target_link_libraries( | |||
quantized_ops_aot_lib PUBLIC quantized_ops_pybind_lib | |||
quantized_ops_aot_lib PUBLIC quantized_ops_pybind_lib quantized_ops_lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to prevent linker errors when linking the aot lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but lets make sure CI is happy
kernels/quantized/CMakeLists.txt
Outdated
if (_WIN32) | ||
target_link_libraries(quantized_ops_aot_lib PUBLIC quantized_ops_lib) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. Why is the dependency different for Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually know... if I don't do this, then on Windows there are linker errors where the AOT lib cannot find the symbols for the actual operator implementations in quantized_ops_lib
.
However, if I do this across the board, then I run into duplicate registration issues on non-Windows platforms. Could be platform dependent linker behaviour, I suppose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also tested running a model on windows to make sure that the duplicate registration did not occur on Windows.
Summary: ## Context Fix third party `CMakeLists.txt` to allow `flatcc` to build for Windows. Some CMake configuration settings need to be adjusted for windows platforms. Test Plan: ## Test Plan ``` python install_executorch.py ```
Failures don't seem related. In both cases the build succeeds (which this PR would impact) but failures occur during specific tests. Build Linux Wheels got cancelled because "Canceling since a higher priority waiting request for Build Linux Wheels-10946-wheel-linux-pytorch/test-infra-main exists"
|
I just merged a PR that I expect/hope fixes. Feel free to ignore the Arm backend fails. If you rebase I expect it to go away. This: |
Summary:
Context
Fix third party
CMakeLists.txt
to allowflatcc
to build for Windows. Some CMake configuration settings need to be adjusted for windows platforms.Test Plan:
Test Plan