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

Python 3.6 depends on ld at run-time #25763

Closed
nh2 opened this issue May 13, 2017 · 19 comments
Closed

Python 3.6 depends on ld at run-time #25763

nh2 opened this issue May 13, 2017 · 19 comments

Comments

@nh2
Copy link
Contributor

nh2 commented May 13, 2017

Issue description

Python 3.6 fixed Python bug ctypes find_library should search LD_LIBRARY_PATH on Linux.

When you call e.g. find_library("m"), it splits LD_LIBRARY_PATH by colons (:), and for each entry /path/to/entry/e shelling out to ld, like:

ld -t -L /path/to/entry/e -o /dev/null -lm

The code: https://hg.python.org/cpython/rev/385181e809bc

Note that the code fails silently:

            except Exception as e:
                pass  # result will be None

We must add binutils to python36's (and newer) runtime to make it find_library() work.

Steps to reproduce

With binutils providing ld in PATH:

# nix-shell --pure -p python36 binutils --run "LD_LIBRARY_PATH=$(nix-build --no-out-link -E '(import <nixpkgs> {}).glibc')/lib python -c \"from ctypes.util import *; print(find_library('m'))\""
libm.so.6

Without:

# nix-shell --pure -p python36 --run "LD_LIBRARY_PATH=$(nix-build --no-out-link -E '(import <nixpkgs> {}).glibc')/lib python -c \"from ctypes.util import *; print(find_library('m'))\""
None

Technical details

  • System: 17.03
@nh2
Copy link
Contributor Author

nh2 commented May 13, 2017

CC @FRidh

@7c6f434c
Copy link
Member

Did you check that OpenSSL or something else non-default also gets found when binutils are in PATH? Or does empty LD_LIBRARY_PATH break the test? Because libm could be found for wrong reasons.

@nh2
Copy link
Contributor Author

nh2 commented May 13, 2017

Did you check that OpenSSL or something else non-default also gets found when binutils are in PATH? Or does empty LD_LIBRARY_PATH break the test? Because libm could be found for wrong reasons.

OK, another example:

# nix-shell --pure -p python36 binutils --run "LD_LIBRARY_PATH=$(nix-build --no-out-link -E '(import <nixpkgs> {}).imagemagick')/lib python -c \"from ctypes.util import *; print(find_library('MagickCore-6.Q16'))\""
libMagickCore-6.Q16.so.4

# nix-shell --pure -p python36 --run "LD_LIBRARY_PATH=$(nix-build --no-out-link -E '(import <nixpkgs> {}).imagemagick')/lib python -c \"from ctypes.util import *; print(find_library('MagickCore-6.Q16'))\""
None

@nh2
Copy link
Contributor Author

nh2 commented May 13, 2017

It seems that this was already bugged before; Python (all versions) also tries to use the following programs, none of which are available in nix-shell --pure -p python36:

  • /bin/ldconfig
  • gcc
  • objdump

@7c6f434c
Copy link
Member

I think it is OK if some of its attempts fail, we just need one approach to succeed reliably.

@FRidh FRidh self-assigned this May 14, 2017
@FRidh
Copy link
Member

FRidh commented May 14, 2017

Within Nixpkgs we do not use ctypes and patch all code that uses it to use a fixed path to a library instead. However, I can imagine we should support this, especially for those working with non-Nix-managed Python packages.

Adding binutils does add 26 MB to the closure size, and it would require wrapping the Python interpreter which I prefer not to. Adding those other programs you mention would make the closure size even larger.

How about we instead patch the interpreter to error when none of these programs are on PATH?

@7c6f434c
Copy link
Member

7c6f434c commented May 14, 2017 via email

@nh2
Copy link
Contributor Author

nh2 commented May 14, 2017

However, I can imagine we should support this, especially for those working with non-Nix-managed Python packages.

@FRidh I agree, as it can become very difficult and time intensive to keep large/complicated projects (like glusterfs, packaging which I found this issue) patched.

Adding binutils does add 26 MB to the closure size, and it would require the Python interpreter which I prefer not to. Adding those other programs you mention would make the closure size even larger.

How about we instead patch the interpreter to error when none of these programs are on PATH?

@FRidh Hmm, this sounds problematic to me. If upstream says "requires one of {ld,objdump,gcc} to run as intended", I don't think we should say "no, it doesn't" in the name of closure sizes. Especially not because it is easy to go unnoticed; Python devs may think "we already have a dependency on ld, so we can use it in this other place for this other feature as well", and then we might have another thing that silently fails at run-time on NixOS.
To me, it seems like an upstream problem that when they want the Python distribution to be lightweight, they should minimise their dependencies.

Another solution might be to make Python 3.6 depend on a slimmed down version of binutils that only provies what Python needs (ld by itself seems quite a lot smaller than the full 26 MB).

and it would require the Python interpreter which I prefer not to

@FRidh I don't understand this part, what do you mean here? It (Python 3.6) ... would require a Python interpreter?

@nh2
Copy link
Contributor Author

nh2 commented May 14, 2017

There is also «fakeld» path, we don't need ld to work, just to search for libraries in LD_LIBRARY_PATH, which can be implemented by a small shell script.

@7c6f434c I am not quite sure what exactly you have in mind, but the Python docs for find_library say that it should work in the same way as "the linker on the system" -- which Python implements by using the real linker of the system. For example, it will return find_library('c') == "libc.so.6", which (I guess) a small shell script wouldn't do.

@FRidh
Copy link
Member

FRidh commented May 14, 2017

@FRidh Hmm, this sounds problematic to me. If upstream says "requires one of {ld,objdump,gcc} to run as intended", I don't think we should say "no, it doesn't" in the name of closure sizes. Especially not because it is easy to go unnoticed; Python devs may think "we already have a dependency on ld, so we can use it in this other place for this other feature as well", and then we might have another thing that silently fails at run-time on NixOS.
To me, it seems like an upstream problem that when they want the Python distribution to be lightweight, they should minimise their dependencies.

These are not hard dependencies. If they were, they would either require them already at build-time or at the least cause an error when none were found during run-time.

@FRidh I don't understand this part, what do you mean here? It (Python 3.6) ... would require a Python interpreter?

Fixed it. I meant I do not think we should wrap the interpreter to add these tools to PATH.

@FRidh
Copy link
Member

FRidh commented May 14, 2017

@FRidh I agree, as it can become very difficult and time intensive to keep large/complicated projects (like glusterfs, packaging which I found this issue) patched.

Note that expressions inside Nixpkgs should patch references to libraries when possible, instead of wrapping LD_LIBRARY_PATH.

@7c6f434c
Copy link
Member

7c6f434c commented May 14, 2017

For example, it will return find_library('c') == "libc.so.6", which (I guess) a small shell script wouldn't do.

IFS=: for d in $LD_LIBRARY_PATH; do
  n="$d/lib$name.so";
  if test -e "$n"; then
    readlink -f "$n";
    break; 
  fi;
done

will actually be close enough.

@nh2
Copy link
Contributor Author

nh2 commented May 14, 2017

will actually be close enough.

@7c6f434c That seems to simply resolve symlinks, which is not enough:

Have a look into what the contents of libc.so are:

# cat /nix/store/vn6fkjnfps37wa82ri4mwszwvnnan6sk-glibc-2.25/lib/libc.so
/* GNU ld script
   Use the shared library, but some functions are only in
   the static library, so try that secondarily.  */
OUTPUT_FORMAT(elf64-x86-64)
GROUP ( /nix/store/vn6fkjnfps37wa82ri4mwszwvnnan6sk-glibc-2.25/lib/libc.so.6 /nix/store/vn6fkjnfps37wa82ri4mwszwvnnan6sk-glibc-2.25/lib/libc_nonshared.a  AS_NEEDED ( /nix/store/vn6fkjnfps37wa82ri4mwszwvnnan6sk-glibc-2.25/lib/ld-linux-x86-64.so.2 ) )

It's a text file! (A linker script.) Loading this with CDLL will fail, it only loads real .so ELF files.

This is why I'd shy away from replacing upstream logic with "simpler solutions" in NixOS: If it was that simple, upstream could do the simple way themselves.

@7c6f434c
Copy link
Member

Well, I wonder if taking the longest lib$name.so.* would actually work in practice, but creating a separate binutils output for ld is a better idea.

@copumpkin
Copy link
Member

would require wrapping the Python interpreter which I prefer not to

As another data point, wrapping the python interpreter would be a big annoyance on Darwin, which doesn't allow shebang destinations (which is pretty much the main thing the python interpreter is used for) to themselves be shebang'd scripts. So basically it would break everything unless we did #23018 as well.

@nh2
Copy link
Contributor Author

nh2 commented May 17, 2017

So basically it would break everything unless we did #23018 as well.

Oh well, that sounds like a harder issue to tackle.

Then I guess we should patch python (and maybe afterwards upstream a configure option to pass in the path to ld or a general path to where it shall look for such binaries).

Does everybody agree with this?

@FRidh
Copy link
Member

FRidh commented May 17, 2017

@nh2 what exactly do you want to modify then?

Python can work fine without these optional dependencies, as long as they're on PATH. If you need them in a Nix expression, then you can just add them to the buildInputs of your expression. If you need them outside a Nix expression, then you can wrap the Python interpreter so that they're added to its PATH. Inside Nixpkgs there is no need for adding these dependencies to PATH because we hardcode paths.

The only patch I could imagine is to make it fail loudly if it needs to find a library and none of these tools are available.

@edolstra
Copy link
Member

👎 on adding binutils to the closure. The few Python programs that need this can easily add binutils to PATH themselves, or we can patch Python to implement library searching in a better way.

Python's way to find libraries is batshit insane. It will even run gcc at runtime to find libraries, significantly slowing down startup. For Python 2.7, we have a patch that improves this: de1b4e7. But I never ported this to Python 3. (So on Python 3, doing import uuid will still cause Python to run ldconfig and gcc.)

BTW, wrapper scripts are generally a kludge. We have the source of Python, so we can patch the problem in Python itself (as de1b4e7 does). It would be more efficient to patch Python to check the elements of LD_LIBRARY_PATH for lib<foo>.so directly, without using external programs.

nh2 added a commit to nh2/nixpkgs that referenced this issue May 19, 2017
Done by setting PATH and PYTHONPATH appropriately.

Adds the following patches:

* One that removes hardcodes to /sbin, /usr/bin, etc.
  from gluster, so that programs like `lvm` and `xfs_info` can be
  called at runtime; see https://bugzilla.redhat.com/show_bug.cgi?id=1450546.
* One that fixes unsubstituted autoconf macros in paths (a problem
  in the 3.10 release); see https://bugzilla.redhat.com/show_bug.cgi?id=1450588.
* One that removes uses of the `find_library()` Python function that does
  not behave as expected in Python < 3.6 (and would not behave correctly
  even on 3.6 in nixpkgs due to NixOS#25763);
  see https://bugzilla.redhat.com/show_bug.cgi?id=1450593.

I think that all of these patches should be upstreamed.

Also adds tests to check that none of the Python based utilities
throw import errors, calling `--help` or equivalent on them.
qknight pushed a commit that referenced this issue Jun 27, 2017
Done by setting PATH and PYTHONPATH appropriately.

Adds the following patches:

* One that removes hardcodes to /sbin, /usr/bin, etc.
  from gluster, so that programs like `lvm` and `xfs_info` can be
  called at runtime; see https://bugzilla.redhat.com/show_bug.cgi?id=1450546.
* One that fixes unsubstituted autoconf macros in paths (a problem
  in the 3.10 release); see https://bugzilla.redhat.com/show_bug.cgi?id=1450588.
* One that removes uses of the `find_library()` Python function that does
  not behave as expected in Python < 3.6 (and would not behave correctly
  even on 3.6 in nixpkgs due to #25763);
  see https://bugzilla.redhat.com/show_bug.cgi?id=1450593.

I think that all of these patches should be upstreamed.

Also adds tests to check that none of the Python based utilities
throw import errors, calling `--help` or equivalent on them.
@FRidh
Copy link
Member

FRidh commented Jul 29, 2017

Closing this since this won't be changed. Do see #27751.

@FRidh FRidh closed this as completed Jul 29, 2017
@tbenst tbenst mentioned this issue Oct 2, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants