Add check phase to ninja setup hook #50527
Conversation
GrahamcOfBorg
added
10.rebuild-darwin-stdenv
|
Success on x86_64-linux (full log) Attempted: glib, ninja Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: glib, ninja Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: glib, ninja Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: glib, ninja Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: glib, ninja Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: glib, ninja Partial log (click to expand)
|
| $ninjaFlags "${ninjaFlagsArray[@]}" \ | ||
| $buildFlags "${buildFlagsArray[@]}") | ||
| local flagsArray=( | ||
| -j$buildCores -l$buildCores |
There was a problem hiding this comment.
I don't think there is a practical difference between setting -l to $buildCores and $NIX_BUILD_CORES, but conceptually -l specifies a system-wide limit and should be the same for all builds, whereas different builds can have different build-local limits set with -j.
There was a problem hiding this comment.
I am hoping it has the same behavior as in make? The only difference is it is autodetected when left out in ninja.
There was a problem hiding this comment.
There is no change in behaviour, but there is also no need to change -l$NIX_BUILD_CORES to -l$buildCores. I don't want to see -l1 in the command line because -l should be equal to the total number of busy cores across all concurrent builds; it's value should not be affected by the level of parallelism of a particular build.
| $ninjaFlags "${ninjaFlagsArray[@]}") | ||
| local flagsArray=( | ||
| $ninjaFlags "${ninjaFlagsArray[@]}" | ||
| $installFlags "${installFlagsArray[@]}" |
There was a problem hiding this comment.
Ninja does not have any flags that would be useful with the install target. I'd revert the change to glib/setup-hook.sh and remove this line, as it may break other setup hooks too.
There was a problem hiding this comment.
You may still have flags that you only want to go to ninja install command, though, right? This seems like the most consistent thing, otherwise there is a question of why buildFlags work with ninja but not installFlags.
There was a problem hiding this comment.
It looks like no package is using buildFlags with ninja (and the only package using ninjaFlags is otfcc); it seems counterproductive to support installFlags. I'd rather remove buildFlags from ninja flags array, since it is not needed and it is documented as a make flags variable.
There was a problem hiding this comment.
My opinion here is that we should reuse as much of the generic builder as possible. There are definitely some uses for this (for instance with checkFlags below). I can change the doc to reflect that {build|install|check}Flags passes flags to whatever build system you are using - not just make. As meson & cmake replace autotools, I think it is important we make them just as powerful.
There was a problem hiding this comment.
checkFlags are not applicable to Ninja: checkTarget(s) covers all use cases that it offers.
| } | ||
|
|
||
| if [ -z "$dontUseNinjaCheck" -a -z "$checkPhase" ]; then | ||
| checkPhase=ninjaCheckPhase |
There was a problem hiding this comment.
How does this interact with meson setup hook? First come in nativeBuildInputs first served?
There was a problem hiding this comment.
I think we'd have to remove the meson check phase to avoid confusion. It would probably be whichever one is set first.
There was a problem hiding this comment.
Since meson depends on ninja, meson hook will run first and set the checkPhase.
There was a problem hiding this comment.
Oops, @Ericson2314 was working on it, but currently this is not the case, and the order in nativeBuildInputs is decisive.
|
One thing |
|
@jtojnar That is definitely a disadvantage of doing |
|
Ninja does not have flags, it has only targets. |
GrahamcOfBorg
added
the
6.topic: GNOME
|
Failure on aarch64-linux (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
|
Failure on aarch64-linux (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
|
I have looked up how |
Things changed in the Ninja setup-hook: - Respect installFlags - Automatically add checkPhase (can be disabled with dontUseNinjaCheck in the same way as dontUseNinjaBuild and dontUseNinjaInstall). Tests are only run when "ninja test" exists. - Error in build phase when build.ninja is missing. We don’t have a way to fall back to other build methods, so it’s best to be very clear when we aren’t able to build with ninja - Set -l flag to 1 when enableParallelBuilding is disabled
This is now handled in the ninja setup hook. (cherry picked from commit f5e0997)
This is now handled in ninja.
These are already in the generated makefiles!
These don’t have much use in ninja, so easiest to leave them out.
This can be set unconditionally to $NIX_BUILD_CORES according to @orivej.
matthewbauer
force-pushed
the
ninja-check-phase
branch
from
de79e8c to
b75d5ad
Compare
|
Failure on aarch64-linux (full log) Attempted: meson, ninja Partial log (click to expand)
|
|
Failure on x86_64-linux (full log) Attempted: meson, ninja Partial log (click to expand)
|
| fi | ||
| echoCmd 'build flags' "${flagsArray[@]}" | ||
| ninja "${flagsArray[@]}" | ||
| unset flagsArray |
There was a problem hiding this comment.
I have just noticed: since flagsArray is local, we don't have to unset it.
There was a problem hiding this comment.
I noticed that too but thought it might be to avoid it leaking into the post hook?
There was a problem hiding this comment.
runHook is a function, it has a new scope without local variables of the current function.
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
| export LD_LIBRARY_PATH=$(readlink -f ./src) | ||
| CTEST_OUTPUT_ON_FAILURE=1 make test | ||
| export CTEST_OUTPUT_ON_FAILURE=1 |
There was a problem hiding this comment.
CTEST_OUTPUT_ON_FAILURE ought to be the default (at least on Hydra), since without it ctest prints that the test has failed without any details. Shall we initialize it in the cmake setup hook (if it was unset)?
There was a problem hiding this comment.
Yes. Also maybe CTEST_PARALLEL_LEVEL?
There was a problem hiding this comment.
Great idea! (The derivation may explicitly set it back to 1 if the tests don't support concurrency.)
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: glib, meson, ninja Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: meson, ninja Partial log (click to expand)
|
|
As a side note, it would be nice to have support for wrapping tests in the setup hooks so that we do not need to do nixpkgs/pkgs/development/libraries/libdazzle/default.nix Lines 28 to 33 in 0925c48 |
Yes! This would be nice to abstract out. It would also be useful with my "exeWrapper" work: So, you could run tests after cross compiling. It gets a little bit complicated because you may not have all of the right tools cross built in this case, but it's a similar principle to dbus-run-session. |
We should always use this.
|
Success on aarch64-linux (full log) Attempted: cmake, meson, ninja Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: cmake, meson, ninja Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: cmake, meson, ninja Partial log (click to expand)
|
|
@GrahamcOfBorg build cmark at-spi2-atk |
|
Timed out, unknown build status on x86_64-darwin (full log) Attempted: cmark, at-spi2-atk Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: cmark, at-spi2-atk Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: cmark, at-spi2-atk Partial log (click to expand)
|
|
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/using-xvfb-in-checkphase/1497/3 |
Motivation for this change
Most tests in Ninja packages were not getting run because the default check phase assumes you have a Makefile. This adds a check phase to Ninja's setup-hook to fix this. Here is what I changed:
in the same way as dontUseNinjaBuild and dontUseNinjaInstall). Tests
are only run when "ninja test" exists.
way to fall back to other build methods, so it’s best to be very
clear when we aren’t able to build with ninja
Intended as an alternative to #50323.