-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
bpo-42504: fix for MACOSX_DEPLOYMENT_TARGET=11 #23556
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
I have now signed the CLA |
Looks good to me 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. There is more work to be done, though. With:
MACOSX_DEPLOYMENT_TARGET=11 ./configure
making this change in setup.py
gets past the first error only to fail later in distutils/spawn.py
:
File "source/Lib/distutils/spawn.py", line 66, in spawn
if _cfg_target_split > [int(x) for x in cur_target.split('.')]:
TypeError: '>' not supported between instances of 'NoneType' and 'list'
Are there other users of MACOSX_DEPLOYMENT_TARGET
in the source that need to be changed as well?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
A nicer fix is to ensure that sysconfig.get_config_var('MACOSX_DEPLOYMENT_TARGET') evaluates to a string. I haven't checked how easy it would be to do this. |
@ronaldoussoren the code at https://github.com/python/cpython/blob/3.9/Lib/sysconfig.py#L461 shows it's very deliberate that int-type values are returned as int |
Two places I am not 100% sure what to do:
|
I have made the requested changes; please review again. Travis is telling me:
but I haven't changed whitespace in this file, and it is not telling me what the issue actually is. |
Thanks for making the requested changes! @ned-deily: please review the changes made to this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. Running test_distutils:
./python(.exe) -m test -w test_distutils
gives a number of failing test cases, basically due to one of two problems.
-
The missing
str()
at spawn.py:66 -
Failures in
test_build_ext.py:501
:
File "source/Lib/distutils/tests/test_build_ext.py", line 501, in _try_compile_deployment_target
target = '%02d%02d00' % target
TypeError: not enough arguments for format string
I don't think we need to make any changes to configure.ac
or build_installer.py
.
Lib/distutils/spawn.py
Outdated
@@ -57,7 +57,7 @@ def spawn(cmd, search_path=1, verbose=0, dry_run=0): | |||
_cfg_target = sysconfig.get_config_var( | |||
'MACOSX_DEPLOYMENT_TARGET') or '' | |||
if _cfg_target: | |||
_cfg_target_split = [int(x) for x in _cfg_target.split('.')] | |||
_cfg_target_split = [int(x) for x in str(_cfg_target).split('.')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar change is needed below at line 66.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@nilleb wait I see the problem you're referring to, we're passing that int as an environment variable. That's where it should be fixed, actually. If it's defined, |
This was tested with a full build of Python on macOS Big Sur (ARM), as well as building several python-based packages. Without the latest version of the patch, some failed in |
hey @fxcoudert the problem comes from line https://github.com/python/cpython/pull/23556/files#diff-a06590fdcc4068035ba08a86c9c816cb70a78d80cb9ff0babfbb8dae85dd41d0L75 you can reproduce it by git clone https://github.com/nilleb/magpie
cd magpie
bin/setup edit: the interesting part of the stack
|
@nilleb this is fixed in the latest version of this patch. The problem is we pass |
@fxcoudert can you add the blurb using the blurb-it tool so that check will pass? https://blurb-it.herokuapp.com/ |
@ned-deily mind taking a gander at this one again? It looks ready to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks! No doubt we will find other implications of moving from 10.x to 1x release version but this should be a good start.
Thanks @fxcoudert for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
macOS releases numbering has changed as of macOS 11 Big Sur. Previously, major releases were of the form 10.x, 10.x+1, 10.x+2, etc; as of Big Sur, they are now x, x+1, etc, so, for example, 10.15, 10.15.1, ..., 10.15.7, 11, 11.0.1, 11.1, ..., 12, 12.1, etc. Allow Python to build with single-digit deployment target values. Patch provided by FX Coudert. (cherry picked from commit 5291639) Co-authored-by: FX Coudert <fxcoudert@gmail.com>
GH-23622 is a backport of this pull request to the 3.9 branch. |
macOS releases numbering has changed as of macOS 11 Big Sur. Previously, major releases were of the form 10.x, 10.x+1, 10.x+2, etc; as of Big Sur, they are now x, x+1, etc, so, for example, 10.15, 10.15.1, ..., 10.15.7, 11, 11.0.1, 11.1, ..., 12, 12.1, etc. Allow Python to build with single-digit deployment target values. Patch provided by FX Coudert. (cherry picked from commit 5291639) Co-authored-by: FX Coudert <fxcoudert@gmail.com>
Related PR, with |
macOS releases numbering has changed as of macOS 11 Big Sur. Previously, major releases were of the form 10.x, 10.x+1, 10.x+2, etc; as of Big Sur, they are now x, x+1, etc, so, for example, 10.15, 10.15.1, ..., 10.15.7, 11, 11.0.1, 11.1, ..., 12, 12.1, etc. Allow Python to build with single-digit deployment target values. Patch provided by FX Coudert.
macOS releases numbering has changed as of macOS 11 Big Sur. Previously, major releases were of the form 10.x, 10.x+1, 10.x+2, etc; as of Big Sur, they are now x, x+1, etc, so, for example, 10.15, 10.15.1, ..., 10.15.7, 11, 11.0.1, 11.1, ..., 12, 12.1, etc. Allow Python to build with single-digit deployment target values. Patch provided by FX Coudert. (cherry picked from commit 5291639) Co-authored-by: FX Coudert <fxcoudert@gmail.com>
) * bpo-41100: Support macOS 11 and Apple Silicon on Python 3.8 This is a partial backport of bpo-41100 changes `e8b1c038b14b5fc8120aab62c9bf5fb840274cb6` and `96d906b144e6e6aa96c5ffebecbcc5d38034bbda` for Python 3.8. We introduce the ability to build Python from source for `arm64` on macOS, but we do not make a promise of support. This allows us to omit support for Universal2 binaries as well as weak-linking of symbols from the macOS SDK based on the deployment target, which are larger changes much more difficult to merge. This also includes a backport of subsequent bpo-42688 change `7e729978fa08a360cbf936dc215ba7dd25a06a08` to fix build errors with external `libffi`. * bpo-41116: Ensure system supplied libraries are found on macOS 11 (GH-23301) (GH-23455) On macOS system provided libraries are in a shared library cache and not at their usual location. This PR teaches distutils to search in the SDK, even if there was no "-sysroot" argument in the compiler flags. (cherry picked from commit 404a719) * bpo-42504: fix for MACOSX_DEPLOYMENT_TARGET=11 (GH-23556) macOS releases numbering has changed as of macOS 11 Big Sur. Previously, major releases were of the form 10.x, 10.x+1, 10.x+2, etc; as of Big Sur, they are now x, x+1, etc, so, for example, 10.15, 10.15.1, ..., 10.15.7, 11, 11.0.1, 11.1, ..., 12, 12.1, etc. Allow Python to build with single-digit deployment target values. Patch provided by FX Coudert. (cherry picked from commit 5291639) * bpo-42504: Ensure that get_config_var('MACOSX_DEPLOYMENT_TARGET') is a string (GH-24341) (GH-24410) * bpo-42504: Ensure that get_config_var('MACOSX_DEPLOYMENT_TARGET') is a string (cherry picked from commit 49926cf) Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com> Co-authored-by: FX Coudert <fxcoudert@gmail.com> Co-authored-by: Max Bélanger <aeromax@gmail.com>
This pull request provides a fix for https://bugs.python.org/issue42504
https://bugs.python.org/issue42504