All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
@ 2020-11-30  4:42 Eric Sunshine
  2020-11-30  4:42 ` [PATCH v2 1/2] t7900: fix test failures when invoked individually via --run Eric Sunshine
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Eric Sunshine @ 2020-11-30  4:42 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee, Ævar Arnfjörð Bjarmason, Eric Sunshine

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 &&
-- 
2.29.2.576.ga3fc446d84


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] t7900: fix test failures when invoked individually via --run
  2020-11-30  4:42 [PATCH v2 0/2] make macOS `git maintenance` test work on Windows Eric Sunshine
@ 2020-11-30  4:42 ` Eric Sunshine
  2020-11-30  4:42 ` [PATCH v2 2/2] t7900: make macOS-specific test work on Windows Eric Sunshine
  2020-11-30  9:20 ` [PATCH v2 0/2] make macOS `git maintenance` " Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 9+ messages in thread
From: Eric Sunshine @ 2020-11-30  4:42 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee, Ævar Arnfjörð Bjarmason, Eric Sunshine

A couple tests use `rm expect` to remove a file created by earlier
tests. The presence of this file is immaterial at the point the attempt
is made to remove it, yet if it doesn't exist, the test fails
unnecessarily. One case in which the file may validly not exist is when
--run or GIT_SKIP_TESTS is used to selectively run particular tests. Fix
these pointless failures.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t7900-maintenance.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0246e4ce30..ef3aec3253 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -429,7 +429,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
 	EOF
 	test_cmp expect actual &&
 
-	rm expect &&
+	rm -f expect &&
 	for frequency in hourly daily weekly
 	do
 		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
@@ -491,7 +491,6 @@ test_expect_success 'start and stop Windows maintenance' '
 	# stop does not unregister the repo
 	git config --get --global maintenance.repo "$(pwd)" &&
 
-	rm expect &&
 	printf "/delete /tn Git Maintenance (%s) /f\n" \
 		hourly daily weekly >expect &&
 	test_cmp expect args
-- 
2.29.2.576.ga3fc446d84


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] t7900: make macOS-specific test work on Windows
  2020-11-30  4:42 [PATCH v2 0/2] make macOS `git maintenance` test work on Windows Eric Sunshine
  2020-11-30  4:42 ` [PATCH v2 1/2] t7900: fix test failures when invoked individually via --run Eric Sunshine
@ 2020-11-30  4:42 ` Eric Sunshine
  2020-11-30 13:12   ` Derrick Stolee
  2020-11-30  9:20 ` [PATCH v2 0/2] make macOS `git maintenance` " Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 9+ messages in thread
From: Eric Sunshine @ 2020-11-30  4:42 UTC (permalink / raw)
  To: git; +Cc: Derrick Stolee, Ævar Arnfjörð Bjarmason, Eric Sunshine

Although `git maintenance start` and `git maintenance stop` necessarily
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 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 | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ef3aec3253..514977a838 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -408,11 +408,12 @@ test_expect_success 'start preserves existing schedule' '
 	grep "Important information!" cron.txt
 '
 
-test_expect_success !MINGW 'start and stop macOS maintenance' '
-	uid=$(id -u) &&
+test_expect_success 'start and stop macOS maintenance' '
+	# ensure $HOME can be compared against hook arguments on all platforms
+	pfx=$(cd "$HOME" && pwd) &&
 
 	write_script print-args <<-\EOF &&
-	echo $* >>args
+	echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args
 	EOF
 
 	rm -f args &&
@@ -432,11 +433,11 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
 	rm -f expect &&
 	for frequency in hourly daily weekly
 	do
-		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
+		PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
 		test_xmllint "$PLIST" &&
 		grep schedule=$frequency "$PLIST" &&
-		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 &&
 
@@ -446,7 +447,7 @@ 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" \
 		hourly daily weekly >expect &&
 	test_cmp expect args &&
 	ls "$HOME/Library/LaunchAgents" >actual &&
-- 
2.29.2.576.ga3fc446d84


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
  2020-11-30  4:42 [PATCH v2 0/2] make macOS `git maintenance` test work on Windows Eric Sunshine
  2020-11-30  4:42 ` [PATCH v2 1/2] t7900: fix test failures when invoked individually via --run Eric Sunshine
  2020-11-30  4:42 ` [PATCH v2 2/2] t7900: make macOS-specific test work on Windows Eric Sunshine
@ 2020-11-30  9:20 ` Ævar Arnfjörð Bjarmason
  2020-11-30 23:32   ` Junio C Hamano
  2020-12-01  3:58   ` Eric Sunshine
  2 siblings, 2 replies; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2020-11-30  9:20 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git, Derrick Stolee, Johannes Schindelin


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] t7900: make macOS-specific test work on Windows
  2020-11-30  4:42 ` [PATCH v2 2/2] t7900: make macOS-specific test work on Windows Eric Sunshine
@ 2020-11-30 13:12   ` Derrick Stolee
  2020-12-01  3:03     ` Eric Sunshine
  0 siblings, 1 reply; 9+ messages in thread
From: Derrick Stolee @ 2020-11-30 13:12 UTC (permalink / raw)
  To: Eric Sunshine, git; +Cc: Ævar Arnfjörð Bjarmason

On 11/29/2020 11:42 PM, Eric Sunshine wrote:
> Although `git maintenance start` and `git maintenance stop` necessarily
> 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 non-portable use of
> `$(id -u)` and comparison involving the value of $HOME.

I appreciate your time making this change, as I was unable to work
around these issues myself.

> 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 | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index ef3aec3253..514977a838 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -408,11 +408,12 @@ test_expect_success 'start preserves existing schedule' '
>  	grep "Important information!" cron.txt
>  '
>  
> -test_expect_success !MINGW 'start and stop macOS maintenance' '
> -	uid=$(id -u) &&
> +test_expect_success 'start and stop macOS maintenance' '
> +	# ensure $HOME can be compared against hook arguments on all platforms
> +	pfx=$(cd "$HOME" && pwd) &&

> -		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> +		PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&

This pair of changes make sense to get around the $HOME issue
that caused me to surrender the effort.

>  	write_script print-args <<-\EOF &&
> -	echo $* >>args
> +	echo $* | sed "s:gui/[0-9][0-9]*:gui/[UID]:" >>args

Filtering out the UID into a fixed string...

> -		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 &&

.. then checking for that fixed string. Clever!

Thanks!
-Stolee

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
  2020-11-30  9:20 ` [PATCH v2 0/2] make macOS `git maintenance` " Ævar Arnfjörð Bjarmason
@ 2020-11-30 23:32   ` Junio C Hamano
  2020-12-01  3:58   ` Eric Sunshine
  1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2020-11-30 23:32 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Eric Sunshine, git, Derrick Stolee, Johannes Schindelin

Æ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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] t7900: make macOS-specific test work on Windows
  2020-11-30 13:12   ` Derrick Stolee
@ 2020-12-01  3:03     ` Eric Sunshine
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Sunshine @ 2020-12-01  3:03 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Git List, Ævar Arnfjörð Bjarmason

On Mon, Nov 30, 2020 at 8:12 AM Derrick Stolee <stolee@gmail.com> wrote:
> On 11/29/2020 11:42 PM, Eric Sunshine wrote:
> > +     # ensure $HOME can be compared against hook arguments on all platforms
> > +     pfx=$(cd "$HOME" && pwd) &&
> > +             PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
>
> This pair of changes make sense to get around the $HOME issue
> that caused me to surrender the effort.

In case it's not clear to other readers not familiar with Git on
Windows (or who have not read t/README), the magic here is that `pwd`
is overridden in t/test-lib.sh for MINGW to always return a
Windows-style path.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
  2020-11-30  9:20 ` [PATCH v2 0/2] make macOS `git maintenance` " Ævar Arnfjörð Bjarmason
  2020-11-30 23:32   ` Junio C Hamano
@ 2020-12-01  3:58   ` Eric Sunshine
  2020-12-01 12:31     ` Johannes Schindelin
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Sunshine @ 2020-12-01  3:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Derrick Stolee, Johannes Schindelin

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/2] make macOS `git maintenance` test work on Windows
  2020-12-01  3:58   ` Eric Sunshine
@ 2020-12-01 12:31     ` Johannes Schindelin
  0 siblings, 0 replies; 9+ messages in thread
From: Johannes Schindelin @ 2020-12-01 12:31 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, Git List, Derrick Stolee

[-- Attachment #1: Type: text/plain, Size: 3970 bytes --]

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-12-01 12:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-30  4:42 [PATCH v2 0/2] make macOS `git maintenance` test work on Windows Eric Sunshine
2020-11-30  4:42 ` [PATCH v2 1/2] t7900: fix test failures when invoked individually via --run Eric Sunshine
2020-11-30  4:42 ` [PATCH v2 2/2] t7900: make macOS-specific test work on Windows Eric Sunshine
2020-11-30 13:12   ` Derrick Stolee
2020-12-01  3:03     ` Eric Sunshine
2020-11-30  9:20 ` [PATCH v2 0/2] make macOS `git maintenance` " Ævar Arnfjörð Bjarmason
2020-11-30 23:32   ` Junio C Hamano
2020-12-01  3:58   ` Eric Sunshine
2020-12-01 12:31     ` Johannes Schindelin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.