| Message ID | 20201130044224.12298-1-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | make macOS `git maintenance` test work on Windows | expand |
On Mon, Nov 30 2020, Eric Sunshine wrote: > v2 makes the macOS-specific test in t7900 UID-agnostic, as suggested by > Ævar[2], thus the patch which added `test-tool getuid` has been dropped. LGTM. FWIW I think your v1 is fine too, just meant to comment on the basis of "you could also do it like that". Having a C program call getuid() is fine, so is faking it. If you prefer the latter, cool. As an aside and just because this is fresh in my mind, not really relevant to this patch as-such: I did wonder "why not just call perl -e 'print $<' ?" first. But then found (by reading the Perl source[1], didn't actually test this) that it fakes up UID=0 for everything on Windows. I couldn't find any "is root?" in our tree that relies on Perl's $< in a bad way (the couuple of occurances seem innocuous), we have some "id -u" checks, but those also seem OK if it returned 0 on Windows (what does it return?). Seems the worst we'd do there is unintentionally skip some "skip this as root" tests. Also as another aside (but your patch is fine as it is), my suggestion used Perl+Perl RX but you switched it to sed+BRE. Do we want to avoid "sed -E"? I wondered that for something else the other day, we have this: t/check-non-portable-shell.pl: /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)'; So maybe it means "nothing but -nef, or maybe "don't use -efn". The ERE (-E and -r) options aren't mentioned, and a naïve log search of of "sed -E" and "sed -r" in t/ returns nothing. It's also my impression that just using $("$PERL_PATH" -e ...) is fine, and at least to my reading the Perl RX is more readable with look-behind assertions, but I'm biased by familiarity with it. Our PERL prereq & NO_PERL=YesPlease is just for "this may require a non-ancient Perl" & "don't install Perl for runtime stuff" respectively. Is that not the case and we'd like to avoid new perl invocations where possible? I don't really care either way (or, if your switch in this case was just a personal preference, also fine), but if we are trying to somewhat discourage perl use (and maybe eventually get rid of it entirely) that would be a useful t/README doc update. I know Johannes (CC'd) has (this is from wetware memory) wanted to (understandably) not need to bother with Perl as part of GFW, but I can't remember if that's for a reason covered by NO_PERL=YesPlease, i.e. packaging it up, or whether it's also to not wanting to provide a perl for the test suite. 1. https://github.com/Perl/perl5/blob/v5.33.4/win32/win32.c#L1079-L1092
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Also as another aside (but your patch is fine as it is), my suggestion > used Perl+Perl RX but you switched it to sed+BRE. Do we want to avoid > "sed -E"? I wondered that for something else the other day, we have > this: > > t/check-non-portable-shell.pl: /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)'; > > So maybe it means "nothing but -nef, or maybe "don't use -efn". The ERE > (-E and -r) options aren't mentioned, and a naïve log search of of "sed > -E" and "sed -r" in t/ returns nothing. Correct. We currently do not use "sed -E", and the script says "Let's not use it; it's not even in POSIX" for things other than 'n', 'e', and 'f'. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html Thanks.
On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > LGTM. FWIW I think your v1 is fine too, just meant to comment on the > basis of "you could also do it like that". Having a C program call > getuid() is fine, so is faking it. If you prefer the latter, cool. I do like the simplicity of the latter, and I wasn't super happy about introducing a new test-tool subcommand just to make this one test pass, especially since `test-tool getuid` is so different from most other subcommands which are typically added to give us access to some internal element of Git otherwise not accessible to the test scripts. > I did wonder "why not just call perl -e 'print $<' ?" first. But then > found (by reading the Perl source[1], didn't actually test this) that it > fakes up UID=0 for everything on Windows. I totally forgot about Perl's `$<`. Under the Git for Windows SDK, Perl's `$<` returns a large positive number. I suspect this differs from what you saw in the Perl source code because the Windows-specific code you looked at does not come into play in this case. For Git for Windows SDK, Perl is almost certainly instead built in Unix-like mode, linking against the MSYS2 library for its POSIX-like emulation, thus the Windows-specific Perl goop is not needed. > I couldn't find any "is root?" in our tree that relies on Perl's $< in a > bad way (the couuple of occurances seem innocuous), we have some "id -u" > checks, but those also seem OK if it returned 0 on Windows (what does it > return?). Seems the worst we'd do there is unintentionally skip some > "skip this as root" tests. Under Git for Windows SDK, `id -u` returns the same large positive number as returned by Perl's `$<`, which makes sense since `id` is also likely linked against the MSYS2 library. As for getuid() in Git itself, that always returns 1. I see now that's because 1 is hard-coded in Git's compat/mingw.h override of getuid(). So, an alternative would have been for the test to use `uid=1` on MINGW, but I like the current approach better of having the test be UID-agnostic. > It's also my impression that just using $("$PERL_PATH" -e ...) is fine, > and at least to my reading the Perl RX is more readable with look-behind > assertions, but I'm biased by familiarity with it. The `sed` version seems simpler and more straightforward to me, whereas look-behind feels harder to reason about, but of course it's all subjective and not terribly important. Either would be fine. > Our PERL prereq & NO_PERL=YesPlease is just for "this may require a > non-ancient Perl" & "don't install Perl for runtime stuff" > respectively. Is that not the case and we'd like to avoid new perl > invocations where possible? > > I don't really care either way (or, if your switch in this case was just > a personal preference, also fine), but if we are trying to somewhat > discourage perl use (and maybe eventually get rid of it entirely) that > would be a useful t/README doc update. Using `sed` was just a personal preference, partly because the `sed` expression seems simpler to me, but mostly because Perl still feels heavyweight to me compared to `sed`. It also may be that I'm just old-school, preferring the small, sharp utilities (`sed`, `grep`, `sort`, etc.), and only turning to Perl (or one of its ilk) when the task demands a more general-purpose tool. I haven't measured, but it's possible that Perl may indeed be heavyweight on Windows in terms of startup time. > I know Johannes (CC'd) has (this is from wetware memory) wanted to > (understandably) not need to bother with Perl as part of GFW, but I > can't remember if that's for a reason covered by NO_PERL=YesPlease, > i.e. packaging it up, or whether it's also to not wanting to provide a > perl for the test suite. I _think_ that was for the NO_PERL=YesPlease case, but I expect Dscho can answer more concretely.
Hi Eric & Ævar, On Mon, 30 Nov 2020, Eric Sunshine wrote: > On Mon, Nov 30, 2020 at 4:20 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > > I did wonder "why not just call perl -e 'print $<' ?" first. But then > > found (by reading the Perl source[1], didn't actually test this) that it > > fakes up UID=0 for everything on Windows. > > I totally forgot about Perl's `$<`. > > Under the Git for Windows SDK, Perl's `$<` returns a large positive > number. I suspect this differs from what you saw in the Perl source > code because the Windows-specific code you looked at does not come > into play in this case. For Git for Windows SDK, Perl is almost > certainly instead built in Unix-like mode, linking against the MSYS2 > library for its POSIX-like emulation, thus the Windows-specific Perl > goop is not needed. Correct. > > I couldn't find any "is root?" in our tree that relies on Perl's $< in a > > bad way (the couuple of occurances seem innocuous), we have some "id -u" > > checks, but those also seem OK if it returned 0 on Windows (what does it > > return?). Seems the worst we'd do there is unintentionally skip some > > "skip this as root" tests. > > Under Git for Windows SDK, `id -u` returns the same large positive > number as returned by Perl's `$<`, which makes sense since `id` is > also likely linked against the MSYS2 library. Correct. > > I know Johannes (CC'd) has (this is from wetware memory) wanted to > > (understandably) not need to bother with Perl as part of GFW, but I > > can't remember if that's for a reason covered by NO_PERL=YesPlease, > > i.e. packaging it up, or whether it's also to not wanting to provide a > > perl for the test suite. > > I _think_ that was for the NO_PERL=YesPlease case, but I expect Dscho > can answer more concretely. I assume you're referring to NO_PERL being set in the CI builds on Windows? There were two problems we tried to address via setting NO_PERL: MSYS2 problems on 32-bit Windows (where the `fork()` emulation would frequently run into DLL base address issues due to the limited address space combined with the requirement to keep the MSYS2 DLL at a _fixed_ memory location), and speed. We no longer run the 32-bit Pipeline, saving a couple electrons. But speed is still a major concern with the automated test suite. Just have a look: https://github.com/git/git/actions/runs/393089617 shows the `linux-clang` job running for a little over 27 minutes, and that includes the build and two test suite runs (taking 12m08s and 12m41s, respectively). At the same time, the Windows tests (_not_ including the build), split into 10 parallel jobs, take between ~8-13.5 minutes to run (granted, there is a 1.5 minute overhead in each split job to download and set up stuff). And that's _skipping_ the test cases requiring Perl, or GPG, or Perforce, etc... Perl is slow. Even on Linux. Running t3701 with the scripted vs the built-in `git add -i` shows a stark contrast. See for yourself: https://github.com/git/git/runs/1478012705?check_suite_focus=true#step:4:994 says that the scripted version took just under 25 seconds, while https://github.com/git/git/runs/1478012705?check_suite_focus=true#step:4:1908 says that the built-in version took just over 8 seconds (this is the `linux-gcc` job that runs the test suite twice, 2nd time with `GIT_TEST_ADD_I_USE_BUILTIN=1`). And that's on Linux. On Windows, not even running any other tests in parallel (i.e. unlike the `linux-gcc` job), the difference is 1m23s vs 39s on my beefy laptop. That's how slow Perl is. Imagine how much faster the entire test suite would run, for every developer, if we didn't use Perl (nor Bash, for that matter). This is not theoretical. There were, and are, instances of contributions which obviously did not run through the test suite before being submitted. I suspect that it simply takes too long. Ciao, Dscho
This is a re-roll of [1] which makes the macOS-specific test of `git maintenance start` and `git maintenance stop` work correctly on Windows (since the tests have been otherwise carefully crafted to work on any platform even though the `start` and `stop` commands themselves are necessarily platform-specific). v2 makes the macOS-specific test in t7900 UID-agnostic, as suggested by Ævar[2], thus the patch which added `test-tool getuid` has been dropped. This series is built atop ds/maintenance-part-4. [1]: https://lore.kernel.org/git/20201127075054.31174-1-sunshine@sunshineco.com/ [2]: https://lore.kernel.org/git/87o8jikfh7.fsf@evledraar.gmail.com/ Eric Sunshine (2): t7900: fix test failures when invoked individually via --run t7900: make macOS-specific test work on Windows t/t7900-maintenance.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) Range-diff against v1: 1: 9f9b26de93 = 1: d535669fb9 t7900: fix test failures when invoked individually via --run 2: 07c9e9bd69 < -: ---------- test-tool: add `getuid` subcommand 3: 01c6a3229d ! 2: 8561d08bcd t7900: make macOS-specific test work on Windows @@ Commit message invoke platform-specific scheduling utilities, their related tests have been carefully crafted -- with one minor exception -- to work correctly on any platform, thus improving overall coverage. The exception is that - the macOS-specific test fails on Windows due to unportable use of - `$(id -u)` and comparison involving the value of $HOME which suffers - from the typical shortcoming on that platform in which the same path may - be represented two different ways depending upon its source (i.e. as a - Windows path `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path - `/usr/src/git/foo`). Fix both problems and drop the !MINGW prerequisite - from the macOS-specific test, thus allowing the test to run on Windows, - as well. + the macOS-specific test fails on Windows due to non-portable use of + `$(id -u)` and comparison involving the value of $HOME. + In particular, on Windows, the value of getuid() called by the C code is + not guaranteed to be the same as `$(id -u)` invoked by the test. This is + because `git.exe` is a native Windows program, whereas the utility + programs run by the test script mostly utilize the MSYS2 runtime, which + emulates a POSIX-like environment. Since the purpose of the test is to + check that the input to the hook is well-formed, the actual user ID is + immaterial, thus we can work around the problem by making the the test + UID-agnostic. + + As for comparison of $HOME, it suffers from the typical shortcoming on + Windows in which the same path may be represented two different ways + depending upon its source (i.e. as a Windows path + `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path + `/usr/src/git/foo`). + + Fix both problems and drop the !MINGW prerequisite from the + macOS-specific test, thus allowing the test to run on Windows, as well. + + Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> ## t/t7900-maintenance.sh ## @@ t/t7900-maintenance.sh: test_expect_success 'start preserves existing schedule' -test_expect_success !MINGW 'start and stop macOS maintenance' ' - uid=$(id -u) && +test_expect_success 'start and stop macOS maintenance' ' -+ uid=$(test-tool getuid) && + # ensure $HOME can be compared against hook arguments on all platforms + pfx=$(cd "$HOME" && pwd) && write_script print-args <<-\EOF && - echo $* >>args +- echo $* >>args ++ echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args + EOF + + rm -f args && @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS maintenance' ' rm -f expect && for frequency in hourly daily weekly @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS mainten + PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" && test_xmllint "$PLIST" && grep schedule=$frequency "$PLIST" && - echo "bootout gui/$uid $PLIST" >>expect && +- echo "bootout gui/$uid $PLIST" >>expect && +- echo "bootstrap gui/$uid $PLIST" >>expect || return 1 ++ echo "bootout gui/[UID] $PLIST" >>expect && ++ echo "bootstrap gui/[UID] $PLIST" >>expect || return 1 + done && + test_cmp expect args && + @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'start and stop macOS maintenance' ' # stop does not unregister the repo git config --get --global maintenance.repo "$(pwd)" && - printf "bootout gui/$uid $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ -+ printf "bootout gui/$uid $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ ++ printf "bootout gui/[UID] $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \ hourly daily weekly >expect && test_cmp expect args && ls "$HOME/Library/LaunchAgents" >actual &&