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

fix: Correctly resolve macOS SDK paths #2478

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shs96c
Copy link
Contributor

@shs96c shs96c commented Dec 6, 2024

XCode has facilities for accurately telling us where SDKs are installed. This is important to use, particularly when there may be multiple SDKs or versions of XCode installed.

Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM, but please include a CHANGELOG note about the changed behaviour on the MacOS platforms.

Comment on lines 58 to 83
xcode_sdks_json = rctx.execute([
"xcrun",
"xcodebuild",
"-showsdks",
"-json",
], environment = {
"DEVELOPER_DIR": xcode_root,
}).stdout
xcode_sdks = json.decode(xcode_sdks_json)
potential_sdks = [
sdk
for sdk in xcode_sdks
if "productName" in sdk and
sdk["productName"] == "macOS" and
"darwinos" not in sdk["canonicalName"]
]

# Now we'll get two entries here (one for internal and another one for public)
# It shouldn't matter which one we pick.
xcode_sdk_path = potential_sdks[0]["sdkPath"]
else:
xcode_sdk_path = "{}/SDKs/MacOSX.sdk".format(xcode_root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking out loud, is this path absolute? I am wondering if we should have the path in the rules_python_internal where we then can include the path by importing it as a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it's absolute when I run the command on my machine.

#
# We may want to build in an environment without a cc toolchain.
# In those cases, we're limited to --donwload-only, but we should respect that here.
if not rctx.attr.download_only:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also exclude when the filename is specified and it is a whl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -55,9 +55,31 @@ def _get_xcode_location_cflags(rctx):
# This is a full xcode installation somewhere like /Applications/Xcode13.0.app/Contents/Developer
# so we need to change the path to to the macos specific tools which are in a different relative
# path than xcode installed command line tools.
xcode_root = "{}/Platforms/MacOSX.platform/Developer".format(xcode_root)
xcode_sdks_json = rctx.execute([
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use repo_utils.execute_checked from `//python/private:repo_utils.bzl. We have wrappers that do some book keeping to aid debugging etc. (See a few lines above for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no idea that existed. How useful. Done!

python/private/pypi/whl_library.bzl Outdated Show resolved Hide resolved
XCode has facilities for accurately telling us where SDKs are installed.
This is important to use, particularly when there may be multiple SDKs
or versions of XCode installed.
@shs96c shs96c force-pushed the resolve-macos-sdks branch from 450fb76 to f4c5b61 Compare December 18, 2024 17:00
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.

3 participants