-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
ctestCheckHook: init, {pdal,gifticlib,zynaddsubfx}: migrate to ctestCheckHook #379426
base: master
Are you sure you want to change the base?
Conversation
cc8da0e
to
a2a5406
Compare
a2a5406
to
12bcae0
Compare
Rebased. Hopefully rerunning the nixpkgs manual build job will fix the intermittent cachix 501 error. Pinging CMake maintainers here for visibility and their reviews, since Also @wolfgangwalther, since they recently worked on |
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 only looked at the hook. structuredAttrs
-wise I don't see any problems. 👍
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5224 |
12bcae0
to
a1358e7
Compare
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]: https://www.github.com/NixOS/nixpkgs/pull/375955 [3]: op/open62541/package.nix:114 [4]: https://www.github.com/NixOS/nixpkgs/issues/113829 [5]: https://www.github.com/NixOS/nixpkgs/pull/213845
a1358e7
to
494f737
Compare
Motivation for this hook is simple: there's no single documented
way to do trivial things with ctest:
disabledTests
or rust's composable skips incheckFlags
.Current state of things has lead to several different solutions:
checkPhase
[1] and invoking ctest manuallywith the necessary flags. This is most often coupled with
-E
fordisabling test or setting parallel level.
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].
checkFlagsArray
with theARGS
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
andmeson
. Meson setup-hookhas switched from
ninja
tomeson
in [5] with little friction. Doingthe same for cmake in a single sweep would prove problematic due to the
aforementioned zoo of workarounds and hacks for
ctest
. Doing it viaa 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]: #375955
[3]: op/open62541/package.nix:114
[4]: #113829
[5]: #213845
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.