restinio: fix build #375955
Merged
restinio: fix build #375955
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
|
Could you open an issue for the |
|
On second thought, this isn't a catch2 problem at all, but rather a restinio tests bug. Since |
xokdvium
force-pushed
the
fix/restinio
branch
from
a4f5250 to
53775e5
Compare
GaetanLepage
reviewed
Feb 2, 2025
pkgs/by-name/re/restinio/package.nix
Outdated
Comment on lines
37
to
38
| substituteInPlace dev/test/CMakeLists.txt --replace-fail \ | ||
| "add_subdirectory(metaprogramming)" "" |
There was a problem hiding this comment.
Suggested change
| substituteInPlace dev/test/CMakeLists.txt --replace-fail \ | |
| "add_subdirectory(metaprogramming)" "" | |
| substituteInPlace dev/test/CMakeLists.txt \ | |
| --replace-fail "add_subdirectory(metaprogramming)" "" |
nit
There was a problem hiding this comment.
Thanks! Applied the suggestion. For future reference: is this a formatting convention I'm unaware of?
I guess it's useful to make diffs cleaner and reduce merge conflicts in case more substitutions are added.
There was a problem hiding this comment.
I guess it's useful to make diffs cleaner and reduce merge conflicts in case more substitutions are added.
Yes, that's the motive for this one.
xokdvium
force-pushed
the
fix/restinio
branch
from
53775e5 to
18e276d
Compare
I feel like catch2 not allowing this any more is still a regression. That said, restinio should probably reorganize their test directory structure to make it clear that not all tests are implemented with the catch2 framework. |
|
|
Maybe try adding |
Test builds fail with cmake configuration error: > string sub-command JSON failed parsing json string: * Line 1, Column 1 This is a silly regression from catch2_3 3.7.1 -> 3.8.0 bump [1]. Looks like the upstream doesn't hold catch2 correctly and overrides `main` for this test so catch2 test discovery machinery can't work. Upstream issue [2]. [1]: https://www.github.com/NixOS/nixpkgs/pull/371311 [2]: https://www.github.com/Stiffstream/restinio/issues/230
xokdvium
force-pushed
the
fix/restinio
branch
from
18e276d to
97ae828
Compare
|
Yup, seems like I forgot my own suggestion #368300 (review). |
|
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.
Test builds fail with cmake configuration error:
This is a silly regression from catch2_3 3.7.1 -> 3.8.0 bump 1.
Looks like the upstream doesn't hold catch2 correctly and overrides
mainfor this test so catch2 test discovery machinery can't work.
Upstream issue 2.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.