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

nixos: Introduce nix.buildLocation option #83166

Merged
merged 1 commit into from Apr 18, 2020
Merged

Conversation

avnik
Copy link
Contributor

@avnik avnik commented Mar 22, 2020

Allow to specify where package build will happens.
It helps big packages (like browsers) not to overflow tmpfs.

Motivation for this change

/tmp overflow during build big packages (like chromium and/or firefox) is very annoying, so I added option to run builds on dedicated filesystem/directory.

/cc @cleverca22 for review, we discuss possibility of this change earlier.

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.

@avnik
Copy link
Contributor Author

avnik commented Mar 23, 2020

Well, I prefer (ideally) to force nix-daemon to choose /tmp, /var/ssd, or /var/spinning-disk depending of size of sources, and predicted overall usage.

I definitely won't to degrade performance of everything, pushing /tmp to spinning disks. I actually had this solutuion, now generalized to nixos option, which I used couple years. We discussed it in IRC yesterday, and I proposed to make this chanhe (it why I cast @cleverca22 here).

But you are right -- is an opinionated change, and point to discuss,

@avnik
Copy link
Contributor Author

avnik commented Mar 25, 2020

@volth I see, but anyway it not solve memory vs swap usage here. Ideally would be nice to have ability to "annotate" derivation by some labels, (which should not affect purity/cache), but suggest nix to pick better builder (or builder options -- like spool dir/cpu cores/memory/time limits, etc)

@avnik
Copy link
Contributor Author

avnik commented Mar 25, 2020

@Mic92 applied both suggestions

@Mic92
Copy link
Member

Mic92 commented Mar 26, 2020

@GrahamcOfBorg test installer.simple

1 similar comment
@Mic92
Copy link
Member

Mic92 commented Mar 26, 2020

@GrahamcOfBorg test installer.simple

@Mic92
Copy link
Member

Mic92 commented Apr 6, 2020

This has a merge conflict now unfortunately.

Allow to specify where package build will happens.
It helps big packages (like browsers) not to overflow tmpfs.
@Mic92 Mic92 merged commit 35eb779 into NixOS:master Apr 18, 2020
@@ -490,6 +502,8 @@ in
restartTriggers = [ nixConf ];
};

systemd.tmpfiles.rules = [ "d ${cfg.buildLocation} 0775 root root -" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks write permissions for non-root on /tmp with the default value and boot.tmpOnTmpfs = true;

setting up tmpfiles
[/etc/tmpfiles.d/tmp.conf:11] Duplicate line for path "/tmp", ignoring.
$ getfacl /tmp
getfacl: Removing leading '/' from absolute path names
# file: tmp/
# owner: root
# group: root
user::rwx
group::rwx
other::r-x

@primeos
Copy link
Member

primeos commented Apr 19, 2020

As @gravndal already noted the tmpfiles change is problematic and could cause severe regressions (I didn't update yet, but I'd imaging many applications might crash and/or missbehave due to this).

See also #85552.
I think this should be fixed (I would recommend to drop systemd-tmpfiles and only create the directory in preStart if it doesn't already exist) or reverted ASAP to reduce the impact.

@calbrecht
Copy link
Member

Yes :) it is far worse. This affects every reboot. Sway segfaults for me when it cannot write to /tmp.

primeos added a commit that referenced this pull request Apr 19, 2020
This reverts commit 5291925.
Reason: This started to cause severe regressions, see:
- #85552
- #83166 (review)
Fixes #85552.
@primeos
Copy link
Member

primeos commented Apr 19, 2020

I reverted this change in 0e4417f for now so we can properly resolve this without any pressure.

@Mic92
Copy link
Member

Mic92 commented Apr 19, 2020

@primeos thanks!

@avnik
Copy link
Contributor Author

avnik commented Apr 20, 2020

@Mic92 @primeos should I re-do other PR with change to preStart?

@Mic92
Copy link
Member

Mic92 commented Apr 20, 2020

yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants