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

pypy3: remove runtime-wrapper #52635

Closed
wants to merge 1 commit into from
Closed

pypy3: remove runtime-wrapper #52635

wants to merge 1 commit into from

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 21, 2018

LD_LIBRARY_PATH can break executables of child processes that are linked
against different libc versions. Since pypy uses cffi for all c-extensions
we no longer need this mechanism as our cc wrapper will set rpath on all
shared objects properly. This makes the build a lot simpler.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Mic92 Mic92 requested a review from FRidh as a code owner December 21, 2018 16:57
@Mic92 Mic92 changed the title pypy3: remove runtime-wrapper [WIP] pypy3: remove runtime-wrapper Dec 21, 2018
@Mic92
Copy link
Member Author

Mic92 commented Dec 21, 2018

Just noticed on a different machine, that I am missing something. Stay tuned.

@Mic92
Copy link
Member Author

Mic92 commented Dec 21, 2018

sys.prefix is incorrect.

LD_LIBRARY_PATH can break executables of child processes that are linked
against different libc versions. Since pypy uses cffi for all c-extensions
we no longer need this mechanism as our cc wrapper will set rpath on all
shared objects properly. This makes the build a lot simpler.
The disadvantage is that we can no longer run tests with this compilation mode.
@Mic92 Mic92 changed the title [WIP] pypy3: remove runtime-wrapper pypy3: remove runtime-wrapper Dec 22, 2018
@Mic92
Copy link
Member Author

Mic92 commented Dec 22, 2018

Ok. Everything works now. I had to disable the tests though the binaries are compiled slighty different when compiling for distribution vs in-tree builds.

pypy3 -c "import sys; print(sys.prefix)"
/nix/store/9za6kr3k9i9fyviycdkrja848qg18qsa-pypy3-6.0.0/share/pypy

@Mic92
Copy link
Member Author

Mic92 commented Dec 22, 2018

cc @andersk

@andersk
Copy link
Contributor

andersk commented Dec 22, 2018

Why pypy3 and not pypy2?

Removing the runtime wrapper seems like generally a good idea, but I don’t see what the path changes have to do with that, and disabling all the tests is disappointing—can we just set LD_LIBRARY_PATH while running the tests?

@Mic92
Copy link
Member Author

Mic92 commented Dec 22, 2018

We already set LD_LIBRARY_PATH during the build. However the way it is currently build one cannot run the tests because the build is not configured for an in-tree build.
Supporting tests would mean compiling pypy twice, which is not meaningful however because one would not test the final binary that goes into production. Before It would set the sys.prefix equal to the build directory even after the installation, which is horrible broken. I started with pypy3 because I need it for my work. I can later look at the other legacy variant too.

@andersk
Copy link
Contributor

andersk commented Dec 22, 2018

I don’t understand. The current package sets sys.prefix to /nix/store/mgakb5cbh4kf7r5456bn5a61jiy66lpw-pypy3-6.0.0/pypy3-c—is there something wrong with that? What exactly is an in-tree build and which of your changes is responsible for altering that configuration?

@Mic92
Copy link
Member Author

Mic92 commented Dec 22, 2018

After removing LD_LIBRARY_PATH it was set to /build/... also I and nix have not spotted the string reference in the binary. The build artifacts are now in a different directory which makes their packaging tool picking them up, but not the tests appearently. I spent the whole day re-compiling pypy. If you want to find a workaround, go for it.

@andersk
Copy link
Contributor

andersk commented Dec 22, 2018

I built pypy3 (edit: and pypy2) with just the wrapper removed and no other changes. It works as well as it did before.

diff --git a/pkgs/development/interpreters/python/pypy/3/default.nix b/pkgs/development/interpreters/python/pypy/3/default.nix
index 23e239d925b..7a68ea43628 100644
--- a/pkgs/development/interpreters/python/pypy/3/default.nix
+++ b/pkgs/development/interpreters/python/pypy/3/default.nix
@@ -1,7 +1,7 @@
 { stdenv, substituteAll, fetchurl
 , zlib ? null, zlibSupport ? true, bzip2, pkgconfig, libffi
 , sqlite, openssl, ncurses, python, expat, tcl, tk, tix, xlibsWrapper, libX11
-, makeWrapper, callPackage, self, gdbm, db, lzma
+, callPackage, self, gdbm, db, lzma
 , python-setup-hook
 # For the Python package set
 , packageOverrides ? (self: super: {})
@@ -26,7 +26,7 @@ in stdenv.mkDerivation rec {
     sha256 = "0lwq8nn0r5yj01bwmkk5p7xvvrp4s550l8184mkmn74d3gphrlwg";
   };
 
-  nativeBuildInputs = [ pkgconfig makeWrapper ];
+  nativeBuildInputs = [ pkgconfig ];
   buildInputs = [
     bzip2 openssl pythonForPypy libffi ncurses expat sqlite tk tcl xlibsWrapper libX11 gdbm db lzma
   ] ++ stdenv.lib.optional (stdenv ? cc && stdenv.cc.libc != null) stdenv.cc.libc
@@ -96,15 +96,6 @@ in stdenv.mkDerivation rec {
     ln -s $out/pypy3-c/include $out/include/${libPrefix}
     ln -s $out/pypy3-c/lib-python/3 $out/lib/${libPrefix}
 
-    # We must wrap the original, not the symlink.
-    # PyPy uses argv[0] to find its standard library, and while it knows
-    # how to follow symlinks, it doesn't know about wrappers. So, it
-    # will think the wrapper is the original. As long as the wrapper has
-    # the same path as the original, this is OK.
-    wrapProgram "$out/pypy3-c/pypy3-c" \
-      --set LD_LIBRARY_PATH "${LD_LIBRARY_PATH}:$out/lib" \
-      --set LIBRARY_PATH "${LIBRARY_PATH}:$out/lib"
-
     # verify cffi modules
     $out/bin/pypy3 -c "import tkinter;import sqlite3;import curses;import lzma"
 

@Mic92
Copy link
Member Author

Mic92 commented Dec 23, 2018

Sorry I could not test your change.
The following test keeps failing for me on different machines:

test_reorganize (test.test_dbm_gnu.TestGdbm) ... FAIL

======================================================================
FAIL: test_reorganize (test.test_dbm_gnu.TestGdbm)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/pypy-pypy-fdd60ed87e94/lib-python/3/test/test_dbm_gnu.py", line 74, in test_reorganize
    self.assertTrue(size0 < size1)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 5 tests in 0.415s

FAILED (failures=1)
1 test failed:
    test_dbm_gnu
test test_dbm_gnu failed

=========================== short test summary info ============================
FAIL lib-python/3/test/test_dbm_gnu.py::unmodified

@FRidh
Copy link
Member

FRidh commented Jan 3, 2019

I've added derivations for using prebuilt PyPy to #53123. Using those for translating/building speeds it up considerably. Also, what do you think of separating the translating/building from the installing (and testing)?

@Mic92
Copy link
Member Author

Mic92 commented Jan 3, 2019

That might help. I guess I can close this for now as I would need to re-add stuff on top of https://github.com/NixOS/nixpkgs/pull/53123/files anyway.

@Mic92 Mic92 closed this Jan 3, 2019
@Mic92 Mic92 deleted the pypy3-simplification branch January 3, 2019 20:16
@andersk andersk mentioned this pull request Jan 15, 2019
10 tasks
@andersk
Copy link
Contributor

andersk commented Jan 15, 2019

I opened a PR for my more minimal change: #54013.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants