Hi Luis,
Luis Marsano wrote:
> Thanks for looking into this and addressing these issues.
And thank you for digging further. :)
> On Thu, Jun 7, 2018 at 1:20 AM Todd Zullinger <tmz@xxxxxxxxx> wrote:
>> I noticed failures from the contrib/credential/netrc tests
>> while building 2.18.0 release candidates. I was surprised
>> to see the tests being run when called with a simple 'make'
>> command.
>>
>> The first patch in the series adds an empty 'all::' make
>> target to match most of our other Makefiles and avoid the
>> surprise of running tests by default. (When the netrc
>> helper was added to the fedora builds, it copied the same
>> 'make -C contrib/credential/...' pattern from other
>> credential helpers -- despite the lack of anything to
>> build.)
>
> I think this is a good idea.
>
>> The actual test failures were initially due to my build
>> environment lacking the perl autodie module, which was added
>> in 786ef50a23 ("git-credential-netrc: accept gpg option",
>> 2018-05-12).
>
> I added 'use autodie;' without realizing it had external dependencies.
> According to the documentation
> http://perldoc.perl.org/autodie.html
> it's a pragma since perl 5.10.1
> Removing 'use autodie;' should be fine: it's not critical.
I should clarify that part of why autodie isn't in my build
environment is that the Fedora and RHEL7+ perl packages
split out many modules which are shipped as part of the core
perl tarball. So while all the platforms I care about have
perl >= 5.10.1, the Fedora and newer RHEL systems have the
autodie module in a separate package.
That said, the INSTALL docs still suggest that we only
require perl >= 5.8, so if autodie is easily removed, that
would probably be a good plan.
Ævar brought up bumping the minimum supported perl to 5.10.0
last December, in <20171223174400.26668-1-avarab@xxxxxxxxx>
(https://public-inbox.org/git/20171223174400.26668-1-avarab@xxxxxxxxx/).
The general consensus seemed positive, but I don't think
it's happened. Even so, that was 5.10.0, not the 5.10.1
which added autodie.
>> After installing the autodie module, the failures were due
>> to the build environment lacking a git install (specifically
>> the perl Git module). The tests needing a pre-installed
>> perl Git seemed odd and worth fixing.
>
> I mistakenly thought 'use lib (split(/:/, $ENV{GITPERLLIB}));' in
> test.pl handled this.
> Since it doesn't, and I was only following an example from
> t/t9700/test.pl that doesn't fit, this line should be removed and it
> might make more sense to set the environment from
> t-git-credential-netrc.sh near '. ./test-lib.sh', which also sets the
> environment.
> Something like
>
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,5 +23,6 @@
> # The external test will outputs its own plan
> test_external_has_tap=1
>
> + export PERL5LIB="$GITPERLLIB"
> test_external \
> 'git-credential-netrc' \
>
> Your solution, however, is reasonable, and I don't know which is preferred.
I think your placement is better. As you say, it could also
be placed closer to '. ./test-lib.sh'.
It doesn't come up very often, but I wonder if there's any
downside to having test-lib.sh export PERL5LIB?
> It looks like you found an issue with t/t9700/test.pl, too.
> When altered to fail, it first reports ok (then reports failed and
> returns non-0).
>
> not ok 46 - unquote simple quoted path
> not ok 47 - unquote escape sequences
> 1..47
> # test_external test Perl API was ok
> # test_external_without_stderr test no stderr: Perl API failed: perl
> /home/luism/project/git/t/t9700/test.pl:
> $ echo $?
> 1
Oops. Nice catch. At least that does exit non-zero I
guess.
> To make git-credential-netrc tests behave correctly, I ended up making
> the changes below.
> They might be okay, unless someone knows better.
>
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,9 +23,10 @@
> # The external test will outputs its own plan
> test_external_has_tap=1
>
> + export PERL5LIB="$GITPERLLIB"
> test_external \
> 'git-credential-netrc' \
> - perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
> + perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
>
> test_done
> )
This reminds me, while we're here it might be worth adding a
whitespace cleanup commit to indent the lines following
test_expect_success and test_external with 2 tabs.
> diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
> index 1e10010..abc9081 100755
> --- a/contrib/credential/netrc/test.pl
> +++ b/contrib/credential/netrc/test.pl
> @@ -1,6 +1,4 @@
> #!/usr/bin/perl
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> -
> use warnings;
> use strict;
> use Test::More qw(no_plan);
> @@ -12,7 +10,6 @@ BEGIN
> # t-git-credential-netrc.sh kicks off our testing, so we have to go
> # from there.
> Test::More->builder->current_test(1);
> - Test::More->builder->no_ending(1);
> }
>
> my @global_credential_args = @ARGV;
> @@ -104,6 +101,9 @@ BEGIN
>
> ok(scalar keys %$cred == 2, 'Got keys decrypted by command option');
>
> +my $is_passing = eval { Test::More->is_passing };
> +exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
> +
> sub run_credential
> {
> my $args = shift @_;
I tested this and it worked well for my builds with and
without the autodie module (passing and failing
appropriately).
Thanks,
--
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A common mistake people make when trying to design something
completely foolproof is to underestimate the ingenuity of complete
fools.
-- Douglas Adams
