Re: [ANNOUNCE] Git v2.16.0-rc1
Johannes Schindelin writes: > No, the only thing that changed was the introduction of Git::Packet (and > t0021/*.perl uses that). And that Perl module is not yet installed. Ahh, that is the difference among other users of split(/:/, $ENV{GITPERLLIB}). Scripts other than 0021 may be using installed ones and using the wrong separator does not affect them. > Granted, we tested incorrect versions of those modules, but this late in > the -rc phase, I would prefer to be cautious. People might be relying on > the current bug. People meaning those who run "make test"? In any case, the patch to 0021 as posted sounds like the most conservative thing to do at this point, even though we would definitely want to revisit it and clean it up after the release. As long as the reason why only 0021 wants the special case while others are OK is understood, I am OK with the patch ;-) Thanks.
Re: [git-for-windows] Re: [ANNOUNCE] Git v2.16.0-rc1
Am 10.01.2018 um 18:37 schrieb Johannes Schindelin: On Tue, 9 Jan 2018, Junio C Hamano wrote: Johannes Schindelin writes: diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl index f1678851de9..470107248eb 100644 --- a/t/t0021/rot13-filter.pl +++ b/t/t0021/rot13-filter.pl @@ -31,7 +31,22 @@ # use 5.008; -use lib (split(/:/, $ENV{GITPERLLIB})); +sub gitperllib { +... + if ($ENV{GITPERLLIB} =~ /;/) { + return split(/;/, $ENV{GITPERLLIB}); + } + return split(/:/, $ENV{GITPERLLIB}); +} This cannot be the whole story for a few reasons. - In t/test-lib.sh we see this: GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git export GITPERLLIB If this part wants to split with ';', then the joining needs to be done with ';' to match, no? No. It is a lot more complicated than that. As you know, on Linux there is this implicit assumption that path lists are colon-separated. As a consequence, Cygwin does the same (because it would be too hard to port all those Linux/Unix projects to stop assuming colon-separated path lists, right?) This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for Windows, too). Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence the MSYS2 runtime knows that it has to convert the path lists to Windows paths separated by semicolons. The next thing happening in our case is that the Perl script is called from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl interpreter) does *not* convert those path lists back to Unix-like paths separated by colons. But this is a bug in MSYS2, isn't it? The MSYS2 runtime should detect that it was not invoked by some other MSYS2 process. The MSYS2 startup sequence should assume in this case that the environment is Windows-style and convert to POSIX before it calls into perl's main(). And that's why the Unix shell script can happily construct the colon-separated list, and the Perl script will *still* receive the semicolon-separated version of it. - In addition to t0021, there are similar split with colon in 0202, 9000 and 9700, yet I am getting the feeling that you observed the issue only in0021, to which I do not think of a good explanation why. Here is the good explanation: t0021 relies on a Perl package that is not yet installed. t0202 relies on Git::I18N, of which there is a version installed in Git for Windows' SDK. (I do not bother to slow down the test runs by the Subversion tests, I always skip all of them, that's why t9* does not matter to me.) t0202 and the t9* cases are different because perl is invoked by bash directly (AFAICS), without a non-MSYS2 process between them. There is no difference when the path conversion is omitted in this case by design or due to a bug. -- Hannes
Re: [ANNOUNCE] Git v2.16.0-rc1
Hi Junio, On Tue, 9 Jan 2018, Junio C Hamano wrote: > Junio C Hamano writes: > > > Johannes Schindelin writes: > > > >> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl > >> index f1678851de9..470107248eb 100644 > >> --- a/t/t0021/rot13-filter.pl > >> +++ b/t/t0021/rot13-filter.pl > >> @@ -31,7 +31,22 @@ > >> # > >> > >> use 5.008; > >> -use lib (split(/:/, $ENV{GITPERLLIB})); > >> +sub gitperllib { > >> +... > >> + if ($ENV{GITPERLLIB} =~ /;/) { > >> + return split(/;/, $ENV{GITPERLLIB}); > >> + } > >> + return split(/:/, $ENV{GITPERLLIB}); > >> +} > > > > This cannot be the whole story for a few reasons. > > > > - In t/test-lib.sh we see this: > > > > > > GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git > >export GITPERLLIB > > > >If this part wants to split with ';', then the joining needs to > >be done with ';' to match, no? > > > > - In addition to t0021, there are similar split with colon in 0202, > >9000 and 9700, yet I am getting the feeling that you observed the > >issue only in0021, to which I do not think of a good explanation > >why. > > This somehow vaguely rang a bell, and I dug this thing up from the > archive, [*1*] which ended like so: > > >> In our C code, we have "#define PATH_SEP ';'", and encourage > >> our code to be careful and use it. Is there something > >> similar for Perl scripts, I wonder. > >> > > We probably should find a better solution to allow this to > > work with windows style paths...? I know that python provides > > os.pathsep, but I haven't seen an equivalent for perl yet. > > > > The Env[1] core modules suggests using > > $Config::Config{path_sep}[2].. maybe we should be using this? > > I was testing this recently on the Perl included with Git for > Windows and it returns : for the path separator even though it's > on Windows, so I don't think that would work. The Perl in Git > for Windows seems to want UNIX-style inputs (something Dscho > seemed to allude to in his response earlier.). I'm not sure why > it's that way, but he probably knows. > > Your initial response in this thread made it sound as if -rc1 is the > only thing that changed, but looking at the differences between -rc0 > and -rc1, which does not touch t0021 or any other instances of > "split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible > that perhaps the way Perl is built for GfW has been changed recently > and we can safely and sanely use $Config::Config{path_sep} (contrary > to what was found in late Oct in the message quoted above) now? No, the only thing that changed was the introduction of Git::Packet (and t0021/*.perl uses that). And that Perl module is not yet installed. Granted, we tested incorrect versions of those modules, but this late in the -rc phase, I would prefer to be cautious. People might be relying on the current bug. OTOH I might be way too pessimistic and we should essentially replicate that `sub gitperllib` trick in *all* of our Perl scripts. What do you think? Ciao, Dscho
Re: [ANNOUNCE] Git v2.16.0-rc1
Hi Junio, On Tue, 9 Jan 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl > > index f1678851de9..470107248eb 100644 > > --- a/t/t0021/rot13-filter.pl > > +++ b/t/t0021/rot13-filter.pl > > @@ -31,7 +31,22 @@ > > # > > > > use 5.008; > > -use lib (split(/:/, $ENV{GITPERLLIB})); > > +sub gitperllib { > > +... > > + if ($ENV{GITPERLLIB} =~ /;/) { > > + return split(/;/, $ENV{GITPERLLIB}); > > + } > > + return split(/:/, $ENV{GITPERLLIB}); > > +} > > This cannot be the whole story for a few reasons. > > - In t/test-lib.sh we see this: > > > GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git >export GITPERLLIB > >If this part wants to split with ';', then the joining needs to >be done with ';' to match, no? No. It is a lot more complicated than that. As you know, on Linux there is this implicit assumption that path lists are colon-separated. As a consequence, Cygwin does the same (because it would be too hard to port all those Linux/Unix projects to stop assuming colon-separated path lists, right?) This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for Windows, too). Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence the MSYS2 runtime knows that it has to convert the path lists to Windows paths separated by semicolons. The next thing happening in our case is that the Perl script is called from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl interpreter) does *not* convert those path lists back to Unix-like paths separated by colons. And that's why the Unix shell script can happily construct the colon-separated list, and the Perl script will *still* receive the semicolon-separated version of it. > - In addition to t0021, there are similar split with colon in 0202, >9000 and 9700, yet I am getting the feeling that you observed the >issue only in0021, to which I do not think of a good explanation >why. Here is the good explanation: t0021 relies on a Perl package that is not yet installed. t0202 relies on Git::I18N, of which there is a version installed in Git for Windows' SDK. (I do not bother to slow down the test runs by the Subversion tests, I always skip all of them, that's why t9* does not matter to me.) Granted, that is a bug, and an old one at that: the test suite should test what is in the current revision, not what happens to be installed into the system locations. But this late in the game, I am really uncomfortable with the idea to rush through an equivalent fix to all Perl scripts: who knows what it breaks? Ciao, Dscho
Re: [ANNOUNCE] Git v2.16.0-rc1
Junio C Hamano writes: > Johannes Schindelin writes: > >> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl >> index f1678851de9..470107248eb 100644 >> --- a/t/t0021/rot13-filter.pl >> +++ b/t/t0021/rot13-filter.pl >> @@ -31,7 +31,22 @@ >> # >> >> use 5.008; >> -use lib (split(/:/, $ENV{GITPERLLIB})); >> +sub gitperllib { >> +... >> +if ($ENV{GITPERLLIB} =~ /;/) { >> +return split(/;/, $ENV{GITPERLLIB}); >> +} >> +return split(/:/, $ENV{GITPERLLIB}); >> +} > > This cannot be the whole story for a few reasons. > > - In t/test-lib.sh we see this: > > > GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git >export GITPERLLIB > >If this part wants to split with ';', then the joining needs to >be done with ';' to match, no? > > - In addition to t0021, there are similar split with colon in 0202, >9000 and 9700, yet I am getting the feeling that you observed the >issue only in0021, to which I do not think of a good explanation >why. This somehow vaguely rang a bell, and I dug this thing up from the archive, [*1*] which ended like so: >> In our C code, we have "#define PATH_SEP ';'", and encourage >> our code to be careful and use it. Is there something >> similar for Perl scripts, I wonder. >> > We probably should find a better solution to allow this to > work with windows style paths...? I know that python provides > os.pathsep, but I haven't seen an equivalent for perl yet. > > The Env[1] core modules suggests using > $Config::Config{path_sep}[2].. maybe we should be using this? I was testing this recently on the Perl included with Git for Windows and it returns : for the path separator even though it's on Windows, so I don't think that would work. The Perl in Git for Windows seems to want UNIX-style inputs (something Dscho seemed to allude to in his response earlier.). I'm not sure why it's that way, but he probably knows. Your initial response in this thread made it sound as if -rc1 is the only thing that changed, but looking at the differences between -rc0 and -rc1, which does not touch t0021 or any other instances of "split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible that perhaps the way Perl is built for GfW has been changed recently and we can safely and sanely use $Config::Config{path_sep} (contrary to what was found in late Oct in the message quoted above) now? In any case, I'd prefer this issue to be resolved properly before -rc2; a patch to t0021/rot13-filter.pl alone does not smell like a "proper solution" that is based on the understanding of the root cause (and that is why I spent time digging the list archive). Thanks. [Reference] *1* https://public-inbox.org/git/cagyf7-ejkahgwkn9tro4mfvba9odbwcza9jh0pk6ze6fosk...@mail.gmail.com/
Re: [ANNOUNCE] Git v2.16.0-rc1
Johannes Schindelin writes: > diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl > index f1678851de9..470107248eb 100644 > --- a/t/t0021/rot13-filter.pl > +++ b/t/t0021/rot13-filter.pl > @@ -31,7 +31,22 @@ > # > > use 5.008; > -use lib (split(/:/, $ENV{GITPERLLIB})); > +sub gitperllib { > +... > + if ($ENV{GITPERLLIB} =~ /;/) { > + return split(/;/, $ENV{GITPERLLIB}); > + } > + return split(/:/, $ENV{GITPERLLIB}); > +} This cannot be the whole story for a few reasons. - In t/test-lib.sh we see this: GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git export GITPERLLIB If this part wants to split with ';', then the joining needs to be done with ';' to match, no? - In addition to t0021, there are similar split with colon in 0202, 9000 and 9700, yet I am getting the feeling that you observed the issue only in0021, to which I do not think of a good explanation why.
Re: [ANNOUNCE] Git v2.16.0-rc1
Hi again, On Sat, 6 Jan 2018, Johannes Schindelin wrote: > On Fri, 5 Jan 2018, Junio C Hamano wrote: > > > A release candidate Git v2.16.0-rc1 is now available for testing > > at the usual places. It is comprised of 455 non-merge commits > > since v2.15.0, contributed by 79 people, 23 of which are new faces. > > I rebased Git for Windows' thicket of patch series on top, and I already > got this when running t0021: > > # failed 10 among 26 test(s) > 1..26 I actually was able to figure it out: it was the GITPERLLIB problem I described earlier, where we run (once again) into trouble with Git's expectations that everything behaves like Linux, including colon-separated path lists. The test suite passed, so I kicked off a build which should be available soon at: https://github.com/git-for-windows/git/releases/tag/v2.16.0-rc1.windows.1 The patch to work around the GITPERLLIB issue looks like this: -- snipsnap -- Subject: [PATCH] mingw: handle GITPERLLIB in t0021 in a Windows-compatible way Git's assumption that all path lists are colon-separated is not only wrong on Windows, it is not even an assumption that is compatible with POSIX. In the interest of time, let's not try to fix this properly but simply work around the obvious breakage on Windows, where the MSYS2 Bash used by Git for Windows to interpret the Git's Unix shell scripts will automagically convert path lists in the environment to semicolon-separated lists of Windows paths (with drive letter and the corresponding colon and all that jazz). In other words, we simply look whether there is a semicolon in GITPERLLIB and split by semicolons if found instead of colons. This is not fool-proof, of course, as the path list could consist of a single path. But that is not the case in Git for Windows' test suite, there are always two paths in GITPERLLIB. Signed-off-by: Johannes Schindelin --- t/t0021/rot13-filter.pl | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl index f1678851de9..470107248eb 100644 --- a/t/t0021/rot13-filter.pl +++ b/t/t0021/rot13-filter.pl @@ -31,7 +31,22 @@ # use 5.008; -use lib (split(/:/, $ENV{GITPERLLIB})); +sub gitperllib { + # Git assumes that all path lists are Unix-y colon-separated ones. But + # when the Git for Windows executes the test suite, its MSYS2 Bash + # calls git.exe, and colon-separated path lists are converted into + # Windows-y semicolon-separated lists of *Windows* paths (which + # naturally contain a colon after the drive letter, so splitting by + # colons simply does not cut it). + # + # Detect semicolon-separated path list and handle them appropriately. + + if ($ENV{GITPERLLIB} =~ /;/) { + return split(/;/, $ENV{GITPERLLIB}); + } + return split(/:/, $ENV{GITPERLLIB}); +} +use lib (gitperllib()); use strict; use warnings; use IO::File; -- 2.16.0.rc0.windows.1
Re: [ANNOUNCE] Git v2.16.0-rc1
Hi team, On Fri, 5 Jan 2018, Junio C Hamano wrote: > A release candidate Git v2.16.0-rc1 is now available for testing > at the usual places. It is comprised of 455 non-merge commits > since v2.15.0, contributed by 79 people, 23 of which are new faces. I rebased Git for Windows' thicket of patch series on top, and I already got this when running t0021: # failed 10 among 26 test(s) 1..26 It also seems that the v2.16.0-rc1 *without* Git for Windows' patches on top displays the very same problem: the test suite does *not* pass on Windows. As it is, I am not willing to wreck my weekend, so this will have to wait until Monday (and Git for Windows v2.16.0-rc1 with it). Ciao, Johannes