Some fixes and an improvement for using CTest on Windows #1320
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dscho
force-pushed
the
ctest-on-windows
branch
from
55121c8 to
c7fc5a4
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") | |||
| #test | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> While at it, enable the command trace via `-x` and verbose output via
> `-v`, otherwise it would be near impossible to diagnose any problems.
This sounds like a completely unrelated change. Do the tests behave
so differently here, compared to how they are run in t/Makefile,
where -vx is not the default?
The only plausible reason I can think of that this change "while at
it" is justifiable as part of the other change is if this is only
ever used in the CI environment and no interactive human would be
sitting in front of the session that runs tests here. If that is
the case, I do agree it would make sense, since it would be much
less convenient to go back to a failing test and rerun it with -v
or -x or whatever. But that needs to be explained in the proposed
log message.
Thanks.
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> 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()There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Wed, 10 Aug 2022, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > While at it, enable the command trace via `-x` and verbose output via
> > `-v`, otherwise it would be near impossible to diagnose any problems.
>
> This sounds like a completely unrelated change.
It may sound like it, but it is not. In order to make sense of the broken
tests, I needed access to more verbose output than our test scripts
provide by default.
When running the test suite on the command-line, it is easy to tell the
user "oh, if you need more information, just call the test script with
these here options: ...".
This is not an option when running the tests within Visual Studio.
Does this clarify the intention and validity of the proposed patch? If so,
I will gladly try my best to improve the commit message to explain that
intention better.
Ciao,
DschoThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <[email protected]> writes:
> Hi Junio,
>
> On Wed, 10 Aug 2022, Junio C Hamano wrote:
>
>> "Johannes Schindelin via GitGitGadget" <[email protected]>
>> writes:
>>
>> > While at it, enable the command trace via `-x` and verbose output via
>> > `-v`, otherwise it would be near impossible to diagnose any problems.
>>
>> This sounds like a completely unrelated change.
>
> It may sound like it, but it is not. In order to make sense of the broken
> tests, I needed access to more verbose output than our test scripts
> provide by default.
>
> When running the test suite on the command-line, it is easy to tell the
> user "oh, if you need more information, just call the test script with
> these here options: ...".
>
> This is not an option when running the tests within Visual Studio.
I gave an example of "CI environment it is cumbersome to go back and
run only a single one" in the review you are responding to that may
explain why such a change is needed, and you gave us exactly the
context that was lacking here. The environment does not let users
run the tests as anything but a single monolithic ball of wax.
> Does this clarify the intention and validity of the proposed patch? If so,
> I will gladly try my best to improve the commit message to explain that
> intention better.
It explains why -x -v is needed and needs to be a part of VS+CMake
topic, and it will help readers to have it in the explanation. It
still does not justify why it has to be a part of a step to omit
bin-warppers and skip chain-lint that has nothing to do it, though.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Tue, 16 Aug 2022, Junio C Hamano wrote:
> Johannes Schindelin <[email protected]> writes:
>
> > On Wed, 10 Aug 2022, Junio C Hamano wrote:
> >
> >> "Johannes Schindelin via GitGitGadget" <[email protected]>
> >> writes:
> >>
> >> > While at it, enable the command trace via `-x` and verbose output via
> >> > `-v`, otherwise it would be near impossible to diagnose any problems.
> >>
> >> This sounds like a completely unrelated change.
> >
> > It may sound like it, but it is not. In order to make sense of the broken
> > tests, I needed access to more verbose output than our test scripts
> > provide by default.
> >
> > When running the test suite on the command-line, it is easy to tell the
> > user "oh, if you need more information, just call the test script with
> > these here options: ...".
> >
> > This is not an option when running the tests within Visual Studio.
>
> I gave an example of "CI environment it is cumbersome to go back and
> run only a single one" in the review you are responding to that may
> explain why such a change is needed, and you gave us exactly the
> context that was lacking here. The environment does not let users
> run the tests as anything but a single monolithic ball of wax.
Good, I will amend the commit message.
> > Does this clarify the intention and validity of the proposed patch? If so,
> > I will gladly try my best to improve the commit message to explain that
> > intention better.
>
> It explains why -x -v is needed and needs to be a part of VS+CMake
> topic, and it will help readers to have it in the explanation. It
> still does not justify why it has to be a part of a step to omit
> bin-warppers and skip chain-lint that has nothing to do it, though.
Oh, that ;-)
To be honest, I reflexively do that on Windows because it saves _so much
time_.
I will amend the commit message to clarify that, too.
Ciao,
Dscho
| @@ -1547,7 +1547,7 @@ static int patch_update_file(struct add_p_state *s, | |||
| strbuf_remove(&s->answer, 0, 1); | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
This looks more like a fix to a general problem, not limited to
windows or cmake, that we had since 9254bdfb (built-in add -p:
implement the 'g' ("goto") command, 2019-12-13).
Please pull this out of the series and let's have it reviewed
separately.
Thanks.
> 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);There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Wed, 10 Aug 2022, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
>
> This looks more like a fix to a general problem, not limited to
> windows or cmake, that we had since 9254bdfb (built-in add -p:
> implement the 'g' ("goto") command, 2019-12-13).
>
> Please pull this out of the series and let's have it reviewed
> separately.
The scope of this patch series is to fix running the tests in Visual
Studio when building using CMake.
Pulling out this patch would break that patch series because it would
leave that breakage in place.
Except if you are asking to put this patch series on the back burner and
prioritize the patch that fixes an ambiguous implicit cast between signed
and unsigned data types?
However, that would mean that I'd now have to address all of those
implicit casts, which is unfortunately a larger amount of work than I can
justify to take on.
Therefore I move that in this instance, the perfect is the enemy of the
good, and that the patch should remain within this patch series, even if
the larger-scoped project to avoid any implicit signed/unsigned casts
remains unaddressed.
BTW I would have expected your review to ask the (in hindsight, obvious)
question why the test suite still passes even with `vs-test` exercising
the code that is compiled using Visual C?
The answer to that would have been that the `vs-test` job of our CI runs
defines `NO_PERL`, and t3701 is skipped completely if that is the case.
Ciao,
DschoThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <[email protected]> writes:
> The scope of this patch series is to fix running the tests in Visual
> Studio when building using CMake.
That's only the perspective of who cares about VS+CMake. From my
point of view who wants a healthy Git overall, the priority is
different. "add -p" fix is wider than the context of VS, and I do
not discount the need that we need to fix it before we can call
VS+CMake issue fixed (and I do not mean to say it is unnecessary to
fix VS+CMake). It's just this one can proceed with help by those
who do not care about or have no environment to help with VS+CMake
because it is more generic, and I do not think you mind to make the
rest of the narrower VS+CMake topic _depend_ on it.
> Pulling out this patch would break that patch series because it would
> leave that breakage in place.
We deal with topic that depends on another topic all the time, and I
do not think there is anything that makes VS+CMake topic so special
that it cannot be dependent on another topic. It's just the matter
of splitting this out and make it [1/1], and make the remainder to
[1-4/4] and mark them rely on add-p fix when you send the latter
out, isn't it? Puzzled...
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Junio,
On Tue, 16 Aug 2022, Junio C Hamano wrote:
> Johannes Schindelin <[email protected]> writes:
>
> > The scope of this patch series is to fix running the tests in Visual
> > Studio when building using CMake.
>
> That's only the perspective of who cares about VS+CMake. From my
> point of view who wants a healthy Git overall, the priority is
> different. "add -p" fix is wider than the context of VS, and I do
> not discount the need that we need to fix it before we can call
> VS+CMake issue fixed (and I do not mean to say it is unnecessary to
> fix VS+CMake). It's just this one can proceed with help by those
> who do not care about or have no environment to help with VS+CMake
> because it is more generic, and I do not think you mind to make the
> rest of the narrower VS+CMake topic _depend_ on it.
Sure.
But if I pull out that patch into its code contribution, all of a sudden
the scope of _that_ contribution is to address an implicit signed/unsigned
cast. What other purpose could it have? And if we're addressing that
signed/unsigned cast, we cannot just fix this single one. We need to fix
them all, no?
So let's have a look at that project, since you are implicitly
volunteering me for it. We do have this in our `config.mak.dev`:
# These are disabled because we have these all over the place.
DEVELOPER_CFLAGS += -Wno-empty-body
DEVELOPER_CFLAGS += -Wno-missing-field-initializers
DEVELOPER_CFLAGS += -Wno-sign-compare
DEVELOPER_CFLAGS += -Wno-unused-parameter
endif
Note the `-Wno-sign-compare` part. If I comment that out, I get these
reports:
-- snip --
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
add-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
add-interactive.c:379:25: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:389:11: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
[... 1260 more issues ...]
-- snap --
You see how that increases the amount of work you are asking me to do?
The worst part is that gcc (at least of version 9.4.0 which I have
available in my Ubuntu) does not even catch the line that is fixed by the
patch we are trying to discuss here. It catches 10 issues in
`add-patch.c`, but not the `i < file_diff->mode_change` one.
Neither does Visual C 2022, nor the gcc I have in Git for Windows' SDK,
which is v12.1.0.
So I would first need to find a tool that identifies all code locations
that compare signed with unsigned values.
And that's even before analyzing carefully how to address them (not all
instances will be as easy as upcasting from an unsigned bit to `int`, some
of those instances are about `size_t` vs `ssize_t`).
> > Pulling out this patch would break that patch series because it would
> > leave that breakage in place.
>
> We deal with topic that depends on another topic all the time, and I
> do not think there is anything that makes VS+CMake topic so special
> that it cannot be dependent on another topic. It's just the matter
> of splitting this out and make it [1/1], and make the remainder to
> [1-4/4] and mark them rely on add-p fix when you send the latter
> out, isn't it? Puzzled...
Sure, if you think that the signed/unsigned comparison project is more
important than fixing running Git's test suite inside Visual Studio, or
worse: if you are asking me to do a less than thorough job on those
signed/unsigned fixes by fixing only a single instance and leaving all
others unaddressed in a patch series that has nothing to do with Visual
Studio, then I understand how my stance could confuse you.
But my intention with this patch series is to fix running Git's test suite
in Visual Studio. And my intention is to provide a complete solution in my
patch series, no half measures.
As such, I would be loathe to have my authorship on a single patch that
solves neither the Visual Studio/CTest problem nor the vast majority of
the signed/unsigned problems. It would be incomplete in any way you look
at it. I would consider such a contribution lackluster, sloppy and
definitely not up to my standards.
Ciao,
Dscho
| @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh") | |||
| #test | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
>
> 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.
>
> Let's do the same when running the tests, say, in Visual Studio.
>
> While at it, enable the command trace via `-x` and verbose output via
> `-v`, otherwise it would be near impossible to diagnose any problems.
>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> 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()
Is this authored before a561962479c (cmake: fix CMakeLists.txt on Linux,
2022-05-24) was merged?
The "say, in Visual Studio" seems to elide that we'll now run these
tests differently when you run with cmake everywhere.
It seems much better to pass some "test arguments" to cmake itself,
which we'd then call from the ci specifically. Then e.g. a Linux user of
cmake wouldn't wonder why the "make test" spots e.g. a chain-lint issue
that the cmake testing would hide.
|
User |
t/test-lib-functions.sh
Outdated
| @@ -492,7 +492,10 @@ test_commit_bulk () { | |||
| # of a file in the working directory and add it to the index. | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> [...]
> However, this quirk is only in effect as long as `chmod` is run inside
> the pseudo Unix root directory structure or within the home directory.
> When run outside, such invocations fail like this:
>
> chmod: changing permissions of '<file>': Invalid argument
..ok, but...
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 6da7273f1d5..7c63b22acab 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -492,7 +492,10 @@ test_commit_bulk () {
> # of a file in the working directory and add it to the index.
>
> test_chmod () {
> - chmod "$@" &&
> + if test_have_prereq !MINGW
> + then
> + chmod "$@"
> + fi &&
> git update-index --add "--chmod=$@"
> }
>
> @@ -548,7 +551,10 @@ write_script () {
> echo "#!${2-"$SHELL_PATH"}" &&
> cat
> } >"$1" &&
> - chmod +x "$1"
> + if test_have_prereq !MINGW
> + then
> + chmod +x "$1"
> + fi
... you get +x semantics by default, so we didn't need that "chmod +x"
in the first place?
The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
don't care about that, regardless of the "pseudo Unix root directory"?
What if we get a "test_chmod -o <file>", won't this silently do the
wrong thing?
If so isn't something in this direction (untested) a more targeted &
obvious fix?:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 10258def7be..1c3b6692388 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1690,6 +1690,16 @@ case $uname_s in
find () {
/usr/bin/find "$@"
}
+ chmod () {
+ case "$1" in
+ +x|-x)
+ return;
+ ;;
+ *)
+ ;;
+ esac &&
+ /usr/bin/chmod "$@"
+ }
# git sees Windows-style pwd
pwd () {
builtin pwd -WThere was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Ævar,
On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <[email protected]>
> > [...]
> > However, this quirk is only in effect as long as `chmod` is run inside
> > the pseudo Unix root directory structure or within the home directory.
> > When run outside, such invocations fail like this:
> >
> > chmod: changing permissions of '<file>': Invalid argument
>
> ..ok, but...
>
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 6da7273f1d5..7c63b22acab 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -492,7 +492,10 @@ test_commit_bulk () {
> > # of a file in the working directory and add it to the index.
> >
> > test_chmod () {
> > - chmod "$@" &&
> > + if test_have_prereq !MINGW
> > + then
> > + chmod "$@"
> > + fi &&
> > git update-index --add "--chmod=$@"
> > }
> >
> > @@ -548,7 +551,10 @@ write_script () {
> > echo "#!${2-"$SHELL_PATH"}" &&
> > cat
> > } >"$1" &&
> > - chmod +x "$1"
> > + if test_have_prereq !MINGW
> > + then
> > + chmod +x "$1"
> > + fi
>
> ... you get +x semantics by default, so we didn't need that "chmod +x"
> in the first place?
No. We do not get that `chmod +x` semantics by default. Those `chmod +x`
statements are treated as (expensive) no-ops by default. This is what I
meant when I said this in the commit message (that is missing from the
quoted text above):
[...] it pretends that it succeeded, when in reality it did not
do a thing [...]
I do not know how to say this more clearly.
> The rest of "test_chmod" seems to *happen to* pass +x or -x, but we
> don't care about that, regardless of the "pseudo Unix root directory"?
The rest of "test_chmod" is even quoted above, so we do not need to leave
anybody guessing as to what it does:
git update-index --add "--chmod=$@"
This asks Git to update the index with the executable bit explicitly
turned on or off, regardless of the information that is available on disk.
And yes, Git does what we expect it to do here.
> What if we get a "test_chmod -o <file>", won't this silently do the
> wrong thing?
But we don't?
The code under discussion is Git's test suite, after all, not something
that every Git user is expected to use in more ways than the core Git
developers can imagine.
So even a cursory `git grep test_chmod upstream/seen` could have shown
that there is no such user, and if this was supposed to be a "But what if
we do that at some stage in the future?" feedback, said feedback could be
construed as to intentionally use up valuable contributor time.
> If so isn't something in this direction (untested) a more targeted &
> obvious fix?:
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 10258def7be..1c3b6692388 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1690,6 +1690,16 @@ case $uname_s in
> find () {
> /usr/bin/find "$@"
> }
> + chmod () {
> + case "$1" in
> + +x|-x)
> + return;
> + ;;
> + *)
> + ;;
> + esac &&
> + /usr/bin/chmod "$@"
> + }
> # git sees Windows-style pwd
> pwd () {
> builtin pwd -W
>
>
In that form, I will reject this suggestion as over-engineered and
convoluted. Why is the `*)` clause empty? Why is there an early return
guarding a single statement? Why is the `/usr/bin/chmod` call not in the
`*)` clause to begin with? Why does this code take pains to handle cases
other than `-x` and `+x` when we do not have any callers, and even in the
experimental patches, there are no such users in sight? I would like to
encourage you to address such issues during review before even sending the
mail in the future.
Having said that, there is a nugget in this feedback that I find valuable.
Instead of wasting the run time (even on non-Windows platforms!) to
determine whether the `MINGW` prereq is set in order to skip the `chmod`
call or not, we can make `chmod` a no-op explicitly in that `case
$uname_s` block.
I will make it so.
Ciao,
JohannesThere was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Ævar,
On Mon, 22 Aug 2022, Johannes Schindelin wrote:
> Instead of wasting the run time (even on non-Windows platforms!) to
> determine whether the `MINGW` prereq is set in order to skip the `chmod`
> call or not, we can make `chmod` a no-op explicitly in that `case
> $uname_s` block.
>
> I will make it so.
tl;dr this patch needs to be dropped, without the suggested replacement.
Gaaah!
After struggling with this for much longer than I care to admit, and even
debugging inside the MSYS2 runtime (which is a level boss if there ever
was one), I found out that all about this patch was wrong.
The suggested patch (making `chmod` a no-op) was wrong: the test suite
started failing left and right. Why? Because `chmod` is not a _complete_
no-op. The test suite does not use the `-o` flag you suggested (which
would not make sense in Git's test suite, whether or not you remove file
permissions for "others" than the current user or group, Git behaves
identically), but it _does_ use the `-w`/`+w` flags, and those _are_
respected, even on Windows.
The way the write permission bits are translated to ACLs is admittedly
somewhat magical or all kinds of wrong, too, depending on your point of
view.
But my original patch was also wrong. Why? Because it claimed that `chmod
+x` does not work outside of MSYS2's pseudo Unix root directory tree. And
that's simply not true.
Whether you call `chmod +x C:/Users/avar/my-file.txt` or `chmod +x
/tmp/avar.sh`, it "succeeds" (by silently ignoring the flag that is
inapplicable on Windows, whether or not a file is executable is determined
by its file extension, and yes, that means that shell scripts are never
executable and we have to live with a nasty hack in Git to pretend that
they are, based on their contents starting with a hash-bang line).
So why did I claim that the `chmod +x` invocation does not work outside of
that pseudo Unix root directory tree? Because it actually did not work, at
least for me! But it worked on the build agents. Why? Because I have a
command in my `~/.profile` that mounts several of my Git for Windows SDKs
as "short-cuts" like `/sdk64`, `/sdk32` and the likes (think of these as
bind-mounts). And those `mount` commands did not specify the `noacl` flag.
So what's that `noacl` flag? It is magic, I can tell you. It basically
makes all this pretense work where MSYS2 pretends that we are in a Unix
environment with Unix permissions when we're not actually. The details are
too gnarly and involved to explain, I won't write them up here in order to
avoid putting even more readers to sleep than I must have done already.
So my analysis was based on observation instead of inspection, cutting
corners, and I dearly regret that now. By default, MSYS2 "mounts" the
system drives as `/c`, `/d`, etc with that `noacl` flag, the same as it
mounts the pseudo Unix root as `/`. And that is why the Git test suite
does not throw up every time `chmod +x` is called. And it was a simple
pilot error on my part that caused it to fail on my system, and I did not
realize that the problem was confined to my system, and the bug was in my
`~/.profile` and not in Git's code base.
Working on Git for Windows is never boring.
Ciao,
Johannes
| @@ -2,6 +2,7 @@ | |||
| /fuzz_corpora | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
>
> 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 <[email protected]>
> ---
> .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
So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct
and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests
run inside a "t" directory generated in contrib/buildsystems/ ?
This really seems like a hack-upon-hack, and will break some workflows
when you switch back & forth, e.g.:
make
# run cmake
make -C t
Will now pick up this new file, and result in broken tests, as we'll
rely on the now-stale file.
IOW you're making the assumption that by piggy-backing on the
GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run
it*, but that's not the case.
I can think of ways to get around it, but it would be nasty as we'd need
to complete the dependency graph between the two, and figure out the
various interactions.
But why do we need this at all? Right now I tried:
* (Manually) removing the existing hack
* Copying t/*lib*.sh over to contrib/buildsystems/t
* cd contrib/buildsystems/t
* ../../../t/t0001-init.sh
Which (aside from a small fixable nit about oid-info) Just Works,
because the cmake build is already creating a GIT-BUILD-OPTIONS.
Presumably that "copying" step should be simlynking, or we'd be smarter
about doing includes from test-lib.sh.
But isn't that a much better approach? Rather than working around the
directory discovery just teach it to run from the generated t/ directory
directly?There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Ævar,
On Thu, 11 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Aug 10 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > .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
>
> So we'll now skip the assertion added in 9dbf20e7f62 (test-lib: correct
> and assert TEST_DIRECTORY overriding, 2022-02-27), shouldn't these tests
> run inside a "t" directory generated in contrib/buildsystems/ ?
>
> This really seems like a hack-upon-hack, and will break some workflows
> when you switch back & forth, e.g.:
>
> make
> # run cmake
> make -C t
>
> Will now pick up this new file, and result in broken tests, as we'll
> rely on the now-stale file.
>
> IOW you're making the assumption that by piggy-backing on the
> GIT-BUILD-OPTIONS rule that anything that needs it will also *re-run
> it*, but that's not the case.
>
> I can think of ways to get around it, but it would be nasty as we'd need
> to complete the dependency graph between the two, and figure out the
> various interactions.
>
> But why do we need this at all? Right now I tried:
>
> * (Manually) removing the existing hack
> * Copying t/*lib*.sh over to contrib/buildsystems/t
> * cd contrib/buildsystems/t
> * ../../../t/t0001-init.sh
I appreciate that you want to make it easier for developers. From my
experience with (Windows) developers, I have to point out that what you
describe is _not_ easier than `cd t && ./t0001-init.sh`, though.
It might be easier from some point of view that seems vaguely familiar,
but the user experience of this approach looks prohibitively bad to me.
> Which (aside from a small fixable nit about oid-info) Just Works,
> because the cmake build is already creating a GIT-BUILD-OPTIONS.
>
> Presumably that "copying" step should be simlynking, or we'd be smarter
> about doing includes from test-lib.sh.
>
> But isn't that a much better approach? Rather than working around the
> directory discovery just teach it to run from the generated t/ directory
> directly?
I am sure I could poke more holes into that approach, probably whipping
out something something about `contrib/`, but the user experience alone
(so, dear developer, contrary to what you read in the README and in other
documentation flying all over the internet, yes, you need to switch to
`contrib/buildsystems`, no wait, there's a typo, there's no dash between
`build` and `systems`, there you go, why `buildsystems`? Well, it's just
the way it is. Yes. Now, type out, no, no, wait, *wait!*, you cannot run
`t0006-date.sh` directly as documented, no, wait, you need to prefix it
with this here, and now carefully count the dots, yep, that's right, six
dots in total, three slashes, then a `t/`, yes, I know you were already in
`t/` but not _that_ `t/`, there are now two of 'em, yes, it is
unintuitive, you're absolutely correct, but you know...) is enough for me
to not want to consider this approach any further.
Ciao,
Johannes
| @@ -1547,7 +1547,7 @@ static int patch_update_file(struct add_p_state *s, | |||
| strbuf_remove(&s->answer, 0, 1); | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> > 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.
Oh the joys of bit-fields!
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> 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);
Unrelated to this change but why don't we just call strbuf_reset() here?
> i = hunk_index - DISPLAY_HUNKS_LINES / 2;
> - if (i < file_diff->mode_change)
> + if (i < (int)file_diff->mode_change)
The change itself looks good
Best Wishes
Phillip
> i = file_diff->mode_change;
> while (s->answer.len == 0) {
> i = display_hunks(s, file_diff, i);There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Thu, 11 Aug 2022, Phillip Wood wrote:
> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>
> > 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);
>
> Unrelated to this change but why don't we just call strbuf_reset() here?
This part of the code is used when the `g` command (to "go to hunk") was
issued by the user. And that `g` command allows for a number to be
specified, e.g. `g1` to go to the first hunk.
The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.
The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
the number, e.g. `g 1` should also go to the first hunk.
If we called `strbuf_reset()` here, we would remove the number completely.
Ciao,
DschoThere was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
On 16/08/2022 11:00, Johannes Schindelin wrote:
> Hi Phillip,
> > On Thu, 11 Aug 2022, Phillip Wood wrote:
> >> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>>
>>> 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);
>>
>> Unrelated to this change but why don't we just call strbuf_reset() here?
> > This part of the code is used when the `g` command (to "go to hunk") was
> issued by the user. And that `g` command allows for a number to be
> specified, e.g. `g1` to go to the first hunk.
> > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.
> > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
> the number, e.g. `g 1` should also go to the first hunk.
> > If we called `strbuf_reset()` here, we would remove the number completely.
Oh so if the user is not using interactive.singleKey then they can skip the prompt which displays the hunks and asks which one they want to jump to by guessing the number of the hunk they want and typing "g <n>".
Thanks
Phillip
> Ciao,
> DschoThere was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Tue, 16 Aug 2022, Phillip Wood wrote:
> On 16/08/2022 11:00, Johannes Schindelin wrote:
>
> > On Thu, 11 Aug 2022, Phillip Wood wrote:
> >
> > > On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> > >
> > > > 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);
> > >
> > > Unrelated to this change but why don't we just call strbuf_reset() here?
> >
> > This part of the code is used when the `g` command (to "go to hunk") was
> > issued by the user. And that `g` command allows for a number to be
> > specified, e.g. `g1` to go to the first hunk.
> >
> > The `strbuf_remove(&s->answer, 0, 1)` removes the `g` from the command.
> >
> > The `strbuf_trim(&s->answer)` allows for whitespace between the `g` and
> > the number, e.g. `g 1` should also go to the first hunk.
> >
> > If we called `strbuf_reset()` here, we would remove the number completely.
>
> Oh so if the user is not using interactive.singleKey then they can skip the
> prompt which displays the hunks and asks which one they want to jump to by
> guessing the number of the hunk they want and typing "g <n>".
Yes, precisely.
I forgot that you are using `interactive.singleKey` (I planned on opting
into that mode, but for some reason I never get around to flip the
switch). The default mode is not the single key mode, though.
Ciao,
Dscho
|
User |
| @@ -2,6 +2,7 @@ | |||
| /fuzz_corpora | |||
There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> > 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.
I think it is really good to get away from editing the test files, but one of the nice things about CMake's out of tree builds is that you can have several build directories with different build configurations and this change does not support that. Could we pass the build directory to the test scripts as a commandline option or environment variable instead? e.g.
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
- COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
+ COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint --build-dir=${CMAKE_BINARY_DIR} -vx
WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
endforeach()
Doing that would avoid changing the main Makefile to remove a file which almost certainly does not exist every time make is invoked as well.
Best Wishes
Phillip
> 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 <[email protected]>
> ---
> .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 1There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Phillip,
On Thu, 11 Aug 2022, Phillip Wood wrote:
> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[email protected]>
> >
> > 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.
>
> I think it is really good to get away from editing the test files, but one of
> the nice things about CMake's out of tree builds is that you can have several
> build directories with different build configurations and this change does not
> support that. Could we pass the build directory to the test scripts as a
> commandline option or environment variable instead? e.g.
>
> foreach(tsh ${test_scipts})
> add_test(NAME ${tsh}
> - COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
> + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint
> --build-dir=${CMAKE_BINARY_DIR} -vx
>
> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> endforeach()
>
> Doing that would avoid changing the main Makefile to remove a file which
> almost certainly does not exist every time make is invoked as well.
This is indeed tempting, but one of the things I recommend to Visual
Studio users for ages already is to run the tests in a Git Bash, i.e.
outside of Visual Studio, to allow for passing `--run=...` options and the
likes. This recommendation predates support for CTest, naturally, but it
is still valid.
And that recommended way of running tests would be broken by the suggested
change because the tests would no longer run except when using CTest.
Besides, while the semantics look tempting, the implementation details do
not. The reason is that we use `GIT_BUILD_DIR` to source the
`GIT-BUILD-OPTIONS` and to validate that Git was built, long before we
parse the command-line in `t/test-lib.sh`. I want to refrain from the
prerequisite extensive refactoring required to support this, at least in
the context of this here patch series.
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)?
Ciao,
DschoThere was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
Hi Dscho
On 16/08/2022 11:09, Johannes Schindelin wrote:
> Hi Phillip,
> > On Thu, 11 Aug 2022, Phillip Wood wrote:
> >> On 10/08/2022 16:02, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <[email protected]>
>>>
>>> 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.
>>
>> I think it is really good to get away from editing the test files, but one of
>> the nice things about CMake's out of tree builds is that you can have several
>> build directories with different build configurations and this change does not
>> support that. Could we pass the build directory to the test scripts as a
>> commandline option or environment variable instead? e.g.
>>
>> foreach(tsh ${test_scipts})
>> add_test(NAME ${tsh}
>> - COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>> + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint
>> --build-dir=${CMAKE_BINARY_DIR} -vx
>>
>> WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
>> endforeach()
>>
>> Doing that would avoid changing the main Makefile to remove a file which
>> almost certainly does not exist every time make is invoked as well.
> > This is indeed tempting, but one of the things I recommend to Visual
> Studio users for ages already is to run the tests in a Git Bash, i.e.
> outside of Visual Studio, to allow for passing `--run=...` options and the
> likes. This recommendation predates support for CTest, naturally, but it
> is still valid.
> > And that recommended way of running tests would be broken by the suggested
> change because the tests would no longer run except when using CTest.
Yes the recommendation would have to include adding --build-dir which would be a pain
> Besides, while the semantics look tempting, the implementation details do
> not. The reason is that we use `GIT_BUILD_DIR` to source the
> `GIT-BUILD-OPTIONS` and to validate that Git was built, long before we
> parse the command-line in `t/test-lib.sh`. I want to refrain from the
> prerequisite extensive refactoring required to support this, at least in
> the context of this here patch series.
That sounds awkward.
> 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)?
It is definitely an improvement for the CMake side, I'm not sure it is an improvement for the Makefile but it sounds like the best option overall.
Best Wishes
Phillip
> Ciao,
> Dscho
dscho
force-pushed
the
ctest-on-windows
branch
3 times, most recently
from
d66cacf to
6aaa675
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@e70d8db. |
|
This patch series was integrated into seen via git@2e5daf1. |
|
This patch series was integrated into seen via git@44650c1. |
|
There was a status update in the "New Topics" section about the branch Update to build procedure with VS using CMake/CTest. Needs review. source: <[email protected]> |
|
This patch series was integrated into seen via git@a602ffc. |
|
This patch series was integrated into seen via git@071da60. |
|
There was a status update in the "Cooking" section about the branch Update to build procedure with VS using CMake/CTest. Needs review. source: <[email protected]> |
|
This patch series was integrated into seen via git@84b8dff. |
|
This patch series was integrated into seen via git@7d68c2a. |
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 <[email protected]>
dscho
force-pushed
the
ctest-on-windows
branch
from
6aaa675 to
d771e45
Compare
In 7f5397a (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 <[email protected]>
As suggested in git-for-windows#3966 (comment), 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 <[email protected]>
dscho
force-pushed
the
ctest-on-windows
branch
from
d771e45 to
40cf872
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
| @@ -1,4 +1,5 @@ | |||
| /fuzz_corpora | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Tue, Oct 18 2022, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
>
> 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 <[email protected]>
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.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
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 <[email protected]>
> >
> > 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 <[email protected]>
>
> 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
|
This patch series was integrated into seen via git@3598ccc. |
|
This patch series was integrated into seen via git@d69c69b. |
|
There was a status update in the "Cooking" section about the branch Update to build procedure with VS using CMake/CTest. Will merge to 'next'? source: <[email protected]> |
|
There was a status update in the "Cooking" section about the branch Update to build procedure with VS using CMake/CTest. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@3e09e68. |
|
This patch series was integrated into seen via git@a9567b9. |
|
This patch series was integrated into seen via git@77cc9fd. |
|
This patch series was integrated into next via git@012ec67. |
|
This patch series was integrated into seen via git@f711657. |
|
This patch series was integrated into seen via git@e2cb4e8. |
|
This patch series was integrated into seen via git@b9ab439. |
|
This patch series was integrated into seen via git@10e7aeb. |
|
There was a status update in the "Cooking" section about the branch Update to build procedure with VS using CMake/CTest. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@246eedf. |
|
This patch series was integrated into master via git@246eedf. |
|
This patch series was integrated into next via git@246eedf. |
|
Closed via 246eedf. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Visual Studio users enjoy support for running the test suite via CTest, thanks to Git's CMake definition.
In git-for-windows#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-testsjobs actually useproveto 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 -pthat 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-compareoption (which totally misses the instance that is fixed in this here patch series).Changes since v2:
ac/fuzzers, to avoid merge conflicts in.gitignore.PANICcheck intest-lib.shso that it is not skipped when the fileGIT-BUILD-DIRexists.Changes since v1:
--no-bin-wrappersand--no-chain-lintwhen running on Windows.add -pbug has not been caught earlier.test_chmodwas determined to be not only faulty, but unneeded, and was dropped.cc: Ævar Arnfjörð Bjarmason [email protected]
cc: Phillip Wood [email protected]
cc: Victoria Dye [email protected]
cc: Eric Sunshine [email protected]