fix: Fix up search path of bootstrapped Python toolchain dylib #2089
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aignas
reviewed
Jul 25, 2024
aignas
requested changes
Jul 26, 2024
nicholasjng
force-pushed
the
mac-dyldpath-fixup
branch
2 times, most recently
from
476bd7d to
078f566
Compare
aignas
reviewed
Jul 26, 2024
There was a problem hiding this comment.
LGTM, could you please add a line in CHANGELOG to say that this behaviour was fixed?
@rickeylev, could you PTAL and see if something is missing from your point of view? IMHO this is good to merge as is, because there was a test and things started working better than they used to before the change.
Previously, `//tests/cc/current_py_cc_libs::python_libs_linking_test` failed on macOS because the bootstrapped toolchain's dylib had an incorrect LC_ID_DYLIB field set, pointing to a local directory on the Python standalone build host machine. To fix, add a small conditional to the Python repository rule patching the LC_ID_DYLIB field of the bootstrapped Python's dylib with its fully qualified file system path. Patching is carried out with macOS's own `install_name_tool`, which is part of the standard macOS dynamic linking toolchain. Since this needs macOS to be the host platform, restrict this change to macOS host systems only by checking the host OS name. Qualifies the mentioned test as executable on Mac, now only Windows linker errors are left to fix.
nicholasjng
force-pushed
the
mac-dyldpath-fixup
branch
from
078f566 to
48d30c5
Compare
|
I added a note to the changelog in the latest revision. Thanks for the speedy review! |
|
Any more issues on your end @aignas @rickeylev ? |
|
Doesn't setting this to an absolute path mean you get a non-hermetic build if you link this dylib? |
|
This change breaks multi-platform builds. The contents of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously,
//tests/cc/current_py_cc_libs::python_libs_linking_testfailed on macOS because the bootstrapped toolchain's dylib had an incorrect LC_ID_DYLIB field set, pointing to a local directory on the Python standalone build host machine.To fix, add a small conditional to the Python repository rule patching the LC_ID_DYLIB field of the bootstrapped Python's dylib with its fully qualified file system path. Patching is carried out with macOS's own
install_name_tool, which is part of the standard macOS dynamic linking toolchain.Qualifies the mentioned test as executable on Mac, now only Windows linker errors are left to fix.