Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
On Tue, Jun 12, 2018 at 11:08 PM Todd Zullinger wrote: > As far as removing the autodie dep, is there anything more > to that than dropping the 'use autodie' line? It looks like > doing so leaves us no worse than we were before, but I > haven't written any perl in a long time. Erasing that line should remove the dependency. I added it as a gratuitous safeguard. I think it's fine to simply remove it. If anyone knows better, of course, I hope they'll say. > Thanks, > > -- > Todd > ~~ > Ocean, n. A body of water occupying about two-thirds of a world made > for man -- who has no gills. > -- Ambrose Bierce, "The Devil's Dictionary" >
Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
Hi, Ævar Arnfjörð Bjarmason wrote: > > On Sun, Jun 10 2018, Todd Zullinger wrote: > >>> 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. > > The intent of those docs was and still is to say "5.8 and the modules it > ships with". > > This was discussed when 2.17.0 was released with my changes to make us > unconditionally use Digest::MD5: > https://public-inbox.org/git/87fu50e0ht@evledraar.gmail.com/ > > As noted there that's not some dogmatic "RedHat altered perl so we don't > care about them", but rather that in practice this doesn't impact users > negatively, so I think it's fine. Yeah, that was my understanding. I only included the information on why it was missing from my build environment despite having perl-5.10.1 was due to the Fedora/Red Hat packaging. I agree that any issues which fall out of those packaging differences are on Fedora/Red Hat packagers to fix. (Which should all be relatively trivial, as the perl modules contain a 'Provides: perl($module)'. That allows dnf|yum install 'perl(autodie)' to easily pull in the right package. And the rpm perl dep generator creates a 'Requires: perl(autodie)' when is sees 'use autodie'.) Sorry if I muddied the conversation with that tangential info. :) >> Ævar brought up bumping the minimum supported perl to 5.10.0 >> last December, in <20171223174400.26668-1-ava...@gmail.com> >> (https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/). >> 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. > > Right, this doesn't apply to autodie. Looking at > https://www.cpan.org/ports/binaries.html there were a lot of releases > that had 5.10.0, not *.1. > > Also git-credential-netrc is in contrib, I don't know if that warrants > treating it differently, I don't use it myself, and don't know how much > it's "not really contrib" in practice (e.g. like the bash completion > which is installed everywhere...)> Indeed, that's a fine question. All the platforms I care about have 5.10.1 or newer, so either way works for me. > But yeah, skimming the code it would be easy to remove the dependency. I wrapped up the changes from Luis which replace my 2/2 "use in-tree Git.pm for tests" and to ensure the tests exit properly on failures. I'll send those out now in the hope that it saves a little effort in moving these minor fixes forward. As far as removing the autodie dep, is there anything more to that than dropping the 'use autodie' line? It looks like doing so leaves us no worse than we were before, but I haven't written any perl in a long time. Thanks, -- Todd ~~ Ocean, n. A body of water occupying about two-thirds of a world made for man -- who has no gills. -- Ambrose Bierce, "The Devil's Dictionary"
Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
On Sun, Jun 10 2018, Todd Zullinger wrote: >> 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. The intent of those docs was and still is to say "5.8 and the modules it ships with". This was discussed when 2.17.0 was released with my changes to make us unconditionally use Digest::MD5: https://public-inbox.org/git/87fu50e0ht@evledraar.gmail.com/ As noted there that's not some dogmatic "RedHat altered perl so we don't care about them", but rather that in practice this doesn't impact users negatively, so I think it's fine. > Ævar brought up bumping the minimum supported perl to 5.10.0 > last December, in <20171223174400.26668-1-ava...@gmail.com> > (https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/). > 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. Right, this doesn't apply to autodie. Looking at https://www.cpan.org/ports/binaries.html there were a lot of releases that had 5.10.0, not *.1. Also git-credential-netrc is in contrib, I don't know if that warrants treating it differently, I don't use it myself, and don't know how much it's "not really contrib" in practice (e.g. like the bash completion which is installed everywhere...)> But yeah, skimming the code it would be easy to remove the dependency.
Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
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 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-ava...@gmail.com>
(https://public-inbox.org/git/20171223174400.26668-1-ava...@gmail.com/).
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
> ---
Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
Thanks for looking into this and addressing these issues.
On Thu, Jun 7, 2018 at 1:20 AM Todd Zullinger wrote:
>
> Hi,
>
> 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.
> 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.
> The second patch in the series aims to fix this. I'm not
> sure if there's a better or more preferable way to fix this,
> which is one of the reasons for the RFC tag. (It's also why
> I added you to the Cc Ævar, as you're one of the
> knowledgeable perl folks here.)
>
> The other reason for the RFC tag is that I'm unsure of how
> to fix the last issue I found. The tests exit cleanly even
> when there are failures, which seems undesirable. I'm not
> familiar with the perl test_external framework to suggest a
> fix in patch form. It might be a matter of adding something
> like this, from t/t9700/test.pl:
>
> my $is_passing = eval { Test::More->is_passing };
> exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
>
> ? But that's a wild guess which I haven't tested.
Good idea.
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
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
)
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
[RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements
Hi,
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.)
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).
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.
The second patch in the series aims to fix this. I'm not
sure if there's a better or more preferable way to fix this,
which is one of the reasons for the RFC tag. (It's also why
I added you to the Cc Ævar, as you're one of the
knowledgeable perl folks here.)
The other reason for the RFC tag is that I'm unsure of how
to fix the last issue I found. The tests exit cleanly even
when there are failures, which seems undesirable. I'm not
familiar with the perl test_external framework to suggest a
fix in patch form. It might be a matter of adding something
like this, from t/t9700/test.pl:
my $is_passing = eval { Test::More->is_passing };
exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
? But that's a wild guess which I haven't tested.
Here's the output from 'make test' showing that most tests
fail and we still get a clean exit status:
$ make -C contrib/credential/netrc test ; echo "netrc test exit status: $?"
make: Entering directory
'/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc'
./t-git-credential-netrc.sh
ok 1 - set up test repository
# run 1: git-credential-netrc (perl
/builddir/build/BUILD/git-2.18.0.rc1/t/../contrib/credential/netrc/test.pl)
ok 2 - Got 0 keys from insecure file
ok 3 - Got 0 keys from missing file
not ok 4 - Got first found keys with bad data
ok 5 - Got no corovamilkbar keys
not ok 6 - Got 2 Github keys
not ok 7 - Got correct Github password
not ok 8 - Got correct Github username
not ok 9 - Got 2 username-specific keys
not ok 10 - Got correct user-specific password
not ok 11 - Got correct user-specific protocol
not ok 12 - Got 2 host:port-specific keys
not ok 13 - Got correct host:port-specific password
not ok 14 - Got correct host:port-specific username
not ok 15 - Got 2 'host:port kills host' keys
not ok 16 - Got correct 'host:port kills host' password
not ok 17 - Got correct 'host:port kills host' username
not ok 18 - Got keys decrypted by git config option
not ok 19 - Got keys decrypted by command option
# test_external test git-credential-netrc was ok
make: Leaving directory
'/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc'
netrc test exit status: 0
Todd Zullinger (2):
git-credential-netrc: make "all" default target of Makefile
git-credential-netrc: use in-tree Git.pm for tests
contrib/credential/netrc/Makefile | 3 +++
contrib/credential/netrc/test.pl | 3 +++
2 files changed, 6 insertions(+)
Thanks all.
--
2.18.0.rc1
