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
