* [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
@ 2022-08-23 8:30 ` Johannes Schindelin via GitGitGadget
2022-09-07 22:10 ` Victoria Dye
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
2022-08-23 8:31 ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
` (5 subsequent siblings)
6 siblings, 2 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 8:30 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When a test script fails in Git's test suite, the usual course of action
is to re-run it using options to increase the verbosity of the output,
e.g. `-v` and `-x`.
Like in Git's CI runs, when running the tests in Visual Studio via the
CTest route, it is cumbersome or at least requires a very unintuitive
approach to pass options to the test scripts.
So let's just pass those options by default: This will not clutter any
output window but the log that is written to a log file will have
information necessary to figure out test failures.
While at it, also imitate what the Windows jobs in Git's CI runs do to
accelerate running the test scripts: pass the `--no-bin-wrappers` and
`--no-chain-lint` options. This makes the test runs noticeably faster
because the `bin-wrappers/` scripts as well as the `chain-lint` code
make heavy use of POSIX shell scripting, which is really, really slow on
Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1b23f2440d8..4aee1e24342 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
- COMMAND ${SH_EXE} ${tsh}
+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
@ 2022-09-07 22:10 ` Victoria Dye
2022-10-18 14:02 ` Johannes Schindelin
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 58+ messages in thread
From: Victoria Dye @ 2022-09-07 22:10 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin
Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts.
At first I wondered whether there's a way to make arg specification more
intuitive, rather than silently changing defaults. Unfortunately, it looks
like even in the latest versions of CTest don't really support passing
arguments through to tests [1] (and the workarounds are unpleasant at best).
But then, you mentioned that there *is* a cumbersome/unintuitive approach to
passing the options; what approach were you thinking?
[1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
Makes sense, I don't see any harm in providing more verbose output by
default here.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options. This makes the test runs noticeably faster
> because the `bin-wrappers/` scripts as well as the `chain-lint` code
> make heavy use of POSIX shell scripting, which is really, really slow on
> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
I'm a bit more hesitant on including these. I see how the performance
benefit (on Windows in particular) would make typical user experience nicer.
But, if someone develops locally with '--no-chain-lint' specified, they'll
be much more likely to miss a broken && chain (personally, I get caught by
chain lint errors *all the time* when I'm adding/updating tests) and cause
unnecessary churn in their patch submissions. So, my recommendation would be
to drop '--no-chain-lint' here (unless it's less important to the average
developer than I think it is).
It's also possible that '--no-bin-wrappers' does something weird with their
local installation of Git but I think it's safe enough to make the default
if the performance gain is substantial.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> #test
> foreach(tsh ${test_scipts})
> add_test(NAME ${tsh}
> - COMMAND ${SH_EXE} ${tsh}
> + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
As you have it here, I don't think there's a way for a user to override
these defaults (unless there's something about the manual workaround you
mentioned earlier that allows it). Since a user could feasibly want to set
their own options, could you add a build variable to CMakeLists.txt like
'GIT_TEST_OPTS'? You could use it to set the default options you have here,
but a user could still specify their own value at build time to override.
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
>
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-09-07 22:10 ` Victoria Dye
@ 2022-10-18 14:02 ` Johannes Schindelin
0 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin @ 2022-10-18 14:02 UTC (permalink / raw)
To: Victoria Dye
Cc: Johannes Schindelin via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Phillip Wood
Hi Victoria,
On Wed, 7 Sep 2022, Victoria Dye wrote:
> Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When a test script fails in Git's test suite, the usual course of action
> > is to re-run it using options to increase the verbosity of the output,
> > e.g. `-v` and `-x`.
> >
> > Like in Git's CI runs, when running the tests in Visual Studio via the
> > CTest route, it is cumbersome or at least requires a very unintuitive
> > approach to pass options to the test scripts.
>
> At first I wondered whether there's a way to make arg specification more
> intuitive, rather than silently changing defaults. Unfortunately, it looks
> like even in the latest versions of CTest don't really support passing
> arguments through to tests [1] (and the workarounds are unpleasant at best).
>
> But then, you mentioned that there *is* a cumbersome/unintuitive approach to
> passing the options; what approach were you thinking?
Thank you for pointing out that I assumed this to be more obvious than it
actually is.
The cumbersome way is to edit this part of the CMake definition
(https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096):
#test
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
COMMAND ${SH_EXE} ${tsh}
WORKING_DIRECTORY
${CMAKE_SOURCE_DIR}/t)
endforeach()
To pass an option like `-v --run=1,29`, the user would have to edit the
line `COMMAND ${SH_EXE} ${tsh}`, appending the options, and then
re-configure the CMake cache. And then not forget to _only_ run the test
script in question because those options do not make sense for the other
tests. And then not forget to undo the changes and re-configure the CMake
cache.
> [1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
>
> >
> > So let's just pass those options by default: This will not clutter any
> > output window but the log that is written to a log file will have
> > information necessary to figure out test failures.
>
> Makes sense, I don't see any harm in providing more verbose output by
> default here.
>
> >
> > While at it, also imitate what the Windows jobs in Git's CI runs do to
> > accelerate running the test scripts: pass the `--no-bin-wrappers` and
> > `--no-chain-lint` options. This makes the test runs noticeably faster
> > because the `bin-wrappers/` scripts as well as the `chain-lint` code
> > make heavy use of POSIX shell scripting, which is really, really slow on
> > Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> I'm a bit more hesitant on including these. I see how the performance
> benefit (on Windows in particular) would make typical user experience nicer.
> But, if someone develops locally with '--no-chain-lint' specified, they'll
> be much more likely to miss a broken && chain (personally, I get caught by
> chain lint errors *all the time* when I'm adding/updating tests) and cause
> unnecessary churn in their patch submissions. So, my recommendation would be
> to drop '--no-chain-lint' here (unless it's less important to the average
> developer than I think it is).
>
> It's also possible that '--no-bin-wrappers' does something weird with their
> local installation of Git but I think it's safe enough to make the default
> if the performance gain is substantial.
>
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > contrib/buildsystems/CMakeLists.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 1b23f2440d8..4aee1e24342 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> > #test
> > foreach(tsh ${test_scipts})
> > add_test(NAME ${tsh}
> > - COMMAND ${SH_EXE} ${tsh}
> > + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> As you have it here, I don't think there's a way for a user to override
> these defaults (unless there's something about the manual workaround you
> mentioned earlier that allows it). Since a user could feasibly want to set
> their own options, could you add a build variable to CMakeLists.txt like
> 'GIT_TEST_OPTS'? You could use it to set the default options you have here,
> but a user could still specify their own value at build time to override.
Indeed, there is no way to override this, unless the user edits the
(Git-tracked) `CMakeLists.txt` file and then re-configures the cache.
In my experience, the most common scenario where a developer needs to pass
options to test script is when they need to restrict execution to
individual test cases via, say, `--run=1,29`.
Such a use case cannot be covered via the CMake approach because it is not
fine-grained enough: any options the user could set would apply to the
entire test suite.
I added a comment to the commit message advising users to run the test
script manually via Git Bash instead, if they wish to pass a different set
of command-line options to the test script.
Thanks,
Dscho
>
> > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> > endforeach()
> >
>
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-09-07 22:10 ` Victoria Dye
@ 2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
2022-09-28 6:55 ` Eric Sunshine
1 sibling, 1 reply; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-09-08 7:22 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin, Eric Sunshine
On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts.
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options. This makes the test runs noticeably faster
> because the `bin-wrappers/` scripts as well as the `chain-lint` code
> make heavy use of POSIX shell scripting, which is really, really slow on
> Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 1b23f2440d8..4aee1e24342 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> #test
> foreach(tsh ${test_scipts})
> add_test(NAME ${tsh}
> - COMMAND ${SH_EXE} ${tsh}
> + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
The -vx part of this looks sensible, and matches the commit message
$subject.
I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
here should be stripped out, and put into its own commit.
Which, as I commented on in the v1[1] is making the implicit assumption
that this cmake file is only used on Windows, but since 561962479c
(cmake: fix CMakeLists.txt on Linux, 2022-05-24) that isn't the case.
So, perhaps we should have a performance hack due to Windows's slowness,
but:
A. It should be guarded by some "is windows?" check. Your commit
message justifies why this is a good idea on Windows, but completely
elides over the fact that this isn't Windows-specific code anymore.
B. We can still build wind "make" or "cmake", I don't see a reason for
why such a perf hack (if we decide to keep it) should depend on the
build system.
C. Since I sent [1] we've had submitted chainlint.pl in-flight
series. It's partly trying to take special considerations to be fast
on Windows. I don't think it's the case with that series that this
needs to be skipped on Windows anymore (and if it is, Eric would
like to know).
1. https://lore.kernel.org/git/220811.861qtnqb5p.gmgdl@evledraar.gmail.com/
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
@ 2022-09-28 6:55 ` Eric Sunshine
0 siblings, 0 replies; 58+ messages in thread
From: Eric Sunshine @ 2022-09-28 6:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin via GitGitGadget, Git List, Phillip Wood,
Johannes Schindelin
On Thu, Sep 8, 2022 at 3:28 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> > When a test script fails in Git's test suite, the usual course of action
> > is to re-run it using options to increase the verbosity of the output,
> > e.g. `-v` and `-x`.
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> I think the "--no-bin-wrappers --no-chain-lint" and the "while at it"
> here should be stripped out, and put into its own commit.
>
> So, perhaps we should have a performance hack due to Windows's slowness,
> but:
>
> C. Since I sent [1] we've had submitted chainlint.pl in-flight
> series. It's partly trying to take special considerations to be fast
> on Windows. I don't think it's the case with that series that this
> needs to be skipped on Windows anymore (and if it is, Eric would
> like to know).
I had the opportunity to test chainlint.pl on an 8-core machine using
both Linux and Windows 10. The machine is more than a few years old,
though not nearly as old as my own development machine. Although
chainlint.pl checked all test definitions in the entire project in
less than one _second_ on Linux, it took two _minutes_ on Windows 10
on the same machine (using all 8 cores). Despite the fact that Perl is
run just once to check all test definitions -- unlike chainlint.sed
running 'sed' tens of thousands of times, chainlint.pl is still
painfully slow on Windows, so I certainly understand the desire to use
--no-chain-lint, at least on Windows.
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v2 2/5] cmake: copy the merge tools for testing
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
@ 2022-08-23 8:31 ` Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
6 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 8:31 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Even when running the tests via CTest, t7609 and t7610 rely on more than
only a few mergetools to be copied to the build directory. Let's make it
so.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 4aee1e24342..fe606c179f7 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
#misc copies
file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
- file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
+ file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
+ file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
endif()
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
@ 2022-08-23 8:31 ` Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
6 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 8:31 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.
The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).
According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".
Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.
Let's avoid that by converting the unsigned bit explicitly to a signed
integer.
Note: This is a long-standing bug in the Visual C build of Git, but it
has never been caught because t3701 is skipped when `NO_PERL` is set,
which is the case in the `vs-test` jobs of Git's CI runs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
add-patch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
strbuf_remove(&s->answer, 0, 1);
strbuf_trim(&s->answer);
i = hunk_index - DISPLAY_HUNKS_LINES / 2;
- if (i < file_diff->mode_change)
+ if (i < (int)file_diff->mode_change)
i = file_diff->mode_change;
while (s->answer.len == 0) {
i = display_hunks(s, file_diff, i);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2022-08-23 8:31 ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
@ 2022-08-23 8:31 ` Johannes Schindelin via GitGitGadget
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
2022-09-08 23:37 ` Victoria Dye
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
6 siblings, 2 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 8:31 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 7f5397a07c6c (cmake: support for testing git when building out of the
source tree, 2020-06-26), we implemented support for running Git's test
scripts even after building Git in a different directory than the source
directory.
The way we did this was to edit the file `t/test-lib.sh` to override
`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
directory.
This is unideal because it always leaves a tracked file marked as
modified, and it is all too easy to commit that change by mistake.
Let's change the strategy by teaching `t/test-lib.sh` to detect the
presence of a file called `GIT-BUILD-DIR` in the source directory. If it
exists, the contents are interpreted as the location to the _actual_
build directory. We then write this file as part of the CTest
definition.
To support building Git via a regular `make` invocation after building
it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
convenience, this is done as part of the Makefile rule that is already
run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
up to date).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 +
Makefile | 1 +
contrib/buildsystems/CMakeLists.txt | 7 +------
t/test-lib.sh | 11 ++++++++++-
4 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/.gitignore b/.gitignore
index a4522157641..b72ddf09346 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
/fuzz_corpora
/fuzz-pack-headers
/fuzz-pack-idx
+/GIT-BUILD-DIR
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
/GIT-LDFLAGS
diff --git a/Makefile b/Makefile
index 04d0fd1fe60..9347ed90da7 100644
--- a/Makefile
+++ b/Makefile
@@ -3028,6 +3028,7 @@ else
@echo RUNTIME_PREFIX=\'false\' >>$@+
endif
@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+ @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
### Detect Python interpreter path changes
ifndef NO_PYTHON
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index fe606c179f7..29d7e236ae1 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1067,14 +1067,9 @@ endif()
#Make the tests work when building out of the source tree
get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
- file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
- string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
#Setting the build directory in test-lib.sh before running tests
file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
- "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
- "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
- "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
- "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
+ "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
#misc copies
file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 55857af601b..4468ac51f25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -42,7 +42,16 @@ then
TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
fi
GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
-if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+ GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
+ # On Windows, we must convert Windows paths lest they contain a colon
+ case "$(uname -s)" in
+ *MINGW*)
+ GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+ ;;
+ esac
+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
then
echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
exit 1
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
@ 2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:03 ` Johannes Schindelin
2022-09-08 23:37 ` Victoria Dye
1 sibling, 1 reply; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-09-08 7:39 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin
On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> .gitignore | 1 +
> Makefile | 1 +
> contrib/buildsystems/CMakeLists.txt | 7 +------
> t/test-lib.sh | 11 ++++++++++-
> 4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
> /fuzz_corpora
> /fuzz-pack-headers
> /fuzz-pack-idx
> +/GIT-BUILD-DIR
> /GIT-BUILD-OPTIONS
> /GIT-CFLAGS
> /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
> @echo RUNTIME_PREFIX=\'false\' >>$@+
> endif
> @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> + @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>
> ### Detect Python interpreter path changes
> ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
> #Make the tests work when building out of the source tree
> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> - file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> - string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> #Setting the build directory in test-lib.sh before running tests
> file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> - "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> - "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> + "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> #misc copies
> file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> fi
> GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> + # On Windows, we must convert Windows paths lest they contain a colon
> + case "$(uname -s)" in
> + *MINGW*)
> + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> + ;;
> + esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> then
> echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> exit 1
As pointed out in the v1 this breaks the cmake<->make interaction in
some scenarios, but from some brief testing there seemed to be an easy
workaround which didn't suffer from that problem:
https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
@ 2022-10-18 14:03 ` Johannes Schindelin
2022-10-18 15:09 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin @ 2022-10-18 14:03 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin via GitGitGadget, git, Phillip Wood
[-- Attachment #1: Type: text/plain, Size: 5317 bytes --]
Hi Ævar,
On Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
> >
> > To support building Git via a regular `make` invocation after building
> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> > convenience, this is done as part of the Makefile rule that is already
> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> > up to date).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > .gitignore | 1 +
> > Makefile | 1 +
> > contrib/buildsystems/CMakeLists.txt | 7 +------
> > t/test-lib.sh | 11 ++++++++++-
> > 4 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/.gitignore b/.gitignore
> > index a4522157641..b72ddf09346 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -2,6 +2,7 @@
> > /fuzz_corpora
> > /fuzz-pack-headers
> > /fuzz-pack-idx
> > +/GIT-BUILD-DIR
> > /GIT-BUILD-OPTIONS
> > /GIT-CFLAGS
> > /GIT-LDFLAGS
> > diff --git a/Makefile b/Makefile
> > index 04d0fd1fe60..9347ed90da7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -3028,6 +3028,7 @@ else
> > @echo RUNTIME_PREFIX=\'false\' >>$@+
> > endif
> > @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> > + @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
> >
> > ### Detect Python interpreter path changes
> > ifndef NO_PYTHON
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index fe606c179f7..29d7e236ae1 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1067,14 +1067,9 @@ endif()
> > #Make the tests work when building out of the source tree
> > get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> > if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> > - file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> > - string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> > #Setting the build directory in test-lib.sh before running tests
> > file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> > - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> > - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> > - "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> > - "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> > + "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> > #misc copies
> > file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> > file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> > TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> > fi
> > GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > + # On Windows, we must convert Windows paths lest they contain a colon
> > + case "$(uname -s)" in
> > + *MINGW*)
> > + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > + ;;
> > + esac
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > then
> > echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> > exit 1
>
> As pointed out in the v1 this breaks the cmake<->make interaction in
> some scenarios, but from some brief testing there seemed to be an easy
> workaround which didn't suffer from that problem:
> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/
I do not think that the CMake <-> make interaction will come up in any
other scenario than your and my tests in the context of this mailing list
thread.
Therefore, I am certain that we need not cater to that scenario at all.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-10-18 14:03 ` Johannes Schindelin
@ 2022-10-18 15:09 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-18 15:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Phillip Wood
On Tue, Oct 18 2022, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Thu, 8 Sep 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>>
>> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> >
>> > In 7f5397a07c6c (cmake: support for testing git when building out of the
>> > source tree, 2020-06-26), we implemented support for running Git's test
>> > scripts even after building Git in a different directory than the source
>> > directory.
>> >
>> > The way we did this was to edit the file `t/test-lib.sh` to override
>> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>> > directory.
>> >
>> > This is unideal because it always leaves a tracked file marked as
>> > modified, and it is all too easy to commit that change by mistake.
>> >
>> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
>> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>> > exists, the contents are interpreted as the location to the _actual_
>> > build directory. We then write this file as part of the CTest
>> > definition.
>> >
>> > To support building Git via a regular `make` invocation after building
>> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
>> > convenience, this is done as part of the Makefile rule that is already
>> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
>> > up to date).
>> >
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> > .gitignore | 1 +
>> > Makefile | 1 +
>> > contrib/buildsystems/CMakeLists.txt | 7 +------
>> > t/test-lib.sh | 11 ++++++++++-
>> > 4 files changed, 13 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/.gitignore b/.gitignore
>> > index a4522157641..b72ddf09346 100644
>> > --- a/.gitignore
>> > +++ b/.gitignore
>> > @@ -2,6 +2,7 @@
>> > /fuzz_corpora
>> > /fuzz-pack-headers
>> > /fuzz-pack-idx
>> > +/GIT-BUILD-DIR
>> > /GIT-BUILD-OPTIONS
>> > /GIT-CFLAGS
>> > /GIT-LDFLAGS
>> > diff --git a/Makefile b/Makefile
>> > index 04d0fd1fe60..9347ed90da7 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -3028,6 +3028,7 @@ else
>> > @echo RUNTIME_PREFIX=\'false\' >>$@+
>> > endif
>> > @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
>> > + @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>> >
>> > ### Detect Python interpreter path changes
>> > ifndef NO_PYTHON
>> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> > index fe606c179f7..29d7e236ae1 100644
>> > --- a/contrib/buildsystems/CMakeLists.txt
>> > +++ b/contrib/buildsystems/CMakeLists.txt
>> > @@ -1067,14 +1067,9 @@ endif()
>> > #Make the tests work when building out of the source tree
>> > get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
>> > if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
>> > - file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
>> > - string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
>> > #Setting the build directory in test-lib.sh before running tests
>> > file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
>> > - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
>> > - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
>> > - "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
>> > - "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
>> > + "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
>> > #misc copies
>> > file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
>> > file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
>> > diff --git a/t/test-lib.sh b/t/test-lib.sh
>> > index 55857af601b..4468ac51f25 100644
>> > --- a/t/test-lib.sh
>> > +++ b/t/test-lib.sh
>> > @@ -42,7 +42,16 @@ then
>> > TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>> > fi
>> > GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>> > +then
>> > + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
>> > + # On Windows, we must convert Windows paths lest they contain a colon
>> > + case "$(uname -s)" in
>> > + *MINGW*)
>> > + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>> > + ;;
>> > + esac
>> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> > then
>> > echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>> > exit 1
>>
>> As pointed out in the v1 this breaks the cmake<->make interaction in
>> some scenarios, but from some brief testing there seemed to be an easy
>> workaround which didn't suffer from that problem:
>> https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/
>
> I do not think that the CMake <-> make interaction will come up in any
> other scenario than your and my tests in the context of this mailing list
> thread.
>
> Therefore, I am certain that we need not cater to that scenario at all.
I run it like that now when I build locally, and it slots nicely in with:
1. First run a bunch of 'build' targets
2. Then run a bunch of 'test' targets
I'm not opposed to having this e.g. as a special-case on Windows, or
that we pick up when you *only* have cmake/ctest unconditionally, and
auto-do the right thing.
But I do think that:
A. Getting a "run test-lib against this build dir over there" is
*different* from the bundled behavior of the auto-flip-flopping.
B. Your case for doing it this way seems to be the
Windows-specific case noted in 1/5, can't we just make that
part Windows-specific then? I don't get why it needs to be
bundled up with cmake everywhere....
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
@ 2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
` (2 more replies)
1 sibling, 3 replies; 58+ messages in thread
From: Victoria Dye @ 2022-09-08 23:37 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin
Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
While I like that this removes a user error case, it sacrifices some of the
separation between contrib/ and the main Git tree by adding logic to
'test-lib.sh' that only really benefits the CMake build.
To your point in [1]:
> Can we maybe agree that the proposed patch is a net improvement over the
> status quo, and think about a better solution independently (without
> blocking this here patch)?
I don't think it does more harm than good, but I wouldn't go so far as to
call it a definitive "net improvement." I'd personally very much prefer a
solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
CMake. Unfortunately, after spending a pretty substantial amount of time
looking for an alternative, I couldn't think of anything that didn't either
1) change how users ran tests or 2) change where CMake builds wrote Git's
binary files.
So, I could go either way on this patch - if others feel strongly that it
should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless
someone can think of a better alternative.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> .gitignore | 1 +
> Makefile | 1 +
> contrib/buildsystems/CMakeLists.txt | 7 +------
> t/test-lib.sh | 11 ++++++++++-
> 4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index a4522157641..b72ddf09346 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -2,6 +2,7 @@
> /fuzz_corpora
> /fuzz-pack-headers
> /fuzz-pack-idx
> +/GIT-BUILD-DIR
> /GIT-BUILD-OPTIONS
> /GIT-CFLAGS
> /GIT-LDFLAGS
> diff --git a/Makefile b/Makefile
> index 04d0fd1fe60..9347ed90da7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3028,6 +3028,7 @@ else
> @echo RUNTIME_PREFIX=\'false\' >>$@+
> endif
> @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> + @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
>
> ### Detect Python interpreter path changes
> ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index fe606c179f7..29d7e236ae1 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
> #Make the tests work when building out of the source tree
> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> - file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> - string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> #Setting the build directory in test-lib.sh before running tests
> file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> - "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> - "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> + "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> #misc copies
> file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 55857af601b..4468ac51f25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,7 +42,16 @@ then
> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> fi
> GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> + # On Windows, we must convert Windows paths lest they contain a colon
> + case "$(uname -s)" in
> + *MINGW*)
> + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> + ;;
> + esac
> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> then
> echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> exit 1
Referring to Ævar's review in [1] - while I'm not overly concerned about the
"switching between make & CMake" file staleness (if I'm not mistaken, the
same thing can happen now with the modified 'test-lib.sh', so this patch
doesn't really make anything worse), I do think the changes to 'test-lib.sh'
should be rearranged to preserve the "PANIC" check:
----------------->8----------------->8----------------->8-----------------
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4468ac51f2..7b57f55c37 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -42,6 +42,11 @@ then
TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
fi
GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
+if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+then
+ echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+ exit 1
+fi
if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
then
GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
@@ -51,10 +56,6 @@ then
GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
;;
esac
-elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
-then
- echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
- exit 1
fi
# Prepend a string to a VAR using an arbitrary ":" delimiter, not
-----------------8<-----------------8<-----------------8<-----------------
Otherwise, a user could run the tests from outside a 't/' directory if they
built Git with CMake, which doesn't appear to be part of the intended
behavior of this patch.
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-09-08 23:37 ` Victoria Dye
@ 2022-09-08 23:42 ` Victoria Dye
2022-09-08 23:58 ` Junio C Hamano
2022-10-18 14:03 ` Johannes Schindelin
2 siblings, 0 replies; 58+ messages in thread
From: Victoria Dye @ 2022-09-08 23:42 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin
Victoria Dye wrote:
> Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In 7f5397a07c6c (cmake: support for testing git when building out of the
>> source tree, 2020-06-26), we implemented support for running Git's test
>> scripts even after building Git in a different directory than the source
>> directory.
>>
>> The way we did this was to edit the file `t/test-lib.sh` to override
>> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
>> directory.
>>
>> This is unideal because it always leaves a tracked file marked as
>> modified, and it is all too easy to commit that change by mistake.
>>
>> Let's change the strategy by teaching `t/test-lib.sh` to detect the
>> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
>> exists, the contents are interpreted as the location to the _actual_
>> build directory. We then write this file as part of the CTest
>> definition.
>>
>> To support building Git via a regular `make` invocation after building
>> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
>> convenience, this is done as part of the Makefile rule that is already
>> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
>> up to date).
>
> While I like that this removes a user error case, it sacrifices some of the
> separation between contrib/ and the main Git tree by adding logic to
> 'test-lib.sh' that only really benefits the CMake build.
>
> To your point in [1]:
Forgot this link: https://lore.kernel.org/git/8o4q98s3-sr2r-34qq-p7pr-8o44061o0n76@tzk.qr/
>
>> Can we maybe agree that the proposed patch is a net improvement over the
>> status quo, and think about a better solution independently (without
>> blocking this here patch)?
>
> I don't think it does more harm than good, but I wouldn't go so far as to
> call it a definitive "net improvement." I'd personally very much prefer a
> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
> CMake. Unfortunately, after spending a pretty substantial amount of time
> looking for an alternative, I couldn't think of anything that didn't either
> 1) change how users ran tests or 2) change where CMake builds wrote Git's
> binary files.
>
> So, I could go either way on this patch - if others feel strongly that it
> should be dropped, I'll defer to that. Otherwise, I'm fine keeping it unless
> someone can think of a better alternative.
>
...
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 55857af601b..4468ac51f25 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -42,7 +42,16 @@ then
>> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
>> fi
>> GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
>> -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
>> +then
>> + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
>> + # On Windows, we must convert Windows paths lest they contain a colon
>> + case "$(uname -s)" in
>> + *MINGW*)
>> + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
>> + ;;
>> + esac
>> +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
>> then
>> echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
>> exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the
AND this one (which should be [2]): https://lore.kernel.org/git/220811.86sfm3ov5z.gmgdl@evledraar.gmail.com/
Sorry about that!
> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
>
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> fi
> GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> +if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +then
> + echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> + exit 1
> +fi
> if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> then
> GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
> GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> ;;
> esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> - echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> - exit 1
> fi
>
> # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
>
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
@ 2022-09-08 23:58 ` Junio C Hamano
2022-10-18 14:03 ` Johannes Schindelin
2 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2022-09-08 23:58 UTC (permalink / raw)
To: Victoria Dye
Cc: Johannes Schindelin via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin
Victoria Dye <vdye@github.com> writes:
> I don't think it does more harm than good, but I wouldn't go so far as to
> call it a definitive "net improvement." I'd personally very much prefer a
> solution that didn't involve adding 'GIT-BUILD-DIR' just for the sake of
> CMake.
> ...
> ... "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
Thanks for a quality review.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh
2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
2022-09-08 23:58 ` Junio C Hamano
@ 2022-10-18 14:03 ` Johannes Schindelin
2 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin @ 2022-10-18 14:03 UTC (permalink / raw)
To: Victoria Dye
Cc: Johannes Schindelin via GitGitGadget, git,
Ævar Arnfjörð Bjarmason, Phillip Wood
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
Hi Victoria,
On Thu, 8 Sep 2022, Victoria Dye wrote:
> Johannes Schindelin via GitGitGadget wrote:
>
> > [...]
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 55857af601b..4468ac51f25 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -42,7 +42,16 @@ then
> > TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> > fi
> > GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> > -if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> > +then
> > + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> > + # On Windows, we must convert Windows paths lest they contain a colon
> > + case "$(uname -s)" in
> > + *MINGW*)
> > + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> > + ;;
> > + esac
> > +elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> > then
> > echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> > exit 1
> Referring to Ævar's review in [1] - while I'm not overly concerned about the
> "switching between make & CMake" file staleness (if I'm not mistaken, the
> same thing can happen now with the modified 'test-lib.sh', so this patch
> doesn't really make anything worse), I do think the changes to 'test-lib.sh'
> should be rearranged to preserve the "PANIC" check:
>
> ----------------->8----------------->8----------------->8-----------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 4468ac51f2..7b57f55c37 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -42,6 +42,11 @@ then
> TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
> fi
> GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
> +if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> +then
> + echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> + exit 1
> +fi
> if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> then
> GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> @@ -51,10 +56,6 @@ then
> GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> ;;
> esac
> -elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
> -then
> - echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> - exit 1
> fi
>
> # Prepend a string to a VAR using an arbitrary ":" delimiter, not
> -----------------8<-----------------8<-----------------8<-----------------
>
> Otherwise, a user could run the tests from outside a 't/' directory if they
> built Git with CMake, which doesn't appear to be part of the intended
> behavior of this patch.
Good point!
Thank you,
Dscho
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v2 5/5] cmake: increase time-out for a long-running test
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
@ 2022-08-23 8:31 ` Johannes Schindelin via GitGitGadget
2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
2022-09-08 3:51 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Victoria Dye
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
6 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-08-23 8:31 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
As suggested in
https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
t7112 can run for well over one hour, which seems to be the default
maximum run time at least when running CTest-based tests in Visual
Studio.
Let's increase the time-out as a stop gap to unblock developers wishing
to run Git's test suite in Visual Studio.
Note: The actual run time is highly dependent on the circumstances. For
example, in Git's CI runs, the Windows-based tests typically take a bit
over 5 minutes to run. CI runs have the added benefit that Windows
Defender (the common anti-malware scanner on Windows) is turned off,
something many developers are not at liberty to do on their work
stations. When Defender is turned on, even on this developer's high-end
Ryzen system, t7112 takes over 15 minutes to run.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 29d7e236ae1..b1306f95256 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
+# This test script takes an extremely long time and is known to time out even
+# on fast machines because it requires in excess of one hour to run
+set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+
endif()#BUILD_TESTING
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
@ 2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
2022-09-08 17:29 ` Victoria Dye
0 siblings, 1 reply; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-09-08 7:34 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin
On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> As suggested in
> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
> t7112 can run for well over one hour, which seems to be the default
> maximum run time at least when running CTest-based tests in Visual
> Studio.
>
> Let's increase the time-out as a stop gap to unblock developers wishing
> to run Git's test suite in Visual Studio.
>
> Note: The actual run time is highly dependent on the circumstances. For
> example, in Git's CI runs, the Windows-based tests typically take a bit
> over 5 minutes to run. CI runs have the added benefit that Windows
> Defender (the common anti-malware scanner on Windows) is turned off,
> something many developers are not at liberty to do on their work
> stations. When Defender is turned on, even on this developer's high-end
> Ryzen system, t7112 takes over 15 minutes to run.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 29d7e236ae1..b1306f95256 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
>
> +# This test script takes an extremely long time and is known to time out even
> +# on fast machines because it requires in excess of one hour to run
> +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
> +
> endif()#BUILD_TESTING
I don't see per [1] that it would have any negative effect to just bump
the timeout a lot more, and for all the tests.
If we're running into 3600 seconds, then setting this to 4000 seconds
seems to be a smal stopgap at best. That's just over a 10% increase, so
if one person ran into it it 3600 someone with a slightly slower system
should be running into the same still, or if we just add a few more
tests to 7112 (or some other slow test).
So why not set this ta 3600*10 or whatever for *all* of the test scripts
instead of playing whack-a-mole?
1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2 5/5] cmake: increase time-out for a long-running test
2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
@ 2022-09-08 17:29 ` Victoria Dye
0 siblings, 0 replies; 58+ messages in thread
From: Victoria Dye @ 2022-09-08 17:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason,
Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Johannes Schindelin
Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Aug 23 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> As suggested in
>> https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
>> t7112 can run for well over one hour, which seems to be the default
>> maximum run time at least when running CTest-based tests in Visual
>> Studio.
>>
>> Let's increase the time-out as a stop gap to unblock developers wishing
>> to run Git's test suite in Visual Studio.
>>
>> Note: The actual run time is highly dependent on the circumstances. For
>> example, in Git's CI runs, the Windows-based tests typically take a bit
>> over 5 minutes to run. CI runs have the added benefit that Windows
>> Defender (the common anti-malware scanner on Windows) is turned off,
>> something many developers are not at liberty to do on their work
>> stations. When Defender is turned on, even on this developer's high-end
>> Ryzen system, t7112 takes over 15 minutes to run.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> contrib/buildsystems/CMakeLists.txt | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> index 29d7e236ae1..b1306f95256 100644
>> --- a/contrib/buildsystems/CMakeLists.txt
>> +++ b/contrib/buildsystems/CMakeLists.txt
>> @@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
>> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>> endforeach()
>>
>> +# This test script takes an extremely long time and is known to time out even
>> +# on fast machines because it requires in excess of one hour to run
>> +set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
>> +
>> endif()#BUILD_TESTING
>
> I don't see per [1] that it would have any negative effect to just bump
> the timeout a lot more, and for all the tests.
I'm a bit confused as to how that link (which explains that the 'TIMEOUT'
property is used to determine when to kill a test process - more or less
standard behavior for timeouts in applications) shows that excessively long
timeouts on every test "[wouldn't] have any negative effect."
In terms of the timeout length, the stated reason for adding the timeout in
the first place is:
>> Let's increase the time-out as a stop gap to unblock developers wishing
>> to run Git's test suite in Visual Studio.
I would consider ~1 hour a manageable (albeit frustrating) amount of time to
wait for a test, but not 10 hours (as you suggest later).
As for setting the timeout on all tests: that might be helpful, only because
(AFAICT from CMake's source code & documentation) the default timeout is "no
timeout." If setting a global default, though, I'd prefer a shorter timespan
(e.g. 1800s) so that a timeout properly indicates a failure condition ("this
test runs much longer than it should, so we may have introduced a
performance bug").
Practically, though, I don't feel too strongly about whether or not to set a
global default. If we wanted to add timeouts to more tests in the future,
I'd want a more extensible solution with carefully-selected timeouts. But if
it's just this one test running too long, special case-ing a timeout for it
with a comment explaining the reasoning seems appropriate to me.
>
> If we're running into 3600 seconds, then setting this to 4000 seconds
> seems to be a smal stopgap at best. That's just over a 10% increase, so
> if one person ran into it it 3600 someone with a slightly slower system
> should be running into the same still, or if we just add a few more
> tests to 7112 (or some other slow test).
I don't think this patch is suggesting that timeout failures are happening
at 3600s, rather that the peak observed execution time of the test is 3600s.
If that's the case, <peak execution time> + an 11% buffer is a reasonable
choice for the value.
>
> So why not set this ta 3600*10 or whatever for *all* of the test scripts
> instead of playing whack-a-mole?
>
> 1. https://cmake.org/cmake/help/latest/prop_test/TIMEOUT.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
@ 2022-09-08 3:51 ` Victoria Dye
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
6 siblings, 0 replies; 58+ messages in thread
From: Victoria Dye @ 2022-09-08 3:51 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Johannes Schindelin
Johannes Schindelin via GitGitGadget wrote:
> Visual Studio users enjoy support for running the test suite via CTest,
> thanks to Git's CMake definition.
>
> In https://github.com/git-for-windows/git/issues/3966, it has been reported
> that this does not work out of the box, though, but causes a couple of test
> failures instead. These problems are not caught by Git's CI runs because the
> vs-tests jobs actually use prove to run the test suite, not CTest.
>
> In addition to fixing these problems, this patch series also addresses a
> long-standing gripe I have with the way Git's CMake definition supports
> CTest: It edits t/test-lib.sh, which leaves this file eternally modified
> (but these modification should never be committed, they refer to a
> local-only, configuration-dependent directory).
>
> Note: The signed/unsigned comparison bug in git add -p that is fixed in this
> here patch series is a relatively big one, and it merits further
> investigation whether there are similar bugs lurking in Git's code base.
> However, this is a much bigger project than can be addressed as part of this
> patch series, in particular because the analysis would require tools other
> than GCC's -Wsign-compare option (which totally misses the instance that is
> fixed in this here patch series).
>
> Changes since v1:
>
> * Clarified why it is a good idea to pass --no-bin-wrappers and
> --no-chain-lint when running on Windows.
> * Clarified why the add -p bug has not been caught earlier.
> * Clarified the scope of this patch series to fix running Git's tests
> within Visual Studio.
> * Increased the time-out for the very slow t7112 test script.
> * The test_chmod was determined to be not only faulty, but unneeded, and
> was dropped.
>
> Johannes Schindelin (5):
> cmake: make it easier to diagnose regressions in CTest runs
> cmake: copy the merge tools for testing
> add -p: avoid ambiguous signed/unsigned comparison
> cmake: avoid editing t/test-lib.sh
> cmake: increase time-out for a long-running test
I've reviewed patches 1-3 & 5 and started looking over 4, but I'd like to
spend a bit more time on it (mostly to understand pros/cons of the
'GIT-BUILD-DIR' vs. any alternatives). I'm planning to finish that up
tomorrow.
Thanks for your patience!
>
> .gitignore | 1 +
> Makefile | 1 +
> add-patch.c | 2 +-
> contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
> t/test-lib.sh | 11 ++++++++++-
> 5 files changed, 21 insertions(+), 10 deletions(-)
>
>
> base-commit: bbea4dcf42b28eb7ce64a6306cdde875ae5d09ca
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1320
>
> Range-diff vs v1:
>
> 1: 9cf14984c0a ! 1: e00cb37b98a cmake: align CTest definition with Git's CI runs
> @@ Metadata
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> ## Commit message ##
> - cmake: align CTest definition with Git's CI runs
> + cmake: make it easier to diagnose regressions in CTest runs
>
> - In Git's CI runs, the Windows tests are run with `--no-bin-wrappers` and
> - `--no-chain-lint`, mainly to win back some time caused by the serious
> - performance penalty paid for the tests relying so heavily on POSIX shell
> - scripting, which only works by using a POSIX emulation layer.
> + When a test script fails in Git's test suite, the usual course of action
> + is to re-run it using options to increase the verbosity of the output,
> + e.g. `-v` and `-x`.
>
> - Let's do the same when running the tests, say, in Visual Studio.
> + Like in Git's CI runs, when running the tests in Visual Studio via the
> + CTest route, it is cumbersome or at least requires a very unintuitive
> + approach to pass options to the test scripts.
>
> - While at it, enable the command trace via `-x` and verbose output via
> - `-v`, otherwise it would be near impossible to diagnose any problems.
> + So let's just pass those options by default: This will not clutter any
> + output window but the log that is written to a log file will have
> + information necessary to figure out test failures.
> +
> + While at it, also imitate what the Windows jobs in Git's CI runs do to
> + accelerate running the test scripts: pass the `--no-bin-wrappers` and
> + `--no-chain-lint` options. This makes the test runs noticeably faster
> + because the `bin-wrappers/` scripts as well as the `chain-lint` code
> + make heavy use of POSIX shell scripting, which is really, really slow on
> + Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 2: 86ab58b6508 = 2: de7b47a9aa7 cmake: copy the merge tools for testing
> 3: 79abfa82c32 < -: ----------- tests: explicitly skip `chmod` calls on Windows
> 4: 4d24a4345ba ! 3: f96d5ab484c add -p: avoid ambiguous signed/unsigned comparison
> @@ Commit message
> Let's avoid that by converting the unsigned bit explicitly to a signed
> integer.
>
> + Note: This is a long-standing bug in the Visual C build of Git, but it
> + has never been caught because t3701 is skipped when `NO_PERL` is set,
> + which is the case in the `vs-test` jobs of Git's CI runs.
> +
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ## add-patch.c ##
> 5: c7fc5a4ee4c = 4: 22473d6b8f3 cmake: avoid editing t/test-lib.sh
> -: ----------- > 5: 6aaa675301c cmake: increase time-out for a long-running test
>
^ permalink raw reply [flat|nested] 58+ messages in thread* [PATCH v3 0/5] Some fixes and an improvement for using CTest on Windows
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
` (5 preceding siblings ...)
2022-09-08 3:51 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Victoria Dye
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
` (4 more replies)
6 siblings, 5 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin
Visual Studio users enjoy support for running the test suite via CTest,
thanks to Git's CMake definition.
In https://github.com/git-for-windows/git/issues/3966, it has been reported
that this does not work out of the box, though, but causes a couple of test
failures instead. These problems are not caught by Git's CI runs because the
vs-tests jobs actually use prove to run the test suite, not CTest.
In addition to fixing these problems, this patch series also addresses a
long-standing gripe I have with the way Git's CMake definition supports
CTest: It edits t/test-lib.sh, which leaves this file eternally modified
(but these modification should never be committed, they refer to a
local-only, configuration-dependent directory).
Note: The signed/unsigned comparison bug in git add -p that is fixed in this
here patch series is a relatively big one, and it merits further
investigation whether there are similar bugs lurking in Git's code base.
However, this is a much bigger project than can be addressed as part of this
patch series, in particular because the analysis would require tools other
than GCC's -Wsign-compare option (which totally misses the instance that is
fixed in this here patch series).
Changes since v2:
* Enhanced a commit message to clarify how the "cumbersome approach" looks
like when a developer wishes to pass options to the test scripts in
Visual Studio.
* Changed the base branch to ac/fuzzers, to avoid merge conflicts in
.gitignore.
* Moved the PANIC check in test-lib.sh so that it is not skipped when the
file GIT-BUILD-DIR exists.
Changes since v1:
* Clarified why it is a good idea to pass --no-bin-wrappers and
--no-chain-lint when running on Windows.
* Clarified why the add -p bug has not been caught earlier.
* Clarified the scope of this patch series to fix running Git's tests
within Visual Studio.
* Increased the time-out for the very slow t7112 test script.
* The test_chmod was determined to be not only faulty, but unneeded, and
was dropped.
Johannes Schindelin (5):
cmake: make it easier to diagnose regressions in CTest runs
cmake: copy the merge tools for testing
add -p: avoid ambiguous signed/unsigned comparison
cmake: avoid editing t/test-lib.sh
cmake: increase time-out for a long-running test
.gitignore | 1 +
Makefile | 1 +
add-patch.c | 2 +-
contrib/buildsystems/CMakeLists.txt | 16 ++++++++--------
t/test-lib.sh | 10 ++++++++++
5 files changed, 21 insertions(+), 9 deletions(-)
base-commit: 6713bfc70c4dc6da1fa4084f000b72f5d74fecfb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1320%2Fdscho%2Fctest-on-windows-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1320/dscho/ctest-on-windows-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1320
Range-diff vs v2:
1: e00cb37b98a ! 1: 356b2e9a100 cmake: make it easier to diagnose regressions in CTest runs
@@ Commit message
Like in Git's CI runs, when running the tests in Visual Studio via the
CTest route, it is cumbersome or at least requires a very unintuitive
- approach to pass options to the test scripts.
+ approach to pass options to the test scripts: the CMakeLists.txt file
+ would have to be modified, passing the desired options to _all_ test
+ scripts, and then the CMake Cache would have to be reconfigured before
+ running the test in question individually. Unintuitive at best, and
+ opposite to the niceties IDE users expect.
So let's just pass those options by default: This will not clutter any
output window but the log that is written to a log file will have
@@ Commit message
While at it, also imitate what the Windows jobs in Git's CI runs do to
accelerate running the test scripts: pass the `--no-bin-wrappers` and
- `--no-chain-lint` options. This makes the test runs noticeably faster
- because the `bin-wrappers/` scripts as well as the `chain-lint` code
- make heavy use of POSIX shell scripting, which is really, really slow on
- Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
+ `--no-chain-lint` options.
+
+ This makes the test runs noticeably faster because the `bin-wrappers/`
+ scripts as well as the `chain-lint` code make heavy use of POSIX shell
+ scripting, which is really, really slow on Windows due to the need to
+ emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
+ Sunshine, it added two minutes (!) just to perform the chain-lint task.
+
+ The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
+ considered during the development of this patch, but then dropped: such
+ a setting is global, across _all_ tests, where e.g. `--run=...` would
+ not make sense. Users wishing to override these new defaults are better
+ advised running the test script manually, in a Git Bash, with full
+ control over the command line.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
2: de7b47a9aa7 = 2: 9faca9d5cbe cmake: copy the merge tools for testing
3: f96d5ab484c = 3: 41a8021a4bd add -p: avoid ambiguous signed/unsigned comparison
4: 22473d6b8f3 ! 4: 5b0c2a150e9 cmake: avoid editing t/test-lib.sh
@@ Commit message
## .gitignore ##
@@
/fuzz_corpora
- /fuzz-pack-headers
- /fuzz-pack-idx
+/GIT-BUILD-DIR
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
@@ contrib/buildsystems/CMakeLists.txt: endif()
## t/test-lib.sh ##
@@ t/test-lib.sh: then
- TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY
+ echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
+ exit 1
fi
- GIT_BUILD_DIR="${TEST_DIRECTORY%/t}"
--if test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+ GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
@@ t/test-lib.sh: then
+ GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+ ;;
+ esac
-+elif test "$TEST_DIRECTORY" = "$GIT_BUILD_DIR"
- then
- echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
- exit 1
++fi
+
+ # Prepend a string to a VAR using an arbitrary ":" delimiter, not
+ # adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
5: 6aaa675301c = 5: 40cf872f483 cmake: increase time-out for a long-running test
--
gitgitgadget
^ permalink raw reply [flat|nested] 58+ messages in thread* [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
2022-10-18 13:41 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When a test script fails in Git's test suite, the usual course of action
is to re-run it using options to increase the verbosity of the output,
e.g. `-v` and `-x`.
Like in Git's CI runs, when running the tests in Visual Studio via the
CTest route, it is cumbersome or at least requires a very unintuitive
approach to pass options to the test scripts: the CMakeLists.txt file
would have to be modified, passing the desired options to _all_ test
scripts, and then the CMake Cache would have to be reconfigured before
running the test in question individually. Unintuitive at best, and
opposite to the niceties IDE users expect.
So let's just pass those options by default: This will not clutter any
output window but the log that is written to a log file will have
information necessary to figure out test failures.
While at it, also imitate what the Windows jobs in Git's CI runs do to
accelerate running the test scripts: pass the `--no-bin-wrappers` and
`--no-chain-lint` options.
This makes the test runs noticeably faster because the `bin-wrappers/`
scripts as well as the `chain-lint` code make heavy use of POSIX shell
scripting, which is really, really slow on Windows due to the need to
emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
Sunshine, it added two minutes (!) just to perform the chain-lint task.
The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
considered during the development of this patch, but then dropped: such
a setting is global, across _all_ tests, where e.g. `--run=...` would
not make sense. Users wishing to override these new defaults are better
advised running the test script manually, in a Git Bash, with full
control over the command line.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2237109b57f..6ac20bc5054 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
#test
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
- COMMAND ${SH_EXE} ${tsh}
+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
@ 2022-10-18 13:41 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-18 13:41 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Victoria Dye, Eric Sunshine,
Johannes Schindelin
On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When a test script fails in Git's test suite, the usual course of action
> is to re-run it using options to increase the verbosity of the output,
> e.g. `-v` and `-x`.
>
> Like in Git's CI runs, when running the tests in Visual Studio via the
> CTest route, it is cumbersome or at least requires a very unintuitive
> approach to pass options to the test scripts: the CMakeLists.txt file
> would have to be modified, passing the desired options to _all_ test
> scripts, and then the CMake Cache would have to be reconfigured before
> running the test in question individually. Unintuitive at best, and
> opposite to the niceties IDE users expect.
>
> So let's just pass those options by default: This will not clutter any
> output window but the log that is written to a log file will have
> information necessary to figure out test failures.
>
> While at it, also imitate what the Windows jobs in Git's CI runs do to
> accelerate running the test scripts: pass the `--no-bin-wrappers` and
> `--no-chain-lint` options.
>
> This makes the test runs noticeably faster because the `bin-wrappers/`
> scripts as well as the `chain-lint` code make heavy use of POSIX shell
> scripting, which is really, really slow on Windows due to the need to
> emulate POSIX behavior via the MSYS2 runtime. In a test by Eric
> Sunshine, it added two minutes (!) just to perform the chain-lint task.
>
> The idea of adding a CMake config option (á la `GIT_TEST_OPTS`) was
> considered during the development of this patch, but then dropped: such
> a setting is global, across _all_ tests, where e.g. `--run=...` would
> not make sense. Users wishing to override these new defaults are better
> advised running the test script manually, in a Git Bash, with full
> control over the command line.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 2237109b57f..6ac20bc5054 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> #test
> foreach(tsh ${test_scipts})
> add_test(NAME ${tsh}
> - COMMAND ${SH_EXE} ${tsh}
> + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
This all seems sensible, but where you lose me is why this needs to be
cmake-specific.
Maybe I'm just missing something, but everything you're describing seems
to me to be Windows-specific, e.g. "chain lint is slow on windows" and
the like.
Why aren't we just declaring that it's not worth it on Windows, but
doing so in test-lib.sh, or at least doing it in both cmake's recipe &
the Makefile?
I can see why we'd e.g. want it not to run with "make test", but want it
when you run individual tests, but then both could just set some "this
is running as a batch" env flag or whatever.
Also, even then everything you're describing is eliding that:
- This ill makes sense for Windows, sure, but the cmake recipe is no
longer Windows-specific, was this authored before that happened &
rebased? Why do we want cmake/ctest on Linux to have --no-chain-lint?
- Having cmake be some "mostly the Makefile, but also other defaults"
is really changing the scope of the whole cmake build alternative,
which until now (but I may have missed some other special-cases) has
been "let's build it like the Makefile".
- That "ctest" doesn't work on Linux *at all* currently, this fixes it,
but is really sweeping the underlying issue under the rug. AFAICT
it's been limping along because there's no "chmod +x" or whatever on
Windows (IIRC we magically look at she shebang).
For that last one & more I hacked up
https://github.com/avar/git/tree/avar/cmake-test-path a few days ago as
an alternative to some parts of this series, but given past exchanges
wanted to ask you first what you thought about it off-list, but didn't
get around to it..
But anyway, I do think starting with something like
https://github.com/avar/git/commit/5bd0ee2bc826d2bb358af9d93c88ba28584bbda1
makes more sense, i.e. get "ctest" to work at all with the
bin-wrappers/* in a portable way, and then *maybe* tweak it/Makefile to
pass some custom flags on some platform(s).
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 2/5] cmake: copy the merge tools for testing
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
2022-10-18 13:49 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
4 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Even when running the tests via CTest, t7609 and t7610 rely on more than
only a few mergetools to be copied to the build directory. Let's make it
so.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 6ac20bc5054..0c741e7d878 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
#misc copies
file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
- file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
+ file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
+ file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
endif()
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 2/5] cmake: copy the merge tools for testing
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
@ 2022-10-18 13:49 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-18 13:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Victoria Dye, Eric Sunshine,
Johannes Schindelin
On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Even when running the tests via CTest, t7609 and t7610 rely on more than
> only a few mergetools to be copied to the build directory. Let's make it
> so.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 6ac20bc5054..0c741e7d878 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1078,7 +1078,8 @@ if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> #misc copies
> file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
> - file(COPY ${CMAKE_SOURCE_DIR}/mergetools/tkdiff DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
> + file(GLOB mergetools "${CMAKE_SOURCE_DIR}/mergetools/*")
> + file(COPY ${mergetools} DESTINATION ${CMAKE_BINARY_DIR}/mergetools/)
> file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-prompt.sh DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
> file(COPY ${CMAKE_SOURCE_DIR}/contrib/completion/git-completion.bash DESTINATION ${CMAKE_BINARY_DIR}/contrib/completion/)
> endif()
I mentioned that I hacked up a "root cause fix" for some issues in cmake
in 1/5. Continuing that this works, but is really just a
hack-for-the-hack fix, where we should fix the root cause:
Which actually, is really simple:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
As you know we fake up the $GIT_BUILD_DIR for cmake, but then later in
the recipe (i.e. here) we have to scurry around and adjust that because
for some stuff we mean contrib/buildsystems/out/<path>, whereas in
others we mean the original <path>.
So here (and this isn't on you, we had this before) if you run "cmake"
and e.g. tweak the mergetools sources we'll need to re-copy them, blah
blah blah.
But we can just ... not copy it at all, and have the test-lib.sh learn
that some stuff it needs from whatever the "build dir" is, but other
stuff is *always* relative to the root of the project.
So, for most of that & the context it simply goes away, we don't need to
copy po/is.po, the tests can just know where to find it, ditto
mergetools/tkdiff (which is not generated), and the bash completion.
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
2022-10-18 13:53 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
4 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In the interactive `add` operation, users can choose to jump to specific
hunks, and Git will present the hunk list in that case. To avoid showing
too many lines at once, only a maximum of 21 hunks are shown, skipping
the "mode change" pseudo hunk.
The comparison performed to skip the "mode change" pseudo hunk (if any)
compares a signed integer `i` to the unsigned value `mode_change` (which
can be 0 or 1 because it is a 1-bit type).
According to section 6.3.1.8 of the C99 standard (see e.g.
https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
happen is an automatic conversion of the "lesser" type to the "greater"
type, but since the types differ in signedness, it is ill-defined what
is the correct "usual arithmetic conversion".
Which means that Visual C's behavior can (and does) differ from GCC's:
When compiling Git using the latter, `add -p`'s `goto` command shows no
hunks by default because it casts a negative start offset to a pretty
large unsigned value, breaking the "goto hunk" test case in
`t3701-add-interactive.sh`.
Let's avoid that by converting the unsigned bit explicitly to a signed
integer.
Note: This is a long-standing bug in the Visual C build of Git, but it
has never been caught because t3701 is skipped when `NO_PERL` is set,
which is the case in the `vs-test` jobs of Git's CI runs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
add-patch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/add-patch.c b/add-patch.c
index 509ca04456b..3524555e2b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1547,7 +1547,7 @@ soft_increment:
strbuf_remove(&s->answer, 0, 1);
strbuf_trim(&s->answer);
i = hunk_index - DISPLAY_HUNKS_LINES / 2;
- if (i < file_diff->mode_change)
+ if (i < (int)file_diff->mode_change)
i = file_diff->mode_change;
while (s->answer.len == 0) {
i = display_hunks(s, file_diff, i);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
@ 2022-10-18 13:53 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-18 13:53 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Victoria Dye, Eric Sunshine,
Johannes Schindelin
On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the interactive `add` operation, users can choose to jump to specific
> hunks, and Git will present the hunk list in that case. To avoid showing
> too many lines at once, only a maximum of 21 hunks are shown, skipping
> the "mode change" pseudo hunk.
>
> The comparison performed to skip the "mode change" pseudo hunk (if any)
> compares a signed integer `i` to the unsigned value `mode_change` (which
> can be 0 or 1 because it is a 1-bit type).
>
> According to section 6.3.1.8 of the C99 standard (see e.g.
> https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), what should
> happen is an automatic conversion of the "lesser" type to the "greater"
> type, but since the types differ in signedness, it is ill-defined what
> is the correct "usual arithmetic conversion".
>
> Which means that Visual C's behavior can (and does) differ from GCC's:
> When compiling Git using the latter, `add -p`'s `goto` command shows no
> hunks by default because it casts a negative start offset to a pretty
> large unsigned value, breaking the "goto hunk" test case in
> `t3701-add-interactive.sh`.
>
> Let's avoid that by converting the unsigned bit explicitly to a signed
> integer.
>
> Note: This is a long-standing bug in the Visual C build of Git, but it
> has never been caught because t3701 is skipped when `NO_PERL` is set,
> which is the case in the `vs-test` jobs of Git's CI runs.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> add-patch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 509ca04456b..3524555e2b0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1547,7 +1547,7 @@ soft_increment:
> strbuf_remove(&s->answer, 0, 1);
> strbuf_trim(&s->answer);
> i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> - if (i < file_diff->mode_change)
> + if (i < (int)file_diff->mode_change)
> i = file_diff->mode_change;
> while (s->answer.len == 0) {
> i = display_hunks(s, file_diff, i);
Junio pointed out in the last round that this really could use a
separate submission, and I tend to agree. It's really nothing to do with
cmake...
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
4 siblings, 1 reply; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 7f5397a07c6c (cmake: support for testing git when building out of the
source tree, 2020-06-26), we implemented support for running Git's test
scripts even after building Git in a different directory than the source
directory.
The way we did this was to edit the file `t/test-lib.sh` to override
`GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
directory.
This is unideal because it always leaves a tracked file marked as
modified, and it is all too easy to commit that change by mistake.
Let's change the strategy by teaching `t/test-lib.sh` to detect the
presence of a file called `GIT-BUILD-DIR` in the source directory. If it
exists, the contents are interpreted as the location to the _actual_
build directory. We then write this file as part of the CTest
definition.
To support building Git via a regular `make` invocation after building
it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
convenience, this is done as part of the Makefile rule that is already
run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
up to date).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 +
Makefile | 1 +
contrib/buildsystems/CMakeLists.txt | 7 +------
t/test-lib.sh | 10 ++++++++++
4 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/.gitignore b/.gitignore
index 73df7295795..89ad7e68b4b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
/fuzz_corpora
+/GIT-BUILD-DIR
/GIT-BUILD-OPTIONS
/GIT-CFLAGS
/GIT-LDFLAGS
diff --git a/Makefile b/Makefile
index 88178c5b466..886614340c7 100644
--- a/Makefile
+++ b/Makefile
@@ -3029,6 +3029,7 @@ else
@echo RUNTIME_PREFIX=\'false\' >>$@+
endif
@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
+ @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
### Detect Python interpreter path changes
ifndef NO_PYTHON
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 0c741e7d878..1d8cebb4cfe 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1067,14 +1067,9 @@ endif()
#Make the tests work when building out of the source tree
get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
- file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
- string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
#Setting the build directory in test-lib.sh before running tests
file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
- "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
- "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
- "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
- "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
+ "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
#misc copies
file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 120f11812c3..dfc0144ed3b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -47,6 +47,16 @@ then
echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
exit 1
fi
+if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
+then
+ GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
+ # On Windows, we must convert Windows paths lest they contain a colon
+ case "$(uname -s)" in
+ *MINGW*)
+ GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
+ ;;
+ esac
+fi
# Prepend a string to a VAR using an arbitrary ":" delimiter, not
# adding the delimiter if VAR or VALUE is empty. I.e. a generalized:
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
@ 2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:21 ` Johannes Schindelin
0 siblings, 1 reply; 58+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-10-18 13:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Phillip Wood, Victoria Dye, Eric Sunshine,
Johannes Schindelin
On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 7f5397a07c6c (cmake: support for testing git when building out of the
> source tree, 2020-06-26), we implemented support for running Git's test
> scripts even after building Git in a different directory than the source
> directory.
>
> The way we did this was to edit the file `t/test-lib.sh` to override
> `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> directory.
>
> This is unideal because it always leaves a tracked file marked as
> modified, and it is all too easy to commit that change by mistake.
>
> Let's change the strategy by teaching `t/test-lib.sh` to detect the
> presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> exists, the contents are interpreted as the location to the _actual_
> build directory. We then write this file as part of the CTest
> definition.
>
> To support building Git via a regular `make` invocation after building
> it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> convenience, this is done as part of the Makefile rule that is already
> run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> up to date).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Re my earlier feedback, I came up with this as an alternative, which
nicely allows us to have "cmake" and "make" play together, you can even
run them concurrently!:
https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
In case that OID changes it's on my
https://github.com/avar/git/commits/avar/cmake-test-path branch,
currently 30f2265fd07 (cmake & test-lib.sh: add a $GIT_SOURCE_DIR
variable, 2022-10-14).
And it...
> diff --git a/Makefile b/Makefile
> index 88178c5b466..886614340c7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3029,6 +3029,7 @@ else
> @echo RUNTIME_PREFIX=\'false\' >>$@+
> endif
> @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> + @if test -f GIT-BUILD-DIR; then rm GIT-BUILD-DIR; fi
...allows us to get rid of this, which you understandably need with your
approach, but which I'd *really* prefer we not have. Let's not sneak
things into make's dependency DAG that it doesn't know about in FORCE'd
shell command (but more on that later).
> ### Detect Python interpreter path changes
> ifndef NO_PYTHON
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 0c741e7d878..1d8cebb4cfe 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -1067,14 +1067,9 @@ endif()
> #Make the tests work when building out of the source tree
> get_filename_component(CACHE_PATH ${CMAKE_CURRENT_LIST_DIR}/../../CMakeCache.txt ABSOLUTE)
> if(NOT ${CMAKE_BINARY_DIR}/CMakeCache.txt STREQUAL ${CACHE_PATH})
> - file(RELATIVE_PATH BUILD_DIR_RELATIVE ${CMAKE_SOURCE_DIR} ${CMAKE_BINARY_DIR}/CMakeCache.txt)
> - string(REPLACE "/CMakeCache.txt" "" BUILD_DIR_RELATIVE ${BUILD_DIR_RELATIVE})
> #Setting the build directory in test-lib.sh before running tests
> file(WRITE ${CMAKE_BINARY_DIR}/CTestCustom.cmake
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh GIT_BUILD_DIR_REPL REGEX \"GIT_BUILD_DIR=(.*)\")\n"
> - "file(STRINGS ${CMAKE_SOURCE_DIR}/t/test-lib.sh content NEWLINE_CONSUME)\n"
> - "string(REPLACE \"\${GIT_BUILD_DIR_REPL}\" \"GIT_BUILD_DIR=\\\"$TEST_DIRECTORY/../${BUILD_DIR_RELATIVE}\\\"\" content \"\${content}\")\n"
> - "file(WRITE ${CMAKE_SOURCE_DIR}/t/test-lib.sh \${content})")
> + "file(WRITE ${CMAKE_SOURCE_DIR}/GIT-BUILD-DIR \"${CMAKE_BINARY_DIR}\")")
> #misc copies
> file(COPY ${CMAKE_SOURCE_DIR}/t/chainlint.sed DESTINATION ${CMAKE_BINARY_DIR}/t/)
> file(COPY ${CMAKE_SOURCE_DIR}/po/is.po DESTINATION ${CMAKE_BINARY_DIR}/po/)
...and this whole section just goes away, we don't need any
cmake-specifi hacking here, and actually it's not cmake-specific at
all. It's just a "GIT_TEST_INSTALLED for things that are built, not
installed". E.g.:
(cd g/git.scratch && make)
(cd g/git && make clean && GIT_TEST_BUILD_DIR="$PWD/../git.scratch" make -C t)
Supporting cmake then just becomes a special-case of test-lib.sh knowing
"hey, my built stuff is at <dir> instead of "../".
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 120f11812c3..dfc0144ed3b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -47,6 +47,16 @@ then
> echo "PANIC: Running in a $TEST_DIRECTORY that doesn't end in '/t'?" >&2
> exit 1
> fi
> +if test -f "$GIT_BUILD_DIR/GIT-BUILD-DIR"
> +then
> + GIT_BUILD_DIR="$(cat "$GIT_BUILD_DIR/GIT-BUILD-DIR")" || exit 1
> + # On Windows, we must convert Windows paths lest they contain a colon
> + case "$(uname -s)" in
> + *MINGW*)
> + GIT_BUILD_DIR="$(cygpath -au "$GIT_BUILD_DIR")"
> + ;;
> + esac
> +fi
...but one thing that I migh thave missed (and would really appreciate
your testing for) is that I didn't invoke cygpath in my version. CI
passes, but since Windows CI doesn't use "ctest" that doesn't tell us
much, and in any case that's Cygwin, no, which we don't run anyway
there?
Anyway, we could run that "cypath" easily in the cmake recipe itself, or
just pass a "hey, please make this canonical" flag to test-lib.sh.
But anyway, one thing that approach explicitly leaves out is that you
want to be able to:
1. Build with cmake
2. cd t
3. Run a test
And have the test itself know to locate and use the cmake binaries
instead of the "main" binaries.
Now, I suspect that we don't actually have cases anyone cares about
where we have *both*, but that's how this code behaves. I.e. a
top-level:
make test
Will wpe that GIT-BUILD-DIR and use the "make" built "git", but e.g.:
make
<build with cmake>
cd t
# At this point I forgot I used cmake earlier
./t0001-init.sh # silently uses cmake...
I can see thy case for auto-discovery, per the IDE case you mentioned,
but isn't it much better to just make this part of the slightly later
part (but we need to set it up here now) part where we discover the
built "git" and:
A. Do we not have it in ../git?
B. Do we have it it contrib/buildsystems/out/git
Then (pseudocode):
if (!A && B)
use_cmake();
else if (A && B)
die("you have both, pick one!");
Or just say that "make" entry points always run with stuff it built, and
"ctest" runs with contrib/buildsystems/out/git, that's explicitly what
you don't want though...
Anyway, to wrap this up, I really wish the interaction between the two
wouldn't have these pitfalls. I get that you want to support it on the
specific Windows IDE case, but can't we more narrowlry do that without:
(cd t && ./t0001-init.sh)
Having this new "pick either one" behavior? Cheers.
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh
2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
@ 2022-10-18 14:21 ` Johannes Schindelin
0 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin @ 2022-10-18 14:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin via GitGitGadget, git, Phillip Wood,
Victoria Dye, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]
Hi Ævar,
you did not even give me a chance to send my reply to your original mail
;-)
On Tue, 18 Oct 2022, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In 7f5397a07c6c (cmake: support for testing git when building out of the
> > source tree, 2020-06-26), we implemented support for running Git's test
> > scripts even after building Git in a different directory than the source
> > directory.
> >
> > The way we did this was to edit the file `t/test-lib.sh` to override
> > `GIT_BUILD_DIR` to point somewhere else than the parent of the `t/`
> > directory.
> >
> > This is unideal because it always leaves a tracked file marked as
> > modified, and it is all too easy to commit that change by mistake.
> >
> > Let's change the strategy by teaching `t/test-lib.sh` to detect the
> > presence of a file called `GIT-BUILD-DIR` in the source directory. If it
> > exists, the contents are interpreted as the location to the _actual_
> > build directory. We then write this file as part of the CTest
> > definition.
> >
> > To support building Git via a regular `make` invocation after building
> > it using CMake, we ensure that the `GIT-BUILD-DIR` file is deleted (for
> > convenience, this is done as part of the Makefile rule that is already
> > run with every `make` invocation to ensure that `GIT-BUILD-OPTIONS` is
> > up to date).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Re my earlier feedback, I came up with this as an alternative, which
> nicely allows us to have "cmake" and "make" play together, you can even
> run them concurrently!:
>
> https://github.com/avar/git/commit/30f2265fd07aee97ea66f6e84a824d85d241e245
This approach _still_ modifies the `test-lib.sh`, which is the entire
reason for the patch under review.
I hope you find an elegant, user-friendly alternative that leaves
`test-lib.sh` unmodified even when building via CMake. I would gladly take
that and drop my `GIT-BUILD-DIR` patch.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3 5/5] cmake: increase time-out for a long-running test
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
@ 2022-10-18 10:59 ` Johannes Schindelin via GitGitGadget
4 siblings, 0 replies; 58+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2022-10-18 10:59 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
Victoria Dye, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
As suggested in
https://github.com/git-for-windows/git/issues/3966#issuecomment-1221264238,
t7112 can run for well over one hour, which seems to be the default
maximum run time at least when running CTest-based tests in Visual
Studio.
Let's increase the time-out as a stop gap to unblock developers wishing
to run Git's test suite in Visual Studio.
Note: The actual run time is highly dependent on the circumstances. For
example, in Git's CI runs, the Windows-based tests typically take a bit
over 5 minutes to run. CI runs have the added benefit that Windows
Defender (the common anti-malware scanner on Windows) is turned off,
something many developers are not at liberty to do on their work
stations. When Defender is turned on, even on this developer's high-end
Ryzen system, t7112 takes over 15 minutes to run.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
contrib/buildsystems/CMakeLists.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 1d8cebb4cfe..7e0d040e0f6 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -1088,4 +1088,8 @@ foreach(tsh ${test_scipts})
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
+# This test script takes an extremely long time and is known to time out even
+# on fast machines because it requires in excess of one hour to run
+set_tests_properties("${CMAKE_SOURCE_DIR}/t/t7112-reset-submodule.sh" PROPERTIES TIMEOUT 4000)
+
endif()#BUILD_TESTING
--
gitgitgadget
^ permalink raw reply related [flat|nested] 58+ messages in thread