distutils icon indicating copy to clipboard operation
distutils copied to clipboard

ENH: Don't add system library directories to rpath

Open DWesl opened this issue 2 years ago • 19 comments

Last of the patches from #73 Might close pypa/setuptools#3257 Might also close pypa/setuptools#3450, depending on what the actual problem looks like

Dual purposes here:

  • On platforms like Windows or Cygwin that don't have rpath, try to avoid adding things to rpath (#73, maybe pypa/setuptools#3450)
  • Some distribution binary package makers require that no shared library list a system library directory (/lib, /lib64, /usr/lib, /usr/lib64) in its rpath because these directories are always in the runtime linker search path; this patch simplifies the code to ensure the shared library can find its dependencies at runtime (pypa/setuptools#3257)

DWesl avatar Jul 31 '22 00:07 DWesl

You are assuming that LIBDIR is in the dynamic linker's default search directories which is not true for some python distributions.

isuruf avatar Jul 31 '22 01:07 isuruf

So this should stay a distribution patch? Should there be a note for packagers in the setuptools release notes that any patches on the CPython distutils should be duplicated on setuptools._distutils? (Yes this is implied by "setuptools now uses its own copy of distutils", but not everyone makes that connection).

DWesl avatar Jul 31 '22 01:07 DWesl

So this should stay a distribution patch?

I’d say no. The goal of Setuptools adoption of distutils is to remove the need for distribution patches. If a distribution wishes to customize the behavior, Setuptools would like to devise a hook such that a distribution can configure that behavior and not have to patch it… although one supported way of customizing behavior is through a patch to sysconfig.

jaraco avatar Jul 31 '22 13:07 jaraco

You are assuming that LIBDIR is in the dynamic linker's default search directories which is not true for some python distributions.

Is there a way to access the dynamic linker's default search directories from python?

DWesl avatar Aug 06 '22 11:08 DWesl

You are assuming that LIBDIR is in the dynamic linker's default search directories which is not true for some python distributions.

Is there a way to access the dynamic linker's default search directories from python?

I don't understand this concern well enough to comment. I'm hoping @isuruf or someone else can comment.

jaraco avatar Aug 13 '22 16:08 jaraco

Is there a way to access the dynamic linker's default search directories from python?

I don't think there is any. I guess there are couple of ways to allow this to work for your use-case.

  1. Introduce a sysconfig variable that downstream python maintainers add to their _sysconfigdata* files
  2. Check for LIBDIR and check that LIBDIR starts with /usr/lib.

isuruf avatar Aug 13 '22 22:08 isuruf

Would it be possible to add a test or tests that capture the missed expectation? That way, even if this solution ends up not being the right one, a subsequent change would still honor the concerns this change is attempting to address?

I think so; the rough outline would be to create a C Extension module linked against some shared library in LIBDIR, check its RUNPATH (not sure how to do that) to make sure that's unset, then try to import it to check whether the runtime loader can find all the dependencies. The first and third bits might already be in the test suite.

Is there a way to access the dynamic linker's default search directories from python?

I don't think there is any. I guess there are couple of ways to allow this to work for your use-case.

  1. Introduce a sysconfig variable that downstream python maintainers add to their _sysconfigdata* files

  2. Check for LIBDIR and check that LIBDIR starts with /usr/lib.

That second one looks like a good starting point.

DWesl avatar Aug 15 '22 16:08 DWesl

Is there a way to access the dynamic linker's default search directories from python?

I don't think there is any. I guess there are couple of ways to allow this to work for your use-case.

1. Introduce a sysconfig variable that downstream python maintainers add to their _sysconfigdata* files

2. Check for LIBDIR and check that LIBDIR starts with `/usr/lib`.

I like idea 1 best, because it gives the end user more control, but the second idea may be suitable too if it captures the essential concern.

jaraco avatar Sep 24 '22 15:09 jaraco

What's the status of this PR? @DWesl are you still working on it? It looks like there was maybe just one lingering concern.

jaraco avatar Jun 27 '24 09:06 jaraco

Could you remind me what that concern is? It's been a bit since I worked on this.

One concern would be all the new tests failing. I should figure out how to compile extensions on Linux, and whether renaming an so causes problems

DWesl avatar Jun 27 '24 13:06 DWesl

After scanning through the messages, I guess I was mistaken. I see you addressed adding a test and of the two options suggested by isuruf, you elected the second. So now it's just a matter of making sure the tests pass. I'll work on it.

jaraco avatar Jun 28 '24 15:06 jaraco

@DWesl Let me know if you want to continue working on this, or if we should abandon it.

jaraco avatar Aug 27 '24 07:08 jaraco

I don't understand why copying /usr/lib/libz.so to /tmp/libxx_z.so and trying to link with -L/tmp -lxx_z fails, so I'm not going to be a lot of help finishing.

If you know a library installed outside /usr/lib*/ on the Linux GHA runners, I can use that get a working test, otherwise it might be simplest to drop the test of rpath for non-system library directories.

EDIT: If you've already got a test of RPATH that this behavior won't break, then this PR is done barring removal of that part of the test.

DWesl avatar Aug 27 '24 13:08 DWesl

Alternately, you could declare this PR tests the new behavior, and tests of old behavior are out of scope, at which point I delete the part of the test that checks the old behavior

DWesl avatar Sep 03 '24 16:09 DWesl

I dropped the test of old behavior, so that test passes, but now ruff fails with the extremely informative message "file would be changed". As I can't run ruff on my machine, I'm leaving that for someone else.

DWesl avatar Sep 11 '24 14:09 DWesl

@isuruf or @lazka Would you be willing to check this change for reasonableness? Tests are passing, but I'm a little uneasy to accept it without a second set of eyes more familiar with the domain. Thanks.

jaraco avatar Sep 11 '24 21:09 jaraco

I don't have much experience with rpath (and why one would set it via setuptools), but I wonder if setup.py explicitly requests rpath to be set why remove some of them again? And what is so special about LIBDIR, and /usr/lib with regards to rpath (what about /usr/lib64 used by fedora?)? and the tests seem unfinished (parametrize with one value, left over sleep call)

Regarding the cygwin part here, I was trying to fix it back then in https://github.com/pypa/distutils/pull/150 but missed that cygwincompiler is not actually used outside of tests (see https://github.com/pypa/distutils/issues/185) so I think we should move the cygwin check to ignore runtime_library_dirs to unixcompiler as was initially intended.

lazka avatar Sep 12 '24 21:09 lazka

if setup.py explicitly requests rpath to be set why remove some of them again?

I think the main reason is that some distributions fail when creating a RPM/DEB package when there is a rpath for the system library dir. See the issues that OP posted. Usually, packages in setup.py add this rpath so that when building packages locally, the package works without having to set LD_LIBRARY_PATH which is a bad thing to do. Ideally packages should only add RPATH if the path is not a package in the dynamic linker's path.

And what is so special about LIBDIR, and /usr/lib with regards to rpath (what about /usr/lib64 used by fedora?)?

/usr/lib and /usr/lib64 are in the dynamic linker's default paths to check. RPATHs just adds to the paths to check, so technically adding them is not necessary.

I'm not sure if removing these directories is the correct thing to do, but I don't think it would break anything.

isuruf avatar Sep 13 '24 17:09 isuruf

if setup.py explicitly requests rpath to be set why remove some of them again?

I think the main reason is that some distributions fail when creating a RPM/DEB package when there is a rpath for the system library dir. See the issues that OP posted. Usually, packages in setup.py add this rpath so that when building packages locally, the package works without having to set LD_LIBRARY_PATH which is a bad thing to do. Ideally packages should only add RPATH if the path is not a package in the dynamic linker's path.

Specifically, it looks like it is distrubution policy not to release a package with the system library paths in the rpath of any shared library, because the system library paths are always searched outside certain specific situations. I suspect they don't like the redundancy, as the space gains would be minor.

And what is so special about LIBDIR, and /usr/lib with regards to rpath (what about /usr/lib64 used by fedora?)?

/usr/lib and /usr/lib64 are in the dynamic linker's default paths to check. RPATHs just adds to the paths to check, so technically adding them is not necessary.

I'm not sure if removing these directories is the correct thing to do, but I don't think it would break anything.

I think the LIBDIR check is a simple check for whether python is a system python, so this doesn't break pythons installed to alternate directories not in the default search paths. That seems to have been inherited from the Fedora patch mentioned in #73.

Since my motivation (crashes on Cygwin) seems to have been alleviated in other ways, the main alternatives would be leaving it to distributions that have such policies to patch setuptools on their own.

DWesl avatar Sep 17 '24 22:09 DWesl