Strip versioned Xcode path from build flags #414
Conversation
6629f8a to
52e64a1
Compare
cpython-unix/build.py
Outdated
| ) | ||
|
|
||
| sdk_path = res.stdout.strip() | ||
| sdk_path = os.environ["APPLE_SDK_PATH"] |
There was a problem hiding this comment.
This may break local builds outside of CI which don't have APPLE_SDK_PATH set.
There was a problem hiding this comment.
Ah ok -- I was partly trying to understand whether this is always set in CI or if I was missing something. Makes sense.
cpython-unix/build.py
Outdated
| sdk_path = os.environ["APPLE_SDK_PATH"] | ||
| if not os.path.exists(sdk_path): | ||
| raise Exception("macOS SDK path %s does not exist" % sdk_path) | ||
| env["APPLE_SDK_PATH"] = sdk_path |
There was a problem hiding this comment.
This check is already performed around line 170. Do we need to check this again?
There was a problem hiding this comment.
Yeah will DRY this up once it's working as expected.
52e64a1 to
f53f599
Compare
|
Alternatively, consider removing the sysroot argument from compiler flags. This is probably inferred by the compiler. If it isn't in a default CPython build's cflags, just rip it out. |
|
Yeah, let me check. I'm trying to understand why it doesn't appear for me when building It's not present in >>> sysconfig.get_config_var("CFLAGS")
'-fno-strict-overflow -Wsign-compare -Wunreachable-code -DNDEBUG -g -O3 -Wall -arch arm64 -mmacosx-version-min=11.0 -Wno-nullability-completeness -Wno-expansion-to-defined -Wno-undef-prefix -fPIC -Werror=unguarded-availability-new'But it does appear in |
|
Interesting, it looks like CPython itself strips these out in some cases? |
|
Ahhh ok. It seems that CPython does not strip That's interesting. Should we just drop it then from |
|
It seems more correct to me to just drop |
|
the sysroot arguments are a bit nonconventional. I use those to target a specific Xcode toolchain instead of relying on environment variables. I prefer the arguments over environment variables because they are visible in compiler flags. Env variables are often not logged nor easily preserved by build systems, like Python's sysconfig. |
|
Does it seem right to you to strip it from |
|
(That way, we retain the use of arguments over environment variables in |
4825225 to
2988635
Compare
2988635 to
eaa7556
Compare
|
Okay this work on Python 3.13, but earlier versions have their |
39c1406 to
8d40405
Compare
sysconfig
sysconfig|
|
||
|
|
||
| # Format sysconfig to ensure that string replacements take effect. | ||
| format_sysconfigdata() |
There was a problem hiding this comment.
If we don't reformat the sysconfig data file, then -isysroot and the path that follows it appear on separate lines, as separate strings within an implicit concatenation -- and replacing them becomes much harder. It looks like this file is already formatted on 3.13, but not on prior versions, so this seems fine IMO.
| globals_dict = {} | ||
| locals_dict = {} | ||
| exec(data, globals_dict, locals_dict) | ||
| build_time_vars = locals_dict['build_time_vars'] |
There was a problem hiding this comment.
Should we assert there aren't other variables so this doesn't break something in the future?
|
(Also re-tested these manually against astral-sh/uv#9744.) |
|
|
||
| with open(SYSCONFIGDATA, "wb") as fh: | ||
| fh.write(b'# system configuration generated and used by the sysconfig module\n') | ||
| fh.write(('build_time_vars = %s' % json.dumps(build_time_vars, indent=4)).encode("utf-8")) |
There was a problem hiding this comment.
Does this need a sort_keys=True to make the output deterministic? If there are any dicts, it does.
## Summary This is equivalent to astral-sh/python-build-standalone#414, but at install-time, so that it affects older Python builds too.
Summary
We get reports of users failing to build extension modules on macOS due to the presence of a hardcoded Xcode path. Here's one example:
I wasn't able to reproduce this locally... until I set
CFLAGSin my environment (to anything). It turns out that CPython already strips-isysrooton macOS, but not ifCFLAGSis set. See: https://github.com/python/cpython/blob/a03efb533a58fd13fb0cc7f4a5c02c8406a407bd/Lib/_osx_support.py#L331.(I find this a bit surprising -- it should probably still strip the values that are provided via
_sysconfigdata_t_darwin_darwin.py, and only not strip values that are set explicitly via aCFLAGSenvironment variable? But alas.)This PR modifies our macOS builds to always strip
-isysroot. We're effectively already doing this for users that don't have anyCFLAGSset globally, and the current value is almost always going to be wrong.Test Plan
I confirmed that
CFLAGS="-Wunreachable-code" uv pip install cffi==1.17.1 --python 3.13t --no-cache --verbosesucceeds with a build from this branch, but fails onmain.