| Message ID | patch-7.9-fc9f036695f-20221021T091013Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cmake: fix *nix & general issues, no test-lib.sh editing, ctest in CI | expand |
Hi Ævar On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote: > Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments > to tests. This allows for passing arguments to tests with e.g.: > > GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 --verbose > > There's some overlap with this and what was suggested in [1], but as > noted there we're not passing "--verbose" and friends unconditionally, > so a plain "ctest" invocation without a "cmake" re-build won't pick up > the options. The aim of dscho's patch was to make debugging information available in the test logs without the user having to do anything, now to get that information every user has to set GIT_TEST_OPTS="--no-bin-wrappers --no-chain-lint -vx" when running cmake. I think it would be helpful to have some default options set if the user does not pass GIT_TEST_OPTS. Ideally one would be able to do GIT_TEST_OPTS=... ctest and have the tests pick up the options at runtime. Following on from my previous comment, if we used "sh -c" to launch the tests we could have something like COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2" ${GIT_TEST_OPTS:---no-bin-wrappers --no-chain-lint -vx}]] "${tsh}" "${CMAKE_BINARY_DIR}" "${tsh}" Best Wishes Phillip > 1. https://lore.kernel.org/git/356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com/ > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > contrib/buildsystems/CMakeLists.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt > index 91b7009f4fd..8e29e3f514b 100644 > --- a/contrib/buildsystems/CMakeLists.txt > +++ b/contrib/buildsystems/CMakeLists.txt > @@ -1083,9 +1083,11 @@ endif() > file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") > > #test > +set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}") > +separate_arguments(GIT_TEST_OPTS) > foreach(tsh ${test_scipts}) > add_test(NAME ${tsh} > - COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} > + COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} ${GIT_TEST_OPTS} > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) > endforeach() >
On Fri, Oct 21 2022, Phillip Wood wrote: > Hi Ævar > > On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote: >> Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments >> to tests. This allows for passing arguments to tests with e.g.: >> GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 >> --verbose >> There's some overlap with this and what was suggested in [1], but as >> noted there we're not passing "--verbose" and friends unconditionally, >> so a plain "ctest" invocation without a "cmake" re-build won't pick up >> the options. > > The aim of dscho's patch was to make debugging information available > in the test logs without the user having to do anything, now to get > that information every user has to set > GIT_TEST_OPTS="--no-bin-wrappers --no-chain-lint -vx" when running > cmake. > > I think it would be helpful to have some default options set if the > user does not pass GIT_TEST_OPTS. Ideally one would be able to do > > GIT_TEST_OPTS=... ctest > > and have the tests pick up the options at runtime. Following on from > my previous comment, if we used "sh -c" to launch the tests we could > have something like > > COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2" > ${GIT_TEST_OPTS:---no-bin-wrappers --no-chain-lint -vx}]] > "${tsh}" "${CMAKE_BINARY_DIR}" "${tsh}" That sounds reasonable to me. FWIW I looked into $CTEST_INTERACTIVE_DEBUG_MODE for this purpose, i.e. to stick something like this in test-lib.sh: if test -n "$CTEST_INTERACTIVE_DEBUG_MODE" then verbose=t trace=t fi But I was hoping for some way to tell that "ctest" was in "--verbose" mode, but AFAICT there's no way to get at that without something like compat/linux/procinfo.c (basically a fancier way of parsing "ps auxf"). Anyway, as noted in my review of dscho's series I thought this part of it was odd/outdated given that this thing runs on Linux (mostly, but entirely after this series). I.e. why would we hardcode Windows-specific trade-offs into a portable build-system, and if you do want e.g. "--no-bin-wrappers" why would you want that just when you run "cmake", not "make"? Surely if we're pushing for a new default it should be agnostic to the user's build system. But in any case I think if we're pushing for new (or cmake-specific) opinionated defaults it makes sense to split those up & justify them separately from bug fixes or workarounds. E.g. 2/9 in this series makes much of the tests pass on *nix, but so does "--no-bin-wrappers", but just because it happens to bypass the broken-on-master bin-wrappers/* made by cmake.
On 21/10/2022 16:45, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Oct 21 2022, Phillip Wood wrote: > >> Hi Ævar >> >> On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote: >>> Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments >>> to tests. This allows for passing arguments to tests with e.g.: >>> GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 >>> --verbose >>> There's some overlap with this and what was suggested in [1], but as >>> noted there we're not passing "--verbose" and friends unconditionally, >>> so a plain "ctest" invocation without a "cmake" re-build won't pick up >>> the options. >> >> The aim of dscho's patch was to make debugging information available >> in the test logs without the user having to do anything, now to get >> that information every user has to set >> GIT_TEST_OPTS="--no-bin-wrappers --no-chain-lint -vx" when running >> cmake. >> >> I think it would be helpful to have some default options set if the >> user does not pass GIT_TEST_OPTS. Ideally one would be able to do >> >> GIT_TEST_OPTS=... ctest >> >> and have the tests pick up the options at runtime. Following on from >> my previous comment, if we used "sh -c" to launch the tests we could >> have something like >> >> COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2" >> ${GIT_TEST_OPTS:---no-bin-wrappers --no-chain-lint -vx}]] >> "${tsh}" "${CMAKE_BINARY_DIR}" "${tsh}" > > That sounds reasonable to me. FWIW I looked into > $CTEST_INTERACTIVE_DEBUG_MODE for this purpose, i.e. to stick something > like this in test-lib.sh: > > if test -n "$CTEST_INTERACTIVE_DEBUG_MODE" > then > verbose=t > trace=t > fi I think it is useful to have -vx set even if we're not passing --verbose to ctest as if a test fails we've got the information to debug it stored in ctest's log without having to re-run the test. > But I was hoping for some way to tell that "ctest" was in "--verbose" > mode, but AFAICT there's no way to get at that without something like > compat/linux/procinfo.c (basically a fancier way of parsing "ps auxf"). > > Anyway, as noted in my review of dscho's series I thought this part of > it was odd/outdated given that this thing runs on Linux (mostly, but > entirely after this series). > > I.e. why would we hardcode Windows-specific trade-offs into a portable > build-system, and if you do want e.g. "--no-bin-wrappers" why would you > want that just when you run "cmake", not "make"? Surely if we're pushing > for a new default it should be agnostic to the user's build system. > > But in any case I think if we're pushing for new (or cmake-specific) > opinionated defaults it makes sense to split those up & justify them > separately from bug fixes or workarounds. > > E.g. 2/9 in this series makes much of the tests pass on *nix, but so > does "--no-bin-wrappers", but just because it happens to bypass the > broken-on-master bin-wrappers/* made by cmake. I agree that --no-chain-lint --no-bin-wrappers are there just for windows. We could quite easily add those just for windows builds but the cmake build is basically only there for windows and the linux support is just there to allow testing the cmake build without having to run the ci. Best Wishes Phillip
On Tue, Oct 25 2022, Phillip Wood wrote: > On 21/10/2022 16:45, Ævar Arnfjörð Bjarmason wrote: >> On Fri, Oct 21 2022, Phillip Wood wrote: >> >>> Hi Ævar >>> >>> On 21/10/2022 10:44, Ævar Arnfjörð Bjarmason wrote: >>>> Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments >>>> to tests. This allows for passing arguments to tests with e.g.: >>>> GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 >>>> --verbose >>>> There's some overlap with this and what was suggested in [1], but as >>>> noted there we're not passing "--verbose" and friends unconditionally, >>>> so a plain "ctest" invocation without a "cmake" re-build won't pick up >>>> the options. >>> >>> The aim of dscho's patch was to make debugging information available >>> in the test logs without the user having to do anything, now to get >>> that information every user has to set >>> GIT_TEST_OPTS="--no-bin-wrappers --no-chain-lint -vx" when running >>> cmake. >>> >>> I think it would be helpful to have some default options set if the >>> user does not pass GIT_TEST_OPTS. Ideally one would be able to do >>> >>> GIT_TEST_OPTS=... ctest >>> >>> and have the tests pick up the options at runtime. Following on from >>> my previous comment, if we used "sh -c" to launch the tests we could >>> have something like >>> >>> COMMAND ${SH_EXE} -c [[GIT_TEST_BUILD_DIR="$1"; . "$2" >>> ${GIT_TEST_OPTS:---no-bin-wrappers --no-chain-lint -vx}]] >>> "${tsh}" "${CMAKE_BINARY_DIR}" "${tsh}" >> That sounds reasonable to me. FWIW I looked into >> $CTEST_INTERACTIVE_DEBUG_MODE for this purpose, i.e. to stick something >> like this in test-lib.sh: >> if test -n "$CTEST_INTERACTIVE_DEBUG_MODE" >> then >> verbose=t >> trace=t >> fi > > I think it is useful to have -vx set even if we're not passing > --verbose to ctest[...] Yeah, and to clarify, that's basically what this is, i.e. you don't need to pass --verbose. > [...] as if a test fails we've got the information to debug it stored > in ctest's log without having to re-run the test. Yes, that's sounds useful, but I'm still entirely unclear on why that needs to be build-system specific. I.e. you'll have the same with "make test" and "--verbose-log", with ctest the equivalent of a hypothetical "--tee-to-al-log" is the default, so we're partway towards a "--verbose-log". Is it just an omission and we should add it to t/Makefile and/or t/test-lib.sh eventually, or something cmake/ctest-specific in some way I don't get? >> But I was hoping for some way to tell that "ctest" was in "--verbose" >> mode, but AFAICT there's no way to get at that without something like >> compat/linux/procinfo.c (basically a fancier way of parsing "ps auxf"). >> Anyway, as noted in my review of dscho's series I thought this part >> of >> it was odd/outdated given that this thing runs on Linux (mostly, but >> entirely after this series). >> I.e. why would we hardcode Windows-specific trade-offs into a >> portable >> build-system, and if you do want e.g. "--no-bin-wrappers" why would you >> want that just when you run "cmake", not "make"? Surely if we're pushing >> for a new default it should be agnostic to the user's build system. >> But in any case I think if we're pushing for new (or cmake-specific) >> opinionated defaults it makes sense to split those up & justify them >> separately from bug fixes or workarounds. >> E.g. 2/9 in this series makes much of the tests pass on *nix, but so >> does "--no-bin-wrappers", but just because it happens to bypass the >> broken-on-master bin-wrappers/* made by cmake. > > I agree that --no-chain-lint --no-bin-wrappers are there just for > windows. We could quite easily add those just for windows builds but > the cmake build is basically only there for windows and the linux > support is just there to allow testing the cmake build without having > to run the ci. That part makes sense.
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 91b7009f4fd..8e29e3f514b 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -1083,9 +1083,11 @@ endif() file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") #test +set(GIT_TEST_OPTS "$ENV{GIT_TEST_OPTS}") +separate_arguments(GIT_TEST_OPTS) foreach(tsh ${test_scipts}) add_test(NAME ${tsh} - COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} + COMMAND env GIT_TEST_BUILD_DIR=${CMAKE_BINARY_DIR} ${SH_EXE} ${tsh} ${GIT_TEST_OPTS} WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t) endforeach()
Get "GIT_TEST_OPTS" from the environment, and use it to pass arguments to tests. This allows for passing arguments to tests with e.g.: GIT_TEST_OPTS="--verbose --debug" cmake .; ctest -R t0001 --verbose There's some overlap with this and what was suggested in [1], but as noted there we're not passing "--verbose" and friends unconditionally, so a plain "ctest" invocation without a "cmake" re-build won't pick up the options. 1. https://lore.kernel.org/git/356b2e9a1007bcd1382f26f333926ff0d5b9abe2.1666090745.git.gitgitgadget@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- contrib/buildsystems/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)