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

distutils: pass -rpath on macOS when requested #37

Merged
merged 1 commit into from
May 18, 2021

Conversation

acolinisi
Copy link

Resurrecting a patch to an old issue that is still relevant: distutils does not set the runtime search path when linking the shared object on macOS, passing a -L argument instead of an -rpath argument, like on the other systems. The code in question states that OS X linker doesn't accept -rpath, but that code was written 19 years ago, and that comment is no longer true (certainly not on macOS 11.2, but I do not know what is the newest system version for which -rpath is not accepted, if any).

The runtime_library_dirs configuration option is vital for extensions that need to build against libraries that temporarily exist in a location different from their final installation location (such as during the build of a package for a distro).

Submitted to setuptools, but got instructions to submit to distutils instead for later merging to setuptools:
pypa/setuptools#2627

Fix -R option of build_ext for macOS (darwin)

Resolves this old bug against distutils that expired due to PIP 632:
https://bugs.python.org/issue36353

Applies patch originally submitted to CPython:
python/cpython#12418

@jmroot
Copy link
Contributor

jmroot commented Apr 7, 2021

I do not know what is the newest system version for which -rpath is not accepted, if any

10.5 and later support @rpath. So MACOSX_DEPLOYMENT_TARGET should be checked before using this, given that cpython can still target Mac OS X versions older than 10.3.

@acolinisi acolinisi force-pushed the PR--rpath-on-macOS branch from 694904e to d2659ce Compare April 7, 2021 15:52
@acolinisi
Copy link
Author

Pushed check of MACOSX_DEPLOYMENT_TARGET.

Yeah, I didn't know about this variable. Setting this to an older version and then attempting to pass -rpath, we get the version info we needed:

ld: -rpath can only be used when targeting Mac OS X 10.5 or later

@jmroot
Copy link
Contributor

jmroot commented Apr 7, 2021

Be aware that if #36 is merged, the value in the environment at runtime will be relevant, as it overrides sysconfig if present, and will no longer be restricted to being >= the sysconfig value.

@acolinisi
Copy link
Author

So, you want me to get the value from os.environ instead? Or, from os.environ with fallback to sysconfig?

Btw, that mismatch error was triggering for me when I was trying to test this by overriding the env var. So, where does sysconfig get the value from? It's not from environment -- "hardcoded" per system? So, testing this PR on a 11.2 by setting the env var to 10.3 won't work, correct?

@jmroot
Copy link
Contributor

jmroot commented Apr 8, 2021

If #36 is merged, the effective target would need to be discovered here the same way that spawn.py does it, i.e.

cur_target = os.environ.get('MACOSX_DEPLOYMENT_TARGET', _cfg_target)

@jmroot
Copy link
Contributor

jmroot commented Apr 8, 2021

So, where does sysconfig get the value from? It's not from environment -- "hardcoded" per system? So, testing this PR on a 11.2 by setting the env var to 10.3 won't work, correct?

The value is recorded into sysconfig when building Python. So yes, using an older target currently doesn't work unless you also build Python with that same target or even older.

@acolinisi
Copy link
Author

If #36 is merged, the effective target would need to be discovered here the same way that spawn.py does it, i.e.

cur_target = os.environ.get('MACOSX_DEPLOYMENT_TARGET', _cfg_target)

Ok, and _cfg_target should be gotten again in unixccompiler.py with

            _cfg_target = sysconfig.get_config_var(
                                  'MACOSX_DEPLOYMENT_TARGET') or ''

or is there some way to get it directly from spawn.py without duplicating this piece?

@jmroot
Copy link
Contributor

jmroot commented Apr 8, 2021

I guess a common helper function might make sense.

@acolinisi
Copy link
Author

Ok, then, we'll have to block on #36. I'll revisit once that is merged/notmerged.

@acolinisi acolinisi changed the title distutils: pass -rpath on macOS when requested [WIP] distutils: pass -rpath on macOS when requested Apr 8, 2021
@jaraco
Copy link
Member

jaraco commented Apr 24, 2021

#36 is now merged. Also, the main branch now runs the tests in CI. If you merge/rebase with main, you'll see there's a test failure with this patch. Please have a look.

@acolinisi acolinisi force-pushed the PR--rpath-on-macOS branch 3 times, most recently from 71e2348 to 65a5ab4 Compare April 25, 2021 22:07
@acolinisi
Copy link
Author

Rebased, factored into helper function used from the two places (as discussed earlier in this PR), updated and expanded the test case.

There is only one behavior change that I deliberately introduced: before this PR, if sysconfig did not return a version, then the MACOSX_DEPLOYMENT_TARGET env var would be ignored; after this PR, the env var is honored regardless of whether or not sysconfig returns a value (the validation check of version from env var for compatibility with version from sysconfig still happens only when sysconfig returned a version, of course). So, this change only matters for systems where sysconfig does not return a value (there may be no such systems), on all other normal systems, the behavior is entirely unchanged by the refactoring changes (sans the new -Wl,-rpath change).

Tested on macOS 11 on a real extension, with various combinations of the env var value.

@acolinisi acolinisi changed the title [WIP] distutils: pass -rpath on macOS when requested distutils: pass -rpath on macOS when requested Apr 25, 2021
@acolinisi acolinisi force-pushed the PR--rpath-on-macOS branch from 65a5ab4 to a441569 Compare April 25, 2021 23:33
distutils/util.py Outdated Show resolved Hide resolved
distutils/util.py Outdated Show resolved Hide resolved
Fix -R option of build_ext for macOS (darwin)

Resolves this old bug against distutils that expired due to PIP 632:
https://bugs.python.org/issue36353

Applies patch originally submitted to CPython:
python/cpython#12418

Contributor: Toon Verstraelen <[email protected]>
Signed-off-by: Alexei Colin <[email protected]>
@acolinisi acolinisi force-pushed the PR--rpath-on-macOS branch from a441569 to 221e871 Compare April 27, 2021 01:08
@acolinisi
Copy link
Author

Suggested changes to the patch (renames) have been pushed.

@jaraco jaraco merged commit 8f53bf3 into pypa:main May 18, 2021
jaraco added a commit that referenced this pull request Jul 1, 2023
* Replace pep517.build with build

Resolves #30

* Prefer simple usage

Co-authored-by: Jason R. Coombs <[email protected]>
jaraco added a commit that referenced this pull request Jul 1, 2023
* Use `extend-ignore` in flake8 config

This option allows to add extra ignored rules to the default list
instead of replacing it.

The default exclusions are: E121, E123, E126, E226, E24, E704,
W503 and W504.

Fixes #28.

Refs:
* https://github.com/pypa/setuptools/pull/2486/files#r541943356
* https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-extend-ignore
*
https://flake8.pycqa.org/en/latest/user/options.html#cmdoption-flake8-ignore

* Enable complexity limit. Fixes jaraco/skeleton#34.

* Replace pep517.build with build (#37)

* Replace pep517.build with build

Resolves #30

* Prefer simple usage

Co-authored-by: Jason R. Coombs <[email protected]>

* Use license_files instead of license_file in meta (#35)

Singular `license_file` is deprecated since wheel v0.32.0.

Refs:
* https://wheel.readthedocs.io/en/stable/news.html
* https://wheel.readthedocs.io/en/stable/user_guide.html#including-license-files-in-the-generated-wheel-file

Co-authored-by: Jason R. Coombs <[email protected]>
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.

4 participants