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

nixos: add preferLocalBuild=true; on derivations for config files and closureInfo #50504

Merged
merged 1 commit into from Feb 22, 2019
Merged

nixos: add preferLocalBuild=true; on derivations for config files and closureInfo #50504

merged 1 commit into from Feb 22, 2019

Conversation

symphorien
Copy link
Member

Motivation for this change

When using remote builders, nix would copy the closure of the config files to the remote builder when only the path is really needed, which can take a sizeable amount of time depending on the network speed. This is notably annoying for closure-info.

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) : I tested the installer test only; it works except one test which fails with
Path length (109) is longer than maximum supported length (108) and will be truncated at /nix/store/0ingn8cwwnl84i374hcl6nafsm2c5m2p-perl-5.28.0/lib/perl5/5.28.0/x86_64-linux-thread-multi/Socket.pm >
boot-after-install# qemu-system-x86_64: -monitor unix:./monitor: Failed to connect socket ./monitor: No such file or directory

which is independent of this change (it was failing before)

  • 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)
  • Fits CONTRIBUTING.md.

Tested on top of current nixos-unstable, not tested after rebase on master.

@matthewbauer
Copy link
Member

Looks okay to me. Not to bikeshed here but I wonder if runCommand should just always do preferLocalBuild? Those are rarely big enough for it to be worthwhile to do remotely.

@rycee
Copy link
Member

rycee commented Nov 17, 2018

Perhaps allowSubstitutes = false also should be included?

@symphorien
Copy link
Member Author

What is the purpose of allowSubstitutes = false ?

@rycee
Copy link
Member

rycee commented Nov 17, 2018 •
edited

@symphorien If it is false then the derivation will always be built, no substitution will be attempted. I.e., no network traffic. At least that is my interpretation of the description in the manual.

@symphorien
Copy link
Member Author

So I tried to add allowSubstitutes=false. Installer tests fail this way:

machine: must succeed: nixos-install < /dev/null >&2
machine# > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > building the configuration in /mnt/etc/nixos/configuration.nix...
warning: unable to download 'https://rt.perl.org/Public/Ticket/Attachment/1502646/807252/0001-Fix-missing-build-dependency-for-pods.patch': Timeout was reached (28); retrying in 342 ms
warning: unable to download 'https://rt.perl.org/Public/Ticket/Attachment/1502646/807252/0001-Fix-missing-build-dependency-for-pods.patch': Timeout was reached (28); retrying in 669 ms
warning: unable to download 'https://rt.perl.org/Public/Ticket/Attachment/1502646/807252/0001-Fix-missing-build-dependency-for-pods.patch': Timeout was reached (28); retrying in 1018 ms
warning: unable to download 'https://rt.perl.org/Public/Ticket/Attachment/1502646/807252/0001-Fix-missing-build-dependency-for-pods.patch': Timeout was reached (28); retrying in 2370 ms
builder for '/nix/store/4x3famhx2zy5q0khadzdj2wsp3glg6n3-0001-Fix-missing-build-dependency-for-pods.patch.drv' failed with exit code 1; last 1 log lines:
machine#   error: unable to download 'https://rt.perl.org/Public/Ticket/Attachment/1502646/807252/0001-Fix-missing-build-dependency-for-pods.patch': Timeout was reached (28)
cannot build derivation '/nix/store/3nnfzfb8a9kw19k28fyznjxl9wd4nmix-perl-5.28.0.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/0d2w8f99r9f1r469b0c4mxm9wykdslwk-bison-3.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/sgyk07dap5m36zp7960ajnsm42fdqfc1-binutils-2.30.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/2gn5waz0i53qyxiiqvlz5x5hbb0285v5-binutils-wrapper-2.30.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/bd7bd91gzd4q354ndmmaygfyfd9mq0kf-binutils-wrapper-2.30.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/2yvxznr3simh127q5g6kspgna95pwnb9-bootstrap-stage2-gcc-wrapper.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/rycm5cr87gxs68c286wghi5ds5krnkfw-bootstrap-stage3-gcc-wrapper.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/06ingf8raac2684mmrpsm2bpvrxbwbpm-bootstrap-stage3-stdenv-linux.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/sj1g4r65ws715ssj88hgqng56vvc08jl-zlib-1.2.11.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/3da3gxyccpksnvgichy0crk9s7k8659z-curl-7.61.1.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/mzp034izmcmwjpa3as6iayz7lixbx8v3-certdata2pem.py.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/xlik0ggbzgpqk32h8db3z360qb1b8xq0-nss-3.39.tar.gz.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/04yr17rzp7hwlpq0fcsqx7fb8lp2rf95-system-path.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/vsx64y5bjixbisi9if0z4zqhng14sihv-nixos-system-nixos-19.03.git.428164a.drv': 1 dependencies couldn't be built
[0 built (1 failed), 300 copied (1086.6 MiB)]
error: build of '/nix/store/vsx64y5bjixbisi9if0z4zqhng14sihv-nixos-system-nixos-19.03.git.428164a.drv' failed
machine: exit status 1
machine: output:
error: command `nixos-install < /dev/null >&2' did not succeed (exit code 1)
command `nixos-install < /dev/null >&2' did not succeed (exit code 1)
cleaning up
killing machine (pid 600)
builder for '/nix/store/7mrrmpaj77xd56z10n4ij2l1bx8sbwwa-vm-test-run-installer-simple.drv' failed with exit code 255
error: build of '/nix/store/7mrrmpaj77xd56z10n4ij2l1bx8sbwwa-vm-test-run-installer-simple.drv' failed

instead of

machine: must succeed: nixos-install < /dev/null >&2
machine# > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > building the configuration in /mnt/etc/nixos/configuration.nix...
[180 built, 302 copied (1042.1 MiB)]

I have no idea what causes this error. Here is the branch if you want to debug it: 428164a

@symphorien
Copy link
Member Author

So the reason seems that the installer uses the installation medium's store as a substituter:
https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/installer/tools/nixos-install.sh#L95-L97
so if we add allowSubstitutes = false on some derivation, then nixos-install will need to build it even if it is present on the live media, which usually requires internet access.
Therefore, allowSubstitutes = false seems to do more harm than good. I think the PR can be merged as it (ie adding only preferLocalBuild = true;).

@symphorien
Copy link
Member Author

Is there something I should add to get the PR merged ?

@rycee
Copy link
Member

rycee commented Dec 26, 2018

It looks good to me 👍

Mic92
Mic92 reviewed Dec 27, 2018
View reviewed changes
nixos/modules/services/continuous-integration/gitlab-runner.nix
@@ -8,6 +8,7 @@ let
if (cfg.configFile == null) then
(pkgs.runCommand "config.toml" {
buildInputs = [ pkgs.remarshal ];
preferLocalBuild = true;
Copy link
Member

Choose a reason for hiding this comment

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

A toTOML would fit well into nixos/lib btw.

Copy link
Member

Choose a reason for hiding this comment

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

However does not need to be solved within the scope of this pr.

@symphorien symphorien force-pushed the local-closureInfo branch 2 times, most recently from 44933f0 to 0a379a5 Compare February 9, 2019 20:48
@symphorien
Copy link
Member Author

I fixed the merge conflict.

@symphorien
Copy link
Member Author

Oh, there is another merge conflict. I expect more to come, so if someone wants to merge this PR, tell me and I will resolve the conflict :)

@rycee
Copy link
Member

rycee commented Feb 21, 2019

@symphorien Please do. I've asked in #nixos-dev whether there are any reservations about merging this and I'll try to follow up on it so that we can get this in.

@symphorien
Copy link
Member Author

I rebased, and ran the installer.simple test succesfully.

@infinisil infinisil merged commit c0318ef into NixOS:master Feb 22, 2019
@infinisil
Copy link
Member

Nice!

@rycee
Copy link
Member

rycee commented Feb 22, 2019

Thanks! 👍

@symphorien symphorien deleted the local-closureInfo branch May 18, 2019 16:01
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

@orivej orivej orivej approved these changes

@LnL7 LnL7 LnL7 approved these changes

@infinisil infinisil infinisil approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@symphorien @matthewbauer @rycee @infinisil @Mic92 @orivej @LnL7 @GrahamcOfBorg