-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
treewide: don’t set ulimit -n
in builds
#351783
base: staging
Are you sure you want to change the base?
Conversation
It turns out that this was actually caused by a Hydra misconfiguration that affected Linux too (albeit to a lesser degree). We’re in the process of testing a fix and if it all goes well then we can probably do the tree‐wide portion of this without the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good sans the change to setup.sh
pkgs/stdenv/generic/setup.sh
Outdated
# Increase the soft file descriptor limit if it’s too low. This is | ||
# mostly for the benefit of Darwin, where the default is a positively | ||
# anaemic 256 FDs by default. We use 10240 because it’s the default | ||
# value of `kern.maxfilesperproc` on macOS 10.12, but this can be | ||
# increased when the minimum macOS version is bumped. | ||
if [[ "$(ulimit -n)" < 10240 ]]; then | ||
ulimit -S -n 10240 || true | ||
fi | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Increase the soft file descriptor limit if it’s too low. This is | |
# mostly for the benefit of Darwin, where the default is a positively | |
# anaemic 256 FDs by default. We use 10240 because it’s the default | |
# value of `kern.maxfilesperproc` on macOS 10.12, but this can be | |
# increased when the minimum macOS version is bumped. | |
if [[ "$(ulimit -n)" < 10240 ]]; then | |
ulimit -S -n 10240 || true | |
fi |
This is now handled in the daemon’s environment.
721d69c
to
234d586
Compare
ulimit -n
if it’s too lowulimit -n
in builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple as.
I’ve dropped the |
This is handled in the daemon’s environment.
Fixed the Darwin |
234d586
to
04fe560
Compare
missed that in reviewing it too. diffs of diffs are the worst. and it also seems like the formatter switches rules on where to put the edit: tho |
I also missed the |
The patch needs fixing. |
This avoids having to work around the incredibly low Darwin default in a bunch of builds. The Nix daemon is meant to get a higher limit from launchd, but it seems to not work or not be passed down to builds for whatever reason. For now, this is a simple and expedient fix for a recurring problem.This was a problem on the NixOS infra that was fixed so now these are redundant even without the
stdenv
patch.See:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.