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

buildBazelPackage: add "fetchConfigured" for fetching only things required for the build #92302

Merged
merged 1 commit into from Jul 17, 2020
Merged

buildBazelPackage: add "fetchConfigured" for fetching only things required for the build #92302

merged 1 commit into from Jul 17, 2020

Conversation

lukegb
Copy link
Contributor

@lukegb lukegb commented Jul 4, 2020 •
edited

Motivation for this change

"bazel fetch" will, by default, fetch everything that might be used,
including things that will later be discarded due to the way the build
is configured.

Concretely, this means that for some builds of Java packages, this will
avoid failures where the builder tries to retrieve the JDK from /usr/share/java
(or equivalent).

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.

@lukegb
Copy link
Contributor Author

lukegb commented Jul 4, 2020

@ofborg build gvisor python35Packages.dm-sonnet bazel-watcher python35Packages.tensorflow-probability python35Packages.tensorflow python35Packages.tensorflow-build_2

@lukegb
Copy link
Contributor Author

lukegb commented Jul 4, 2020

@ofborg build gvisor python37Packages.dm-sonnet bazel-watcher python37Packages.tensorflow-probability python37Packages.tensorflow python37Packages.tensorflow-build_2

@lukegb
buildBazelPackage: add "fetchConfigured" for fetching only things req…
6cb452b 
…uired for the build

"bazel fetch" will, by default, fetch everything that _might_ be used,
including things that will later be discarded due to the way the build
is configured.

Concretely, this means that for some builds of Java packages, this will
avoid failures where the builder tries to retrieve the JDK from /usr/share/java
(or equivalent).

This also means that for most packages we can fetch _fewer_ dependencies,
since the standard tree pruning for artifacts to fetch will take effect.

fetchConfigured is disabled by default since it changes the fetch hashes
of tensorflow/tensorflow2 (since it ends up fetching less).
@lukegb lukegb marked this pull request as ready for review July 5, 2020 12:42
@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

This sounds really useful. I wonder if it should become the default?

Currently, there's a lot of packages in nixpkgs removing stuff in their fetchAttrs.preInstall, and some of this could probably be removed, leading to more binary reproducibility of the bazel fetch phase.

@lukegb
Copy link
Contributor Author

lukegb commented Jul 11, 2020

I wanted to make it the default, but I decided I didn't want to update the fetch hashes of everything in one go (e.g. Tensorflow's hash changes, and possibly some other things too)

@flokli
Copy link
Contributor

flokli commented Jul 11, 2020 •
edited

I meant, while flipping the default, also getting rid of the now obsolete "cleanup commands" in fetchAttrs.preInstall.

Happy to leave this as a follow-up, too, but I assume it'd clean up many footguns and do a great deal w.r.t. changing hashes.

uri-canva
uri-canva approved these changes Jul 12, 2020
View reviewed changes
Copy link
Contributor

@uri-canva uri-canva left a comment

Choose a reason for hiding this comment

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

Sounds very good to me, also agree that the fetch should be removed once all the derivations are switched to use build --nobuild instead.

@Profpatsch Profpatsch merged commit d7d4a92 into NixOS:master Jul 17, 2020
@uri-canva uri-canva mentioned this pull request Nov 25, 2020
Closed
@lukegb lukegb deleted the fetchBazel branch November 25, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uri-canva uri-canva uri-canva approved these changes

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

@guibou guibou Awaiting requested review from guibou

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lukegb @flokli @uri-canva @Profpatsch