Strip versioned Xcode path from build flags by charliermarsh · Pull Request #414 · astral-sh/python-build-standalone

charliermarsh · 2024-12-08T17:50:47Z

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 CFLAGS in my environment (to anything). It turns out that CPython already strips -isysroot on macOS, but not if CFLAGS is 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 a CFLAGS environment 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 any CFLAGS set 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 --verbose succeeds with a build from this branch, but fails on main.

indygreg · 2024-12-08T18:17:22Z

cpython-unix/build.py

-            )
-
-            sdk_path = res.stdout.strip()
+        sdk_path = os.environ["APPLE_SDK_PATH"]


This may break local builds outside of CI which don't have APPLE_SDK_PATH set.

Ah ok -- I was partly trying to understand whether this is always set in CI or if I was missing something. Makes sense.

indygreg · 2024-12-08T18:19:18Z

cpython-unix/build.py

+        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


This check is already performed around line 170. Do we need to check this again?

Yeah will DRY this up once it's working as expected.

indygreg · 2024-12-08T18:52:52Z

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.

charliermarsh · 2024-12-08T18:59:17Z

Yeah, let me check. I'm trying to understand why it doesn't appear for me when building cffi (unlike in the repro here).

It's not present in sysconfig.get_config_var:

>>> 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 _sysconfigdata_t_darwin_darwin.py:

'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  -isysroot /Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk -fPIC    -Werror=unguarded-availability-new',

charliermarsh · 2024-12-08T19:12:52Z

Interesting, it looks like CPython itself strips these out in some cases?

https://github.com/python/cpython/blob/a03efb533a58fd13fb0cc7f4a5c02c8406a407bd/Lib/_osx_support.py#L438

charliermarsh · 2024-12-08T19:16:30Z

Ahhh ok. It seems that CPython does not strip --isysroot (even from the sysconfig-provided CFLAGS) if you have a non-empty CFLAGS set in your environment, so I can reproduce the failure with:

CFLAGS="-Wunreachable-code" uv pip install cffi==1.17.1 --python 3.13t --no-cache --verbose

That's interesting. Should we just drop it then from CFLAGS et al?

charliermarsh · 2024-12-08T19:17:06Z

It seems more correct to me to just drop isysroot everywhere, in that case.

indygreg · 2024-12-08T19:22:23Z

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.

charliermarsh · 2024-12-08T19:44:07Z

Does it seem right to you to strip it from _sysconfigdata_t_darwin_darwin.py, though? (That's my latest attempt, above.)

charliermarsh · 2024-12-08T19:46:50Z

(That way, we retain the use of arguments over environment variables in python-build-standalone itself, but avoid shipping those unwanted paths to users. Unless I'm misunderstanding the dataflow.)

charliermarsh · 2024-12-08T20:44:32Z

Okay this work on Python 3.13, but earlier versions have their _sysconfigdata_t_darwin_darwin.py formatted such that -isysroot and the path appear in separate strings within an implicit concatenation, which is super annoying... I'm tempted to re-format them prior to find-replace.

charliermarsh · 2024-12-08T23:31:39Z

cpython-unix/build-cpython.sh

+
+
+# Format sysconfig to ensure that string replacements take effect.
+format_sysconfigdata()


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.

cpython-unix/build-cpython.sh

zanieb · 2024-12-09T15:50:08Z

cpython-unix/build-cpython.sh

+    globals_dict = {}
+    locals_dict = {}
+    exec(data, globals_dict, locals_dict)
+    build_time_vars = locals_dict['build_time_vars']


Should we assert there aren't other variables so this doesn't break something in the future?

cpython-unix/build.py

charliermarsh · 2024-12-10T13:27:28Z

(Also re-tested these manually against astral-sh/uv#9744.)

indygreg

LGTM

indygreg · 2024-12-10T15:21:54Z

cpython-unix/build-cpython.sh

+
+    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"))


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.

charliermarsh force-pushed the charlie/xcode branch from 6629f8a to 52e64a1 Compare December 8, 2024 18:11

indygreg reviewed Dec 8, 2024

View reviewed changes

Use a non-versioned Xcode path in sysconfig

f53f599

charliermarsh force-pushed the charlie/xcode branch from 52e64a1 to f53f599 Compare December 8, 2024 18:26

charliermarsh force-pushed the charlie/xcode branch from 4825225 to 2988635 Compare December 8, 2024 19:49

Drop from _sysconfig

eaa7556

charliermarsh force-pushed the charlie/xcode branch from 2988635 to eaa7556 Compare December 8, 2024 20:20

Reformat sysconfig

8d40405

charliermarsh force-pushed the charlie/xcode branch from 39c1406 to 8d40405 Compare December 8, 2024 23:18

Reverts

6ed2b07

charliermarsh marked this pull request as ready for review December 8, 2024 23:30

charliermarsh requested a review from zanieb December 8, 2024 23:30

charliermarsh changed the title ~~Use a non-versioned Xcode path in sysconfig~~ Strip versioned Xcode path from sysconfig Dec 8, 2024

charliermarsh changed the title ~~Strip versioned Xcode path from sysconfig~~ Strip versioned Xcode path from build flags Dec 8, 2024

charliermarsh commented Dec 8, 2024

View reviewed changes

charliermarsh requested a review from indygreg December 9, 2024 15:40

zanieb reviewed Dec 9, 2024

View reviewed changes

cpython-unix/build-cpython.sh Show resolved Hide resolved

zanieb reviewed Dec 9, 2024

View reviewed changes

cpython-unix/build.py Show resolved Hide resolved

zanieb approved these changes Dec 9, 2024

View reviewed changes

zanieb mentioned this pull request Dec 9, 2024

uv add chromadb fails, error Compiling with an SDK that doesn't seem to exist /Applications/Xcode astral-sh/uv#9744

Closed

charliermarsh merged commit 0649b17 into main Dec 10, 2024

charliermarsh deleted the charlie/xcode branch December 10, 2024 13:27

indygreg reviewed Dec 10, 2024

View reviewed changes

charliermarsh mentioned this pull request Dec 10, 2024

Sort keys in sysconfig data #415

Merged

zanieb mentioned this pull request Dec 13, 2024

Remove -isysroot when patching sysconfig astral-sh/uv#9860

Merged

charliermarsh added a commit to astral-sh/uv that referenced this pull request Dec 13, 2024

Remove -isysroot when patching sysconfig (#9860)

08ea79c

## Summary This is equivalent to astral-sh/python-build-standalone#414, but at install-time, so that it affects older Python builds too.



		# Format sysconfig to ensure that string replacements take effect.
		format_sysconfigdata()

Uh oh!

Comments

Conversation

charliermarsh commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

indygreg Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

charliermarsh Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

indygreg Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

charliermarsh Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

indygreg commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

indygreg commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

charliermarsh commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charliermarsh commented Dec 8, 2024

Uh oh!

charliermarsh Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zanieb Dec 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charliermarsh commented Dec 10, 2024

Uh oh!

indygreg left a comment

Choose a reason for hiding this comment

Uh oh!

indygreg Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

charliermarsh commented Dec 8, 2024 •

edited

Loading

charliermarsh commented Dec 8, 2024 •

edited

Loading