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
go: build each package single-threaded #53390
Conversation
@@ -130,7 +130,7 @@ let | |||
echo "$d" | grep -q "\(/_\|examples\|Godeps\)" && return 0 | |||
[ -n "$excludedPackages" ] && echo "$d" | grep -q "$excludedPackages" && return 0 | |||
local OUT | |||
if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v $d 2>&1)"; then | |||
if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v -p 1 $d 2>&1)"; then |
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.
mhm. Would it be better to just remove the parallel xargs?
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.
@Mic92 - I pushed a commit that removes the parallel xargs
in favor of passing $NIX_BUILD_CORES
through to the compiler. Thoughts? If the new approach looks good, I can squash the commits together.
I can do that too! I would prefer to get the opinion of someone who knows
why we did it this way in the first place, but also happy to just remove
the `xargs` in favor of a `-p $NIX_BUILD_CORES` flag
…On Fri, Jan 4, 2019 at 2:53 AM Jörg Thalheim ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkgs/development/go-modules/generic/default.nix
<#53390 (comment)>:
> @@ -130,7 +130,7 @@ let
echo "$d" | grep -q "\(/_\|examples\|Godeps\)" && return 0
[ -n "$excludedPackages" ] && echo "$d" | grep -q "$excludedPackages" && return 0
local OUT
- if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v $d 2>&1)"; then
+ if ! OUT="$(go $cmd $buildFlags "''${buildFlagsArray[@]}" -v -p 1 $d 2>&1)"; then
mhm. Would it be better to just remove the parallel xargs?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#53390 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABB3hYVt9WZd-DCAsU9ZOTFk7KpEAR1uks5u_zLBgaJpZM4ZpT7d>
.
|
cc @wkennington, who originally wrote this code (assuming my It looks like this was written prior to Go having support for parallel builds, so I think we should be fine to remove all the |
@GrahamcOfBorg build vgo2nix |
@Mic92 - It looks like the build failed due to running out of space on the builder (EDIT: on
Any suggestions? |
I just re-test. I don't think it is the fault of your change. |
maybe. I just test it myself locally. |
@@ -167,7 +167,7 @@ let | |||
if [ -z "$enableParallelBuilding" ]; then | |||
export NIX_BUILD_CORES=1 | |||
fi | |||
getGoDirs "" | xargs -n1 -P $NIX_BUILD_CORES bash -c 'buildGoDir install "$@"' -- |
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.
I think we still need -n1. However it would might be easier to just loop over getGoDirs
in a bash for-loop since we no longer need parallel execution.
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.
@Mic92 - I decided to keep the xargs
instead of looping to avoid having to re-run the go
compiler; if we're building a package with lots of Go modules, it avoids the startup time. I also want to keep the xargs
command to ensure that we don't go past the maximum length of a command line for large packages.
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.
What startup time do you avoid? xargs needs to start and its bash too. A for loop stays within the same process and does not have the argument length limitation:
for pkg in getGoDirs ""; do
buildGoDir install "$pkg"
done
6b9cbc7
to
d8b5218
Compare
d8b5218
to
a088f20
Compare
@Mic92 - Sorry for the delay here; life got busy 🙂 Just pushed a change that uses a |
Co-Authored-By: andrew-d <andrew@du.nham.ca>
@Mic92 - Done! Also, GitHub's "suggestions" feature is pretty cool 👍 |
Motivation for this change
I noticed that I was seeing the Go compiler build things in parallel even when I'd set
-j1 --cores 1
. It appears that the compiler, by default, uses the number of CPUs that are available to perform a build, while nixpkgs parallelizes at the directory level.In order to change the fewest assumptions, this explicitly tells the Go compiler to run single-threaded. The flag's documentation is:
So this should function as expected. Feedback appreciated!
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)