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

bug: fix libnvidia-container build #101665

Merged
merged 1 commit into from Nov 3, 2020
Merged

Conversation

@cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Oct 25, 2020

Motivation for this change

This change fixes the build of libnvidia-container.

Closes #101517.

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.
@cpcloud
Copy link
Contributor Author

@cpcloud cpcloud commented Oct 25, 2020

@GuillaumeDesforges Any idea what I should test here?

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

@ofborg build libnvidia-container

EDIT: wrong, it's nvidia-docker

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

@ofborg build nvidia-docker

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

Seems to build fine.

What did you run to test locally ?

@cpcloud
Copy link
Contributor Author

@cpcloud cpcloud commented Oct 26, 2020

@GuillaumeDesforges Nothing thorough. I ran all the binaries without arguments to make sure that works.

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

@GuillaumeDesforges Nothing thorough. I ran all the binaries without arguments to make sure that works.

What derivation did you build ?

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

Could you please edit the commit message from libnividia-docker to nvidia-docker since it is the name of the attribute fixed ?

@cpcloud
Copy link
Contributor Author

@cpcloud cpcloud commented Oct 26, 2020

@GuillaumeDesforges Nothing thorough. I ran all the binaries without arguments to make sure that works.

What derivation did you build ?

I ran nix build -f. nvidia-docker from a nixpkgs clone.

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Oct 26, 2020

Thanks :) sorry for the bother, I just wanted to ensure that we were talking about the same thing

Looks good to me!

Who should be reviewing/merging this ?
I can't see a maintainer list in the meta attribute of the nvidia-docker derivation, it would be nice to add it maybe ?

poking several people who could be interested: @volth @alyssais @Mic92 @Philipp-M

@theduke
Copy link
Contributor

@theduke theduke commented Oct 29, 2020

Would really love to see this merged.
Anyone else we can poke to move things along?

@theduke
Copy link
Contributor

@theduke theduke commented Oct 29, 2020

Actually, on second look, the current nvidia-docker derivation is really outdated.

How it works has changed a bit and the currently used versions are old.
It no longer uses a fork of runc but just a wrapper. Also hook repository was renamed to nvidia-container-toolkit.

I'm working on updating it.
I have something that builds, but it's not able to start containers with GPU access yet.
I'd appreciate some discussion via IRC if anyone wants to work on this together.

@GuillaumeDesforges
Copy link
Contributor

@GuillaumeDesforges GuillaumeDesforges commented Nov 2, 2020

@theduke thanks ! An update would be awesome.

I think it would be wise, still, to merge this for nvidia-docker to at least be usable, until your update is merged.

@cpcloud
Copy link
Contributor Author

@cpcloud cpcloud commented Nov 2, 2020

@GuillaumeDesforges How does one get anything merged around here?

@theduke
Copy link
Contributor

@theduke theduke commented Nov 2, 2020

@theduke thanks ! An update would be awesome.

I think it would be wise, still, to merge this for nvidia-docker to at least be usable, until your update is merged.

Agreed.
I hope to finalize a PR on the weekend, but that might require a bit of discussion.

This PR should still be merged.

@Ekleog Ekleog closed this Nov 3, 2020
@Ekleog Ekleog reopened this Nov 3, 2020
@Ekleog Ekleog merged commit ea270c0 into NixOS:master Nov 3, 2020
22 of 23 checks passed
@Ekleog
Copy link
Member

@Ekleog Ekleog commented Nov 3, 2020

LGTM, thank you! :)

@cpcloud cpcloud deleted the libnvidia-container-build branch Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants