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

Remove default LTO options, dead_strip in MacOS linker options #16

Merged
merged 2 commits into from
Mar 24, 2024

Conversation

nicholasjng
Copy link
Owner

Follows the official documentation's guidance that LTO is not strictly necessary due to the library component. As an alternative, users can give the thin_lto feature to a nanobind_extension instead.

The second -Wl,-dead_strip removal is due to it appearing twice in the generated linker command. I also saw a missing symbol error during a stubgen attempt earlier, which has since disappeared.

Follows the official documentation's guidance that LTO is not strictly
necessary due to the library component. As an alternative, users can
give the `thin_lto` feature to a nanobind_extension instead.

The second dead_strip removal is due to it appearing twice in the
generated linker command. I also saw a missing symbol error during a
stubgen attempt earlier, which has since disappeared.
@nicholasjng
Copy link
Owner Author

Hm, I wonder what happened here.

The only thing I can think of is that I have removed the building Python's library directories from the linkopts for Windows in the meantime in the nanobind_example. So this still works for all Pythons except 3.12?

@nicholasjng
Copy link
Owner Author

Summary:

  1. @rules_python Windows toolchains do not yet add the unversioned libs/python3.lib to the library target, preventing SABI builds on Windows. This is hopefully fixed in the next release with the PR mentioned above.
  2. Apparently, Python itself (in a header called pyconfig.h) designates the correct library to link on Windows without me having to do anything, so the select()s with the current libs can all go. (cool!)

@nicholasjng
Copy link
Owner Author

I'm going to merge this, and require a newer rules_python for future versions.

@nicholasjng nicholasjng merged commit ed598ea into master Mar 24, 2024
18 of 20 checks passed
@nicholasjng nicholasjng deleted the option-cleanup branch March 24, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant