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

Enable parallel building for CMake based projects by default #32271

Merged
merged 24 commits into from Dec 7, 2017
Merged

Enable parallel building for CMake based projects by default #32271

merged 24 commits into from Dec 7, 2017

Conversation

orivej
Copy link
Contributor

@orivej orivej commented Dec 3, 2017 •
edited

Motivation for this change

This obviously benefits small rebuilds. People contributing new packages, updating and fixing bugs in existing packages won't have to wait as much. Reviewers and maintainers of Nixpkgs won't have to ask for enableParallelBuilding, push it into pull requests, or bundle it when they commit something that rebuilds packages with parallel building not enabled.

The downside of parallel building with make — unpredictable failures that are difficult to reproduce — is almost mitigated by build systems that automatically generate complete dependencies between build commands.

During the testing of this PR, I have encountered just 4 packages that failed (details are in the commits of the PR):

  • editorconfig-core-c running doxygen multiple times from the same template with the same output directory has a race on directory creation
  • shogun running multiple ply.yacc that clobber temporary files of each other
  • dino using (and recommending) Ninja and writing their cmake scripts in a way not supported by GNU Make generator of CMake
  • totem relying on a feature missing from Meson

I have reported the first three bugs upstream and disabled parallel building for them. The fourth was already reported both to Totem and Meson, with both sides agreeing that this is a bug in Meson, so I have disable parallel building for Meson. @jtojnar, since this seems to affect just one package, could you consider this issue and decide if we should rather enable parallel Meson and disable parallel Totem?

This PR also adds -GNinja to cmake when the build phase is ninjaBuildPhase, which is an obvious thing to do after #28444 has added the Ninja setup hook.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 pkgs/top-level/release.nix
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@orivej orivej force-pushed the parallel-building branch 2 times, most recently from f72577d to 6e7dfa6 Compare December 4, 2017 01:29
@orivej
Copy link
Contributor Author

orivej commented Dec 4, 2017

I have rebuilt everything again without any new failures. Disabling parallel building for Meson did not help Totem because it had it explicitly enabled; I disabled it for Totem and reenabled for Meson, with some approval from @jtojnar.

@orivej
Copy link
Contributor Author

orivej commented Dec 4, 2017

This is funny: my attempt to disable parallel building of Totem failed because Ninja enables it unless explicitly disabled. All packages using Ninja have always been building in parallel, and those without enableParallelBuilding = true were also not limited by the load average. I am going to fix this here as if Ninja was another build system, with parallel building being enabled by default: this is safe precisely because it has been the default all along.

@orivej orivej force-pushed the parallel-building branch 3 times, most recently from 46b7ed1 to 09c3362 Compare December 4, 2017 15:30
@vcunat
Copy link
Member

vcunat commented Dec 4, 2017

Oh, I had noticed that large rebuilds were sometimes pushing load to a multiple of --cores on my machine, but I never really found the culprit. Hopefully this are most of the cases.

@orivej orivej changed the title Enable parallel building for CMake based projects by default [WIP] Enable parallel building for CMake based projects by default Dec 5, 2017
@grahamc
Copy link
Member

grahamc commented Dec 5, 2017

@GrahamcOfBorg eval

@orivej orivej force-pushed the parallel-building branch 4 times, most recently from 94e8f5d to 9d07d86 Compare December 6, 2017 11:33
@orivej
shogun: fix parallel building
812e0bc
@orivej
stp: disable parallel building
9f8ec7e
@orivej
ninja: support disabling parallel building
2c7cd24 
but keep it enabled by default
@orivej
totem: disable parallel building
be17bcc 
meson does not support parallel building of certain vala projects:
mesonbuild/meson#1994
https://bugzilla.gnome.org/show_bug.cgi?id=784236

but at the moment this only affects gnome3.totem:

ninja src/plugins/rotation/rotation.vapi
[1/1] Compiling Vala source ../src/plugins/rotation/bacon-video.vapi ../src/plugins/rotation/totem-rotation-plugin.vala.
FAILED: src/plugins/rotation/rotation@sha/totem-rotation-plugin.c src/plugins/rotation/rotation.h src/plugins/rotation/rotation.vapi
valac -C --pkg clutter-gtk-1.0 --pkg cogl-pango-1.0 --pkg libpeas-1.0 --pkg gtk+-3.0 --color=always --directory src/plugins/rotation/rotation@sha --basedir ../src/plugins/rotation --library rotation --header src/plugins/rotation/rotation.h --vapi ../rotation.vapi --girdir=/tmp/nds-build-gnome3.totem/totem-3.26.0/build/src --pkg=Totem-1.0 ../src/plugins/rotation/bacon-video.vapi ../src/plugins/rotation/totem-rotation-plugin.vala
error: Package `Totem-1.0' not found in specified Vala API directories or GObject-Introspection GIR directories
@orivej
cmake: generate for Ninja when the build phase is Ninja
da516f0 
This is overridable by providing a custom build phase or setting
dontUseNinjaBuild = true.
@orivej
dino: build with Ninja
75d70c8 
Upstream recommends Ninja and has a cmake script that does not support GNU Make:
dino/dino#230
@orivej
cmake: enable parallel building of cmake projects by default
031138b
@orivej
meson: enable parallel building of meson projects by default
46f57db
@orivej
qmake4Hook: enable parallel building with qmake4Hook by default
ca4f822
@orivej
qmake: enable parallel building of qmake projects by default
f568450
@orivej
doc: document enableParallelBuilding being unset
683047f
@orivej
gst_all_1.gst-plugins-bad: enable parallel building
199b426
@orivej orivej changed the title [WIP] Enable parallel building for CMake based projects by default Enable parallel building for CMake based projects by default Dec 7, 2017
@orivej
Copy link
Contributor Author

orivej commented Dec 7, 2017

This is ready, but I will also describe the kinds of bugs in individual packages that I have encountered while working on this PR.

@orivej orivej merged commit 06ee1e3 into NixOS:staging Dec 7, 2017
@jtojnar jtojnar mentioned this pull request Dec 13, 2017
Merged
8 tasks
@orivej
Copy link
Contributor Author

orivej commented Dec 14, 2017

Oh, I had noticed that large rebuilds were sometimes pushing load to a multiple of --cores on my machine, but I never really found the culprit. Hopefully this are most of the cases.

I have identified two more cases. First, qtwebengine (both 5.6 and 5.9) uses Make to generate Ninja build files and invoke Ninja (without limiting load average). Second, ghc compilation is multithreaded (./Setup build effectively launches ghc ... -j$(nproc)), and in practice this easily leads to the number of running threads quadratic in the number of Nix max jobs. (Hence the performance of each Linux thread really becomes inversely proportional to the number of CPU threads.)

@orivej orivej mentioned this pull request Dec 15, 2017
Merged
8 tasks
@orivej orivej deleted the parallel-building branch December 15, 2017 17:05
@orivej
Copy link
Contributor Author

orivej commented Dec 19, 2017 •
edited

The issue of unfair distribution of CPU resources in favor of builders that do not respect load average can be solved by putting the processes of each build user into a separate cgroup. This may be possible with some existing cgroup manager, without changing nix-daemon. It should prevent timeouts in Go and QEMU tests.

@orivej orivej mentioned this pull request Dec 20, 2017
Closed
8 tasks
@jtojnar jtojnar mentioned this pull request Jan 1, 2018
Closed
13 tasks
@jtojnar jtojnar mentioned this pull request Feb 3, 2018
Merged
8 tasks
@xeji xeji mentioned this pull request Apr 5, 2018
Merged
8 tasks
@jtojnar jtojnar mentioned this pull request Apr 5, 2018
Closed
8 tasks
@nh2 nh2 mentioned this pull request Jun 25, 2020
Closed
@drewrisinger drewrisinger mentioned this pull request Sep 10, 2020
Merged
10 tasks
@orivej orivej mentioned this pull request Oct 20, 2021
Merged
12 tasks
@LunNova LunNova mentioned this pull request Oct 30, 2022
Draft
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7c6f434c 7c6f434c 7c6f434c approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

@vcunat vcunat Awaiting requested review from vcunat

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

Successfully merging this pull request may close these issues.

None yet
6 participants
@orivej @vcunat @grahamc @7c6f434c @nixos-discourse @GrahamcOfBorg