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

cupy: Move cudatoolkit to propagatedNativeBuildInputs to fix Thrust #77708

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Jan 14, 2020

Motivation for this change

The nvcc binary needs to be found in PATH at build time for Thrust support to be enabled.

Test case: check the cupy.cuda.thrust_enabled boolean, or try to use cupy.sort, which doesn’t work without Thrust.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @hyphon81

The nvcc binary needs to be found in PATH at build time for Thrust
support to be enabled.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@@ -18,8 +18,11 @@ buildPythonPackage rec {
mock
];

propagatedBuildInputs = [
propagatedNativeBuildInputs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's just for nvcc to be on the path, this makes sense. Is there more that cudatoolkit provides?

My main concern is for cross compilation, in which it will get a binary of the wrong architecture.

However, seeing as linuxPackages.nvidia_x11 gets propagated as well, this expression probably isn't very cross compilation friendly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cudatoolkit has both nvcc used at build time and libraries linked at runtime. I have no idea whether it might support cross compilation. But the question is entirely moot at this time, as we only package it for Linux x86-64.

Copy link
Member

Choose a reason for hiding this comment

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

cudatoolkit has both nvcc used at build time and libraries linked at runtime.

If cudatoolkit doesn't contain any python libraries then it should not be in propagated. Now for libraries to link it should be in buildInputs. I believe, there is some strange thing that may be specific to python derivations, where binaries may not be visible unless they are also in nativeBuildInputs [1]. I never dug into this, but it seems like a bug in nixpkgs, so you might need to also have it in nativeBuildInputs as a workaround.

[1] #64047 (comment)

@veprbl
Copy link
Member

veprbl commented Mar 30, 2020

@andersk Could you please rebase.

@stale
Copy link

stale bot commented Jun 7, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 7, 2021
@wegank wegank marked this pull request as draft March 20, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

None yet

4 participants