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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pillow: Fix build on non-NixOS systems #36323

Merged
merged 1 commit into from Mar 5, 2018
Merged

pillow: Fix build on non-NixOS systems #36323

merged 1 commit into from Mar 5, 2018

Conversation

andrew-d
Copy link
Contributor

@andrew-d andrew-d commented Mar 5, 2018 •
edited

Motivation for this change

The Pillow install script will, by default, add paths like /usr/lib and
/usr/include to the search paths. This can break things when building
on a non-NixOS system that has some libraries installed that are not
installed in Nix (for example, Arch Linux has jpeg2000 but Nix doesn't
build Pillow with this support).

We solve this by telling Pillow to knock it off 馃槃

Fixes #36317

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

cc @timokau

@andrew-d andrew-d requested a review from FRidh as a code owner March 5, 2018 03:18
@Mic92
Copy link
Member

Mic92 commented Mar 5, 2018

@GrahamcOfBorg pythonPackages.pillow python3Packages.pillow

@timokau
Copy link
Member

timokau commented Mar 5, 2018

Wow, that was fast! Thanks

The build now succeeds.

It seems like this problem should have been prevented by sandboxing. I thought I have sandboxing enabled (sandbox = true in ~/nix.conf) but is there a way to confirm nix is actually using sandboxing?

@Mic92
Copy link
Member

Mic92 commented Mar 5, 2018

nix 2.0 has a nix show-config, in older nix versions you can try to download something in the sandbox.

@timokau
Copy link
Member

timokau commented Mar 5, 2018

nix show-config tells me sandboxing = true while nix-info tells me its disabled. Apparently I need to be a trusted user to enable sandboxing? Seems backwards to me, trust should only be needed to disable it right? If thats the case, its also a special case of NixOS/nix#495

@andrew-d
Copy link
Contributor Author

andrew-d commented Mar 5, 2018

@Mic92 - Forgive me if I'm mistaken, but did you forget to add the build prefix in your command to @GrahamcOfBorg?

@Mic92
Copy link
Member

Mic92 commented Mar 5, 2018

@GrahamcOfBorg build pythonPackages.pillow python3Packages.pillow

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Partial log (click to expand)

stripping (with command strip and flags -S) in /nix/store/zvmljpgdkp19v993fwgn5wcvqm5k40xy-python2.7-Pillow-5.0.0/lib
patching script interpreter paths in /nix/store/zvmljpgdkp19v993fwgn5wcvqm5k40xy-python2.7-Pillow-5.0.0
Processing ./Pillow-5.0.0-cp36-cp36m-macosx_10_6_x86_64.whl
Installing collected packages: Pillow
Successfully installed Pillow-5.0.0
/private/tmp/nix-build-python3.6-Pillow-5.0.0.drv-0/Pillow-5.0.0
post-installation fixup
strip is /nix/store/4sdh09gmvl15cy0zb6i7mbvxh5syz206-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/apkg3k5mhgn21cb8ds7658ymbdhlchgl-python3.6-Pillow-5.0.0/lib
patching script interpreter paths in /nix/store/apkg3k5mhgn21cb8ds7658ymbdhlchgl-python3.6-Pillow-5.0.0

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Partial log (click to expand)

Tests/test_tiff_ifdrational.py::Test_IFDRational::test_sanity PASSED     [ 99%]
Tests/test_util.py::TestUtil::test_deferred_error PASSED                 [ 99%]
Tests/test_util.py::TestUtil::test_is_directory PASSED                   [ 99%]
Tests/test_util.py::TestUtil::test_is_not_directory PASSED               [ 99%]
Tests/test_util.py::TestUtil::test_is_not_path PASSED                    [ 99%]
Tests/test_util.py::TestUtil::test_is_not_string_type PASSED             [ 99%]
Tests/test_util.py::TestUtil::test_is_path PASSED                        [ 99%]
Tests/test_util.py::TestUtil::test_is_string_type PASSED                 [100%]

=================== 935 passed, 109 skipped in 12.06 seconds ===================

Mic92
Mic92 reviewed Mar 5, 2018
View reviewed changes
pkgs/development/python-modules/pillow/default.nix Outdated
@@ -31,6 +31,9 @@ buildPythonPackage rec {
++ stdenv.lib.optionals (isPyPy) [ tk libX11 ];

# NOTE: we use LCMS_ROOT as WEBP root since there is not other setting for webp.
# NOTE: we patch the `disable_platform_guessing` setting here, instead of
# passing the `--disable-platform-guessing` command-line option, since the
# command-line option doesn't work when we run tests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also summarize here briefly why, this has to be patched? Basically what you wrote in the commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mic92 - Done; amended and force-pushed the commit.

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Partial log (click to expand)

Tests/test_util.py::TestUtil::test_is_directory PASSED                   [ 99%]
Tests/test_util.py::TestUtil::test_is_not_directory PASSED               [ 99%]
Tests/test_util.py::TestUtil::test_is_not_path PASSED                    [ 99%]
Tests/test_util.py::TestUtil::test_is_not_string_type PASSED             [ 99%]
Tests/test_util.py::TestUtil::test_is_path PASSED                        [ 99%]
Tests/test_util.py::TestUtil::test_is_string_type PASSED                 [100%]

=================== 935 passed, 109 skipped in 52.77 seconds ===================
/nix/store/7vjpszr2sg0ifwnpsyk7nxgig5ddy22i-python2.7-Pillow-5.0.0
/nix/store/nb6qpi1z4fy7cx0wkynfijb1c9k1pvz9-python3.6-Pillow-5.0.0

@andrew-d
pillow: Fix build on non-NixOS systems
8ad2cb1 
The Pillow install script will, by default, add paths like /usr/lib and
/usr/include to the search paths.  This can break things when building
on a non-NixOS system that has some libraries installed that are not
installed in Nix (for example, Arch Linux has jpeg2000 but Nix doesn't
build Pillow with this support).

We solve this by telling Pillow to knock it off 馃槃

Fixes NixOS#36317
@Mic92 Mic92 merged commit 7aad357 into NixOS:master Mar 5, 2018
@andrew-d andrew-d deleted the adunham/fix-pillow branch March 5, 2018 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@andrew-d @Mic92 @timokau @GrahamcOfBorg