meson.setupHook: prefer meson commands over ninja #213845
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eli-schwartz
suggested changes
Jan 31, 2023
There was a problem hiding this comment.
The reason to prefer using the Meson commands is that they take additional options (e.g. setting custom timeouts for tests — my motivation for this change).
- The functionality that is implemented in python (
meson foobarsubcommands) come with convenience targets in ninja, that run the python tools. - The functionality that is implemented in ninja comes with a convenience target in python, that simply runs ninja.
Practically speaking, this means that if you want the default functionality, you can more or less use either entry point interchangeably.
The difference between the two is that building is purely ninja, and everything else is implemented in python. So it makes sense to run meson test and meson install directly if you want to pass additional options, but there's no advantage to running meson compile <args> since it will do nothing other than subprocess out to ninja <args>.
... Oh wait, actually that's not quite true. On Windows, meson compile will first run vcvarsall.bat to set up an MSVC dev cmd environment, if you're using the MSVC compiler and didn't start off in one. This functionality has no reason to exist except that ninja's feature request to not be broken by default, but rather to include functionality for loading an environment block at startup, is in limbo. You don't need this if you use a sane operating system, or if you use a mingw-based compiler even on Windows.
|
@eli-schwartz thanks for the clarification/explanation! I appreciate it. |
|
v2:
I'll do another test build through qemu_kvm, but once review has settled down because it takes a long time. |
|
v3:
- Don't try to pass -j$NIX_BUILD_CORES to meson test, as it does not support it.
- Pass --no-rebuild to meson test and meson install, to avoid rerunning Ninja, which will never have any work to do.
|
|
Tested building through to |
jtojnar
reviewed
Feb 5, 2023
Comment on lines
+31
to
+33
| if [[ ${checkPhase-ninjaCheckPhase} = ninjaCheckPhase && -z $dontUseMesonCheck ]]; then | ||
| checkPhase=mesonCheckPhase | ||
| fi |
There was a problem hiding this comment.
Having this in configurePhase will only work when it is not overridden. But I probably would not bother with this too much until someone comes up with an use case.
There was a problem hiding this comment.
Yeah I think that's fine.
There was a problem hiding this comment.
I found only one usage of an overridden configurePhase with meson.
#259835
github-actions
bot
added
the
8.has: documentation
jtojnar
reviewed
Feb 15, 2023
Meson now comes with its own set of commands for building, testing, installing etc., that by default wrap around Ninja. The reason to prefer using the Meson commands is that they take additional options (e.g. setting custom timeouts for tests — my motivation for this change). Here, I've modified the Meson setup hook so that Meson's test and install commands will be used instead of Ninja's when Meson's configurePhase is used. This restriction is to avoid starting to run Meson directly when dealing with custom build systems that wrap around Meson, like QEMU's. We don't use meson's compile command, as it just runs ninja, and that's handled fine by the existing Ninja setup hook. Naturally the Meson commands don't support entirely the same set of options that the Ninja ones did, but I checked through Nixpkgs to find any packages using Meson that used any options that wouldn't be picked up by this new system. I only found one, and it was just setting checkTarget = "test", which is the default value for Ninja and has no Meson equivalent (because we directly tell Meson to run the tests rather than going through a generic job system like Ninja). Link: NixOS#113829 Co-authored-by: Jan Tojnar <[email protected]>
|
@ofborg eval |
|
OfBorg failure is just it failing a test in aws-sdk-cpp. |
xokdvium
added a commit
to xokdvium/nixpkgs
that referenced
this pull request
Feb 4, 2025
Motivation for this hook is simple: there's no single documented way to do trivial things with ctest: 1. Pass additional flags to ctest invocation. 2. Selectively disable tests in a mechanism similar to python's `disabledTests` or rust's composable skips in `checkFlags`. 3. Disable parallel checking. Current state of things has lead to several different solutions: 1. Completely overriding `checkPhase` [1] and invoking ctest manually with the necessary flags. This is most often coupled with `-E` for disabling test or setting parallel level. 2. Wrangling with weird double string/regex escaping and trying to stuff additional parameters and/or exclusion regex via `CMAKE_CTEST_ARGUMENTS`. This approach is especially painful when test names have spaces. This is the reason I originally decided to implement this hook after wrangling with failing darwin tests here [2]. 3. Stuffing additional arguments into `checkFlagsArray` with the `ARGS` makefile parameter [3]. I don't see any reason to keep the status-quo. Doing something along these lines has been suggested [4] for both `ctest` and `meson`. Meson setup-hook has switched from `ninja` to `meson` in [5] with little friction. Doing the same for cmake in a single sweep would prove problematic due to the aforementioned zoo of workarounds and hacks for `ctest`. Doing it via a separate hook would allow us to refactor things piecemeal and without going through staging. The benefit of the hook is immediately clear and it would allow to drive the refactor tractor at a comfortable pace. [1]: pd/pdal/package.nix:117, cc/ccache/package.nix:108, gl/glog/package.nix:79 [2]: NixOS#375955 [3]: op/open62541/package.nix:114 [4]: NixOS#113829 [5]: NixOS#213845
13 tasks
xokdvium
added a commit
to xokdvium/nixpkgs
that referenced
this pull request
Feb 6, 2025
Motivation for this hook is simple: there's no single documented way to do trivial things with ctest: 1. Pass additional flags to ctest invocation. 2. Selectively disable tests in a mechanism similar to python's `disabledTests` or rust's composable skips in `checkFlags`. 3. Disable parallel checking. Current state of things has lead to several different solutions: 1. Completely overriding `checkPhase` [1] and invoking ctest manually with the necessary flags. This is most often coupled with `-E` for disabling test or setting parallel level. 2. Wrangling with weird double string/regex escaping and trying to stuff additional parameters and/or exclusion regex via `CMAKE_CTEST_ARGUMENTS`. This approach is especially painful when test names have spaces. This is the reason I originally decided to implement this hook after wrangling with failing darwin tests here [2]. 3. Stuffing additional arguments into `checkFlagsArray` with the `ARGS` makefile parameter [3]. I don't see any reason to keep the status-quo. Doing something along these lines has been suggested [4] for both `ctest` and `meson`. Meson setup-hook has switched from `ninja` to `meson` in [5] with little friction. Doing the same for cmake in a single sweep would prove problematic due to the aforementioned zoo of workarounds and hacks for `ctest`. Doing it via a separate hook would allow us to refactor things piecemeal and without going through staging. The benefit of the hook is immediately clear and it would allow to drive the refactor tractor at a comfortable pace. [1]: pd/pdal/package.nix:117, cc/ccache/package.nix:108, gl/glog/package.nix:79 [2]: NixOS#375955 [3]: op/open62541/package.nix:114 [4]: NixOS#113829 [5]: NixOS#213845
xokdvium
added a commit
to xokdvium/nixpkgs
that referenced
this pull request
Feb 14, 2025
Motivation for this hook is simple: there's no single documented way to do trivial things with ctest: 1. Pass additional flags to ctest invocation. 2. Selectively disable tests in a mechanism similar to python's `disabledTests` or rust's composable skips in `checkFlags`. 3. Disable parallel checking. Current state of things has lead to several different solutions: 1. Completely overriding `checkPhase` [1] and invoking ctest manually with the necessary flags. This is most often coupled with `-E` for disabling test or setting parallel level. 2. Wrangling with weird double string/regex escaping and trying to stuff additional parameters and/or exclusion regex via `CMAKE_CTEST_ARGUMENTS`. This approach is especially painful when test names have spaces. This is the reason I originally decided to implement this hook after wrangling with failing darwin tests here [2]. 3. Stuffing additional arguments into `checkFlagsArray` with the `ARGS` makefile parameter [3]. I don't see any reason to keep the status-quo. Doing something along these lines has been suggested [4] for both `ctest` and `meson`. Meson setup-hook has switched from `ninja` to `meson` in [5] with little friction. Doing the same for cmake in a single sweep would prove problematic due to the aforementioned zoo of workarounds and hacks for `ctest`. Doing it via a separate hook would allow us to refactor things piecemeal and without going through staging. The benefit of the hook is immediately clear and it would allow to drive the refactor tractor at a comfortable pace. [1]: pd/pdal/package.nix:117, cc/ccache/package.nix:108, gl/glog/package.nix:79 [2]: NixOS#375955 [3]: op/open62541/package.nix:114 [4]: NixOS#113829 [5]: NixOS#213845
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Meson now comes with its own set of commands for building, testing, installing etc., that by default wrap around Ninja. The reason to prefer using the Meson commands is that they take additional options (e.g. setting custom timeouts for tests — my motivation for this change).
Here, I've modified the Meson setup hook so instead of Ninja being run automatically, Meson's equivalent commands will be if Meson is present. This only happens when Meson is used to configure, to avoid starting to run Meson directly when dealing with custom build systems that wrap around Meson, like QEMU's.
Naturally the Meson commands don't support entirely the same set of options that the Ninja ones did, but I checked through Nixpkgs to find any packages using Meson that used any options that wouldn't be picked up by this new system. I only found one, and it was just setting
checkTarget = "test", which is the default value for Ninja and has no Meson equivalent (because we directly tell Meson to run the tests rather than going through a generic job system like Ninja).I've tested building all the way to qemu_kvm, which I chose because it has lots of dependencies built with Meson, and also has its own custom Meson-wrapping build system so I could be sure that also still worked.
Fixes, partially: #113829
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes