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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

python3Packages.pyopencl: fix build #56082

Merged
merged 2 commits into from
Mar 13, 2019
Merged

python3Packages.pyopencl: fix build #56082

merged 2 commits into from
Mar 13, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 20, 2019 •
edited
Loading

Motivation for this change

Fix the recently broken build by adding pybind11 and pkgs.pybind11
to the build. Also set $HOME to a temporary directory during the build
to avoid "Permission denied" errors in the build script.

This also unbreaks sasview and pybitmessage.

See also https://hydra.nixos.org/build/89037506

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.

@Ma27 Ma27 requested a review from FRidh as a code owner February 20, 2019 09:08
@FRidh
Copy link
Member

FRidh commented Feb 21, 2019

Why are both needed? That seems a bit weird.

FRidh
FRidh reviewed Feb 21, 2019
View reviewed changes
pkgs/development/python-modules/pyopencl/default.nix Outdated

propagatedBuildInputs = [ numpy cffi pytools decorator appdirs six Mako ];
propagatedBuildInputs = [ numpy cffi pytools decorator appdirs six Mako pybind11 ];
Copy link
Member

Choose a reason for hiding this comment

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

The Python pybind11 should go into nativeBuildInputs

Copy link
Member

Choose a reason for hiding this comment

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

No, wait, this is non-native because it will be linked to, so buildInputs. @dotlambda seems we have a case here where setup_requires does not map to nativeBuildInputs.

Copy link
Member

Choose a reason for hiding this comment

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

But shouldn't that linking be covered by pkgs.pybind11? Maybe it's our pybind11 packaging that needs fixing.

Copy link
Member

Choose a reason for hiding this comment

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

The Python version of the package essentially has the same data, the headers. We have to use the Python version because it's added in setup_requires, but other then that, we should be able to use any of the two in buildInputs.

Copy link
Member

Choose a reason for hiding this comment

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

I see. How about we just patch it out of setup_requires?

Copy link
Member

Choose a reason for hiding this comment

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

@Ma27 pybind11 should be moved to buildInputs, shouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right. Updated the patch accordingly.

@Ma27
Copy link
Member Author

Ma27 commented Feb 21, 2019

WIthout pkgs.pybind11 as build input the compilation fails with the following errors, so I'm not sure if the Python version will work:

gcc -Wno-unused-result -Wsign-compare -fwrapv -Wall -O3 -DNDEBUG -fPIC -DPYGPU_PACKAGE=pyopencl -DPYGPU_PYOPENCL=1 -Ipybind11/include -I/nix/store/6la3fblc497pacyy0vxhnjq1l1wlb5w7-python3-3.7.2/include/python3.7m -I/build/tmp.aabYVn0RAS/.local/include/python3.7m -I/nix/store/rirgscxdd9r8kgady8ilgpv6r7yvk388-python3.7-numpy-1.16.0/lib/python3.7/site-packages/numpy/core/include -I/nix/store/rirgscxdd9r8kgady8ilgpv6r7yvk388-python3.7-numpy-1.16.0/lib/python3.7/site-packages/numpy/core/include -I/nix/store/6la3fblc497pacyy0vxhnjq1l1wlb5w7-python3-3.7.2/include/python3.7m -c src/wrap_constants.cpp -o build/temp.linux-x86_64-3.7/src/wrap_constants.o -fvisibility=hidden -DVERSION_INFO="2018.2.2" -std=c++14 -fvisibility=hidden
In file included from src/wrap_cl.hpp:86:0,
                 from src/wrap_constants.cpp:27:
src/wrap_helpers.hpp:31:10: fatal error: pybind11/pybind11.h: No such file or directory
 #include <pybind11/pybind11.h>
          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
error: command 'gcc' failed with exit status 1
builder for '/nix/store/289qbw2j1a2n2a7b4cpbfl2r4hxln6y8-python3.7-pyopencl-2018.2.2.drv' failed with exit code 1

If I get this right, we want to patch the pybind11 reference away from setup_requires and only use pkgs.pybind11 for compilation?

@dotlambda
Copy link
Member

If I get this right, we want to patch the pybind11 reference away from setup_requires and only use pkgs.pybind11 for compilation?

Correct.

@Ma27
Copy link
Member Author

Ma27 commented Feb 21, 2019

Hmm I'm afraid that this isn't that easy: we need pkgs.pybind11 as build input (otherwise the compilation will fail as posted above), however we need pythonPackages.pybind11 as well, otherwise the build will break with the following error:

...
File "setup.py", line 51, in __str__
    import pybind11
ModuleNotFoundError: No module named 'pybind11'

Admittedly I'm not that experienced with the pybind11 integration, so I'm not 100% sure what else to do (instead of using the current approach from this PR).

@FRidh FRidh self-assigned this Feb 21, 2019
@dotlambda
Copy link
Member

dotlambda commented Feb 21, 2019 •
edited
Loading 

...
File "setup.py", line 51, in __str__
    import pybind11
ModuleNotFoundError: No module named 'pybind11'

That probably comes from https://github.com/inducer/pyopencl/blob/0faf48a10edd086c783f794a6decdf85e8dc625b/aksetup_helper.py#L894. We'd need to patch that function to return "${pkgs.pybind11}/include".
Not sure it's worth the hassle.

@Ma27
Copy link
Member Author

Ma27 commented Feb 21, 2019

I see. If @FRidh prefers the latter solution you proposed, I'd implement this, otherwise I'd vote for merging this :)

@FRidh
Copy link
Member

FRidh commented Feb 21, 2019 •
edited
Loading

pybind11/__init__.py:get_include needs to be fixed because it returns the path to the headers folder of ${python3} instead of ${python3.pkgs.pybind11}.

@Ma27 Ma27 mentioned this pull request Mar 5, 2019
Closed
10 tasks
@Ma27
Copy link
Member Author

Ma27 commented Mar 5, 2019

Cross-referencing #56826 (the build is broken on the new release as well).

@knedlsepp
Copy link
Member

knedlsepp commented Mar 5, 2019 •
edited
Loading

See also pybind/pybind11#1426 for a request to fix get_include.

@FRidh
Copy link
Member

FRidh commented Mar 5, 2019

@Ma27 we might use that PR as a patch.

@Ma27
Copy link
Member Author

Ma27 commented Mar 7, 2019

@FRidh applied the pybind11 patch and updated.

@GrahamcOfBorg GrahamcOfBorg requested a review from FRidh March 7, 2019 22:00
@Ma27
pythonPackages.pybind11: apply patch to properly get headers directories
94c3ac2 
It seems as the `pybind11` build code returns the Python headers
directory (where the `pybind11` headers are stored as well on traditional
setups) rather than returning the dedicated prefix[1].

An exemplary fallout is the broken build of `pyopencl`[2].

[1] pybind/pybind11#1425
[2] NixOS#56082
@Ma27
Copy link
Member Author

Ma27 commented Mar 9, 2019

@dotlambda anything else to do? :)

@Ma27
python3Packages.pyopencl: fix build
d237919 
Fix the recently broken build by adding `pybind11`
to the build. Also set $HOME to a temporary directory during the build
to avoid "Permission denied" errors in the build script.

This also unbreaks `sasview` and `pybitmessage`.

See also NixOS#56826
See also https://hydra.nixos.org/build/89037506
@dotlambda dotlambda merged commit 098cb9f into NixOS:master Mar 13, 2019
dotlambda pushed a commit that referenced this pull request Mar 13, 2019
@Ma27
pythonPackages.pybind11: apply patch to properly get headers directories
2d8b128 
It seems as the `pybind11` build code returns the Python headers
directory (where the `pybind11` headers are stored as well on traditional
setups) rather than returning the dedicated prefix[1].

An exemplary fallout is the broken build of `pyopencl`[2].

[1] pybind/pybind11#1425
[2] #56082

(cherry picked from commit 94c3ac2)
@dotlambda
Copy link
Member

cherry picked in 2d8b128 and 68c73b9

@Ma27 Ma27 deleted the fix-pyopencl branch March 14, 2019 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda left review comments

@FRidh FRidh Awaiting requested review from FRidh

Assignees

@FRidh FRidh

Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ma27 @FRidh @dotlambda @knedlsepp @GrahamcOfBorg