[PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Todd Zullinger]

Hi,

Looking through the build logs for the fedora git packages, I noticed it
was missing the GPGSM prereq.  I added the necessary package to the
build requirements but GPGSM was still failing to be set.  This turned
out to be due to a use of ${GNUPGHOME} without quoting, which leads to a
non-zero exit from echo and the end of the happy && chain when using
bash as the test shell.  Fixing this allows the GPGSM test prereq to be
set.

While I was poking around I also saw an extra gpgconf call to kill
gpg-agent.  This was copied from the GPG block earlier in lib-gpg.sh,
but should not be needed (as far as I can tell).  I don't think it can
cause any real harm apart from causing gpg and gpgsm to start the agent
more often than necessary.  But I didn't run the tests with the --stress
option to look for potential issues that could be more serious.

Lastly, the GPG test prereq was failing in two of the tests where it was
used, t5573-pull-verify-signatures and t7612-merge-verify-signatures.  I
tracked this down to an annoying issue with gnugp-2¹, which recently
became the default /bin/gpg in fedora².

Using gnupg2 as /bin/gpg means using gpg-agent by default.  When using a
non-standard GNUPGHOME, gpg-agent defaults to putting its socket files
in GNUPGHOME and fails if the path for any of them is longer than
sun_path (108 chars on linux, 104 on OpenBSD and FreeBSD, and likely
similar on other unices).

When building in the typical fedora build tool (mock), the path to the
git test dir is "/builddir/build/BUILD/git-2.20.1/t."  That path then
has "trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" appended and a
"gpghome" directory within.  For t5573 and t7612, the gpg-agent socket
path for S.gpg-agent.browser exceeds the sun_path limit and gpg-agent
fails to start.  Sadly, this is handled poorly by gpg and makes the
tests fail to set either the GPG or GPGSM prereqs.

For the fedora packages, I decided to pass --root=/tmp/git-t.XXXX (via
mktemp, of course) to the test suite which ensures a path short enough
to keep gpg-agent happy.

I don't know if there are other packagers or builders who run into this,
so maybe it's not worth much effort to try and have the test suite cope
better.  It took me longer than I would have liked to track it down, so
I thought I'd mention it in case anyone else has run into this or has
thoughts on how to improve lib-gpg.sh while waiting for GnuPG to improve
this area.

A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
dirs in the tests is one thought I had, but didn't try to put it into
patch form.  Setting the --root test option is probably enough control
for most cases.

¹ https://dev.gnupg.org/T2964
² https://fedoraproject.org/wiki/Changes/GnuPG2_as_default_GPG_implementation

Todd Zullinger (2):
  t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt
  t/lib-gpg: drop redundant killing of gpg-agent

 t/lib-gpg.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
Todd

[PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt

[Todd Zullinger]

When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
relax the checking of some root certificate requirements.  The path to
"${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
warning when bash is used to run the tests:

  $ bash t7030-verify-tag.sh
  /git/t/lib-gpg.sh: line 66: ${GNUPGHOME}/trustlist.txt: ambiguous redirect
  ok 1 - create signed tags
  ok 2 # skip create signed tags x509  (missing GPGSM)
  ...

No warning is issued when using bash called as /bin/sh, dash, or mksh.

Quote the path to ensure the redirect works as intended and sets the
GPGSM prereq.  While we're here, drop the space after ">>".

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 t/lib-gpg.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index f1277bef4f..207009793b 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -63,7 +63,7 @@ then
 		cut -d" " -f4 |
 		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
 
-		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
+		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
 		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
 			-u committer@example.com -o /dev/null --sign - 2>&1 &&
-- 
Todd

Re: [PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt

[SZEDER Gábor]

On Thu, Feb 07, 2019 at 10:17:45PM -0500, Todd Zullinger wrote:
> When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
> relax the checking of some root certificate requirements.  The path to
> "${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
> warning when bash is used to run the tests:

s/error/warning/

>   $ bash t7030-verify-tag.sh
>   /git/t/lib-gpg.sh: line 66: ${GNUPGHOME}/trustlist.txt: ambiguous redirect
>   ok 1 - create signed tags
>   ok 2 # skip create signed tags x509  (missing GPGSM)
>   ...
> 
> No warning is issued when using bash called as /bin/sh, dash, or mksh.

Likewise.

POSIX says that no field splitting should be performed on the result
of a parameter expansion that is used as the target of a redirection,
but Bash doesn't conform in this respect (unless in POSIX mode).

> Quote the path to ensure the redirect works as intended and sets the
> GPGSM prereq.  While we're here, drop the space after ">>".
> 
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
>  t/lib-gpg.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index f1277bef4f..207009793b 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -63,7 +63,7 @@ then
>  		cut -d" " -f4 |
>  		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
>  
> -		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> +		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
>  		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
>  		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
>  			-u committer@example.com -o /dev/null --sign - 2>&1 &&
> -- 
> Todd

Re: [PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt

[Todd Zullinger]

SZEDER Gábor wrote:
> On Thu, Feb 07, 2019 at 10:17:45PM -0500, Todd Zullinger wrote:
>> When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
>> relax the checking of some root certificate requirements.  The path to
>> "${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
>> warning when bash is used to run the tests:
> 
> s/error/warning/

Did you mean s/warning/error/ so the sentence reads:

    The path to "${GNUPGHOME}" contains spaces which cause
    an "ambiguous redirect" error when bash is used to run
    the tests

?

Is it worth a resend before Junio queues it?

>>   $ bash t7030-verify-tag.sh
>>   /git/t/lib-gpg.sh: line 66: ${GNUPGHOME}/trustlist.txt: ambiguous redirect
>>   ok 1 - create signed tags
>>   ok 2 # skip create signed tags x509  (missing GPGSM)
>>   ...
>> 
>> No warning is issued when using bash called as /bin/sh, dash, or mksh.
> 
> Likewise.
> 
> POSIX says that no field splitting should be performed on the result
> of a parameter expansion that is used as the target of a redirection,
> but Bash doesn't conform in this respect (unless in POSIX mode).

I wish I'd remembered reading your detailed explanation of
this¹ when I was testing and writing the commit message. :)

¹ https://public-inbox.org/git/20180926121107.GH27036@localhost/

-- 
Todd

Re: [PATCH 1/2] t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt

[SZEDER Gábor]

On Fri, Feb 08, 2019 at 03:25:05PM -0500, Todd Zullinger wrote:
> SZEDER Gábor wrote:
> > On Thu, Feb 07, 2019 at 10:17:45PM -0500, Todd Zullinger wrote:
> >> When gpgsm is installed, lib-gpg.sh attempts to update trustlist.txt to
> >> relax the checking of some root certificate requirements.  The path to
> >> "${GNUPGHOME}" contains spaces which cause an "ambiguous redirect"
> >> warning when bash is used to run the tests:
> > 
> > s/error/warning/
> 
> Did you mean s/warning/error/ so the sentence reads:
> 
>     The path to "${GNUPGHOME}" contains spaces which cause
>     an "ambiguous redirect" error when bash is used to run
>     the tests

Oh, wow.  Indeed that's what I meant.

> Is it worth a resend before Junio queues it?

I remember Junio taking care of minor touchups to commit messages like
this, so maybe not.

[PATCH 2/2] t/lib-gpg: drop redundant killing of gpg-agent

[Todd Zullinger]

In 53fc999306 ("gpg-interface t: extend the existing GPG tests with
GPGSM", 2018-07-20), the gpgconf call which kills gpg-agent was copied
from the existing gpg setup code.

The reason for killing gpg-agent is given in 29ff1f8f74 ("t: lib-gpg:
flush gpg agent on startup", 2017-07-20):

  When running gpg-relevant tests, a gpg-daemon is spawned for each
  GNUPGHOME used. This daemon may stay running after the test and cache
  file descriptors for the trash directories, even after the trash
  directory is removed. This leads to ENOENT errors when attempting to
  create files if tests are run multiple times.

  Add a cleanup script to force flushing the gpg-agent for that GNUPGHOME
  (if any) before setting up the GPG relevant-environment.

Killing gpg-agent once per test is sufficient.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 t/lib-gpg.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 207009793b..8d28652b72 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -64,7 +64,6 @@ then
 		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
 
 		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
-		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
 			-u committer@example.com -o /dev/null --sign - 2>&1 &&
 		test_set_prereq GPGSM
-- 
Todd

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Henning Schild]

Hi,

both patches look good to me. Killing the agent once should be enough,
i remember manually killing it many times as i was looking for a way to
generate certs and trust (configure gpgsm for the test). That is
probably why i copied it over in the first place.

Henning

Am Thu, 7 Feb 2019 22:17:44 -0500
schrieb Todd Zullinger <tmz@pobox.com>:

> Hi,
> 
> Looking through the build logs for the fedora git packages, I noticed
> it was missing the GPGSM prereq.  I added the necessary package to the
> build requirements but GPGSM was still failing to be set.  This turned
> out to be due to a use of ${GNUPGHOME} without quoting, which leads
> to a non-zero exit from echo and the end of the happy && chain when
> using bash as the test shell.  Fixing this allows the GPGSM test
> prereq to be set.
> 
> While I was poking around I also saw an extra gpgconf call to kill
> gpg-agent.  This was copied from the GPG block earlier in lib-gpg.sh,
> but should not be needed (as far as I can tell).  I don't think it can
> cause any real harm apart from causing gpg and gpgsm to start the
> agent more often than necessary.  But I didn't run the tests with the
> --stress option to look for potential issues that could be more
> serious.
> 
> Lastly, the GPG test prereq was failing in two of the tests where it
> was used, t5573-pull-verify-signatures and
> t7612-merge-verify-signatures.  I tracked this down to an annoying
> issue with gnugp-2¹, which recently became the default /bin/gpg in
> fedora².
> 
> Using gnupg2 as /bin/gpg means using gpg-agent by default.  When
> using a non-standard GNUPGHOME, gpg-agent defaults to putting its
> socket files in GNUPGHOME and fails if the path for any of them is
> longer than sun_path (108 chars on linux, 104 on OpenBSD and FreeBSD,
> and likely similar on other unices).
> 
> When building in the typical fedora build tool (mock), the path to the
> git test dir is "/builddir/build/BUILD/git-2.20.1/t."  That path then
> has "trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" appended and a
> "gpghome" directory within.  For t5573 and t7612, the gpg-agent socket
> path for S.gpg-agent.browser exceeds the sun_path limit and gpg-agent
> fails to start.  Sadly, this is handled poorly by gpg and makes the
> tests fail to set either the GPG or GPGSM prereqs.
> 
> For the fedora packages, I decided to pass --root=/tmp/git-t.XXXX (via
> mktemp, of course) to the test suite which ensures a path short enough
> to keep gpg-agent happy.
> 
> I don't know if there are other packagers or builders who run into
> this, so maybe it's not worth much effort to try and have the test
> suite cope better.  It took me longer than I would have liked to
> track it down, so I thought I'd mention it in case anyone else has
> run into this or has thoughts on how to improve lib-gpg.sh while
> waiting for GnuPG to improve this area.
> 
> A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
> dirs in the tests is one thought I had, but didn't try to put it into
> patch form.  Setting the --root test option is probably enough control
> for most cases.
> 
> ¹ https://dev.gnupg.org/T2964
> ²
> https://fedoraproject.org/wiki/Changes/GnuPG2_as_default_GPG_implementation
> 
> Todd Zullinger (2):
>   t/lib-gpg: quote path to ${GNUPGHOME}/trustlist.txt
>   t/lib-gpg: drop redundant killing of gpg-agent
> 
>  t/lib-gpg.sh | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Junio C Hamano]

Henning Schild <henning.schild@siemens.com> writes:

> both patches look good to me. Killing the agent once should be enough,
> i remember manually killing it many times as i was looking for a way to
> generate certs and trust (configure gpgsm for the test). That is
> probably why i copied it over in the first place.

Thanks, both.  Will queue.

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[SZEDER Gábor]

On Thu, Feb 07, 2019 at 10:17:44PM -0500, Todd Zullinger wrote:
> Looking through the build logs for the fedora git packages, I noticed it
> was missing the GPGSM prereq.

Just curious: how did you noticed the missing GPGSM prereq?

I'm asking because I use a patch for a good couple of months now that
collects the prereqs missed by test cases and prints them at the end
of 'make test'.  Its output looks like this:

  https://travis-ci.org/szeder/git/jobs/490944032#L2358

Since you seem to be interested in that sort of thing as well, perhaps
it would be worth to have something like this in git.git?  It's just
that I have been too wary of potentially annoying other contributors
by adding (what might be perceived as) clutter to their 'make test'
output :)


> Lastly, the GPG test prereq was failing in two of the tests where it was
> used, t5573-pull-verify-signatures and t7612-merge-verify-signatures.  I
> tracked this down to an annoying issue with gnugp-2¹, which recently
> became the default /bin/gpg in fedora².
> 
> Using gnupg2 as /bin/gpg means using gpg-agent by default.  When using a
> non-standard GNUPGHOME, gpg-agent defaults to putting its socket files
> in GNUPGHOME and fails if the path for any of them is longer than
> sun_path (108 chars on linux, 104 on OpenBSD and FreeBSD, and likely
> similar on other unices).
> 
> When building in the typical fedora build tool (mock), the path to the
> git test dir is "/builddir/build/BUILD/git-2.20.1/t."  That path then
> has "trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" appended and a
> "gpghome" directory within.  For t5573 and t7612, the gpg-agent socket
> path for S.gpg-agent.browser exceeds the sun_path limit and gpg-agent
> fails to start.  Sadly, this is handled poorly by gpg and makes the
> tests fail to set either the GPG or GPGSM prereqs.
> 
> For the fedora packages, I decided to pass --root=/tmp/git-t.XXXX (via
> mktemp, of course) to the test suite which ensures a path short enough
> to keep gpg-agent happy.
> 
> I don't know if there are other packagers or builders who run into this,
> so maybe it's not worth much effort to try and have the test suite cope
> better.  It took me longer than I would have liked to track it down, so
> I thought I'd mention it in case anyone else has run into this or has
> thoughts on how to improve lib-gpg.sh while waiting for GnuPG to improve
> this area.

I stumbled upon this when Dscho inadvertently broke a test script on
setups without gpg last year; sorry for not speaking about it.  I
noticed it in our Travis CI builds on macOS, because it (macOS itself
or Homebrew? I don't know) defaulted to gpg2 already back then, and to
make matters worse its sun_path is on the shorter end, and the path
to the build dir on Travis CI includes the GitHub user/repo as well.

> A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
> dirs in the tests is one thought I had, but didn't try to put it into
> patch form.  Setting the --root test option is probably enough control
> for most cases.

A potential issue I see with GIT_TEST_GNUPGHOME_ROOT is that there are
several test scripts involving gpg, and if GIT_TEST_GNUPGHOME_ROOT is
set for the whole 'make test', then they might interfere with each
other when they happen to be run at the same time.

In the meantime I came up with a '--short-trash-dir' option to
test-lib, which turns 'trash directory.t7612-merge-verify-signatures'
into 'trash dir.t7612'.  It works, but I don't really like it, and it
required various adjustments to the CI build scripts, notably to the
part in 'ci/print-test-failures.sh' that includes the trash dir of
failed test scripts in the build log.

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Todd Zullinger]

SZEDER Gábor wrote:
> On Thu, Feb 07, 2019 at 10:17:44PM -0500, Todd Zullinger wrote:
>> Looking through the build logs for the fedora git packages, I noticed it
>> was missing the GPGSM prereq.
> 
> Just curious: how did you noticed the missing GPGSM prereq?

I just grep the build output for '# SKIP|skipped:' and then
filter out those which I expect (thing like MINGW and
NATIVE_CRLF that aren't likely to be in a Fedora build).

Far more manual than the slick method you have below. :)

> I'm asking because I use a patch for a good couple of months now that
> collects the prereqs missed by test cases and prints them at the end
> of 'make test'.  Its output looks like this:
> 
>   https://travis-ci.org/szeder/git/jobs/490944032#L2358
> 
> Since you seem to be interested in that sort of thing as well, perhaps
> it would be worth to have something like this in git.git?  It's just
> that I have been too wary of potentially annoying other contributors
> by adding (what might be perceived as) clutter to their 'make test'
> output :)

Indeed, I think that would be useful.  At the very least,
the .missing_prereqs files look quite handy.  I wouldn't
mind the output from 'make test' either, but building
packages surely shifts my perspective toward more verbose
build logs than someone hacking on git regularly and reading
the 'make test' output.

>> I don't know if there are other packagers or builders who run into this,
>> so maybe it's not worth much effort to try and have the test suite cope
>> better.  It took me longer than I would have liked to track it down, so
>> I thought I'd mention it in case anyone else has run into this or has
>> thoughts on how to improve lib-gpg.sh while waiting for GnuPG to improve
>> this area.
> 
> I stumbled upon this when Dscho inadvertently broke a test script on
> setups without gpg last year; sorry for not speaking about it.  I
> noticed it in our Travis CI builds on macOS, because it (macOS itself
> or Homebrew? I don't know) defaulted to gpg2 already back then, and to
> make matters worse its sun_path is on the shorter end, and the path
> to the build dir on Travis CI includes the GitHub user/repo as well.

Heh, I figured it was a rather specific group of folks that
might run into this.

>> A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
>> dirs in the tests is one thought I had, but didn't try to put it into
>> patch form.  Setting the --root test option is probably enough control
>> for most cases.
> 
> A potential issue I see with GIT_TEST_GNUPGHOME_ROOT is that there are
> several test scripts involving gpg, and if GIT_TEST_GNUPGHOME_ROOT is
> set for the whole 'make test', then they might interfere with each
> other when they happen to be run at the same time.

Yeah, I was envisioning that var as something which set the
base dir, under which the normal test directories would
live.  Basically, like setting --root, but only for the
GnuPG bits.

I'm not impressed by that idea (and I'm even less so after
realizing how it would most likely make it harder to gather
up the results in the CI scripts).  I mainly tossed it out
in the hope someone would reply with a better method. ;)

> In the meantime I came up with a '--short-trash-dir' option to
> test-lib, which turns 'trash directory.t7612-merge-verify-signatures'
> into 'trash dir.t7612'.  It works, but I don't really like it, and it
> required various adjustments to the CI build scripts, notably to the
> part in 'ci/print-test-failures.sh' that includes the trash dir of
> failed test scripts in the build log.

I can certainly live with setting '--root' to a shorter path
and waiting to see if GnuPG upstream will come up with
something a little more friendly to users like us - running
gpg in a test suite.  Though if we do just wait it out,
maybe we could/should add a note in t/README or t/lib-gpg.sh
about this to warn others?

-- 
Todd

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[SZEDER Gábor]

On Sat, Feb 09, 2019 at 06:05:53PM -0500, Todd Zullinger wrote:
> SZEDER Gábor wrote:
> > Just curious: how did you noticed the missing GPGSM prereq?
> 
> I just grep the build output for '# SKIP|skipped:' and then
> filter out those which I expect (thing like MINGW and
> NATIVE_CRLF that aren't likely to be in a Fedora build).
> 
> Far more manual than the slick method you have below. :)

Yeah, but yours show the SKIP cases, too, i.e. when the whole test is
skipped by:

  if check-something
  then
        skip_all="no can do"
        test_done
  fi

I didn't bother with that because in those cases the prereq is not
denoted by a single word, but rather by a human-readable phrase, and
becase 'prove' runs those skipped test scripts at last when running
slowest first, so I could already see them anyway.

> > I'm asking because I use a patch for a good couple of months now that
> > collects the prereqs missed by test cases and prints them at the end
> > of 'make test'.  Its output looks like this:
> > 
> >   https://travis-ci.org/szeder/git/jobs/490944032#L2358
> > 
> > Since you seem to be interested in that sort of thing as well, perhaps
> > it would be worth to have something like this in git.git?  It's just
> > that I have been too wary of potentially annoying other contributors
> > by adding (what might be perceived as) clutter to their 'make test'
> > output :)
> 
> Indeed, I think that would be useful.  At the very least,
> the .missing_prereqs files look quite handy.  I wouldn't
> mind the output from 'make test' either, but building
> packages surely shifts my perspective toward more verbose
> build logs than someone hacking on git regularly and reading
> the 'make test' output.

The problem with those files is that a successful 'make test'
automatically and unconditionally removes the whole 'test-results'
directory at the end.  So a separate and optional 'make test ; make
show-missed-prereqs' wouldn't have worked, that's why I did it this
way.

I think it would be better if we kept the 'test-results' directory
even after a successful 'make test', there are some interesting things
to be found there:

  https://public-inbox.org/git/CAM0VKjkVreBKQsvMZ=pEE0NN5gG0MM+XJ0MzCbw1rxi_pR+FXQ@mail.gmail.com/


> >> A GIT_TEST_GNUPGHOME_ROOT var to set the root path for the GNUPGHOME
> >> dirs in the tests is one thought I had, but didn't try to put it into
> >> patch form.  Setting the --root test option is probably enough control
> >> for most cases.
> > 
> > A potential issue I see with GIT_TEST_GNUPGHOME_ROOT is that there are
> > several test scripts involving gpg, and if GIT_TEST_GNUPGHOME_ROOT is
> > set for the whole 'make test', then they might interfere with each
> > other when they happen to be run at the same time.
> 
> Yeah, I was envisioning that var as something which set the
> base dir, under which the normal test directories would
> live.  Basically, like setting --root, but only for the
> GnuPG bits.
> 
> I'm not impressed by that idea (and I'm even less so after
> realizing how it would most likely make it harder to gather
> up the results in the CI scripts).  I mainly tossed it out
> in the hope someone would reply with a better method. ;)
> 
> > In the meantime I came up with a '--short-trash-dir' option to
> > test-lib, which turns 'trash directory.t7612-merge-verify-signatures'
> > into 'trash dir.t7612'.  It works, but I don't really like it, and it
> > required various adjustments to the CI build scripts, notably to the
> > part in 'ci/print-test-failures.sh' that includes the trash dir of
> > failed test scripts in the build log.
> 
> I can certainly live with setting '--root' to a shorter path
> and waiting to see if GnuPG upstream will come up with
> something a little more friendly to users like us - running
> gpg in a test suite.

Are they aware of the issue?

  https://lists.gnupg.org/pipermail/gnupg-users/2017-January/057451.html

suggests to put the socket in '/var/run/user/$(id -u)', but that's for
the "regular" use case, and we should take extra steps to prevent the
tests' gpg from interfering with the gpg of the user running the
tests.  Not sure it would work on macOS.  And ultimately it's not much
different from your GIT_TEST_GNUPGHOME_ROOT suggestion.

Then I stumbled on these patches patches:

  https://lists.zx2c4.com/pipermail/password-store/2017-May/002932.html

suggesting that at least one other project is working around this
issue instead of waiting for upstream to come up with something.

> Though if we do just wait it out,
> maybe we could/should add a note in t/README or t/lib-gpg.sh
> about this to warn others?

Well, a comment could help others to not waste time on figuring out
this "path is too long for a unix domains socket" issue...  but now
they will be able to find this thread in the list archives as well :)



On a related note: did you happen to notice occasional failures with
gpg2 on Fedora builds?  I observed some lately in tests like
'./t7004-tag.sh' or 't7030-verify-tag.sh' on the Travis CI macOS
builds: it appears as if the gpg process were to die mid-verification.
Couldn't make any sense of it yet, though didn't tried particularly
hard either.

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Todd Zullinger]

SZEDER Gábor wrote:
> On Sat, Feb 09, 2019 at 06:05:53PM -0500, Todd Zullinger wrote:
>> SZEDER Gábor wrote:
>>> Just curious: how did you noticed the missing GPGSM prereq?
>> 
>> I just grep the build output for '# SKIP|skipped:' and then
>> filter out those which I expect (thing like MINGW and
>> NATIVE_CRLF that aren't likely to be in a Fedora build).
>> 
>> Far more manual than the slick method you have below. :)
> 
> Yeah, but yours show the SKIP cases, too, i.e. when the whole test is
> skipped by:
> 
>   if check-something
>   then
>         skip_all="no can do"
>         test_done
>   fi
> 
> I didn't bother with that because in those cases the prereq is not
> denoted by a single word, but rather by a human-readable phrase, and
> becase 'prove' runs those skipped test scripts at last when running
> slowest first, so I could already see them anyway.

Ahh, good points.

>>> I'm asking because I use a patch for a good couple of months now that
>>> collects the prereqs missed by test cases and prints them at the end
>>> of 'make test'.  Its output looks like this:
>>> 
>>>   https://travis-ci.org/szeder/git/jobs/490944032#L2358
>>> 
>>> Since you seem to be interested in that sort of thing as well, perhaps
>>> it would be worth to have something like this in git.git?  It's just
>>> that I have been too wary of potentially annoying other contributors
>>> by adding (what might be perceived as) clutter to their 'make test'
>>> output :)
>> 
>> Indeed, I think that would be useful.  At the very least,
>> the .missing_prereqs files look quite handy.  I wouldn't
>> mind the output from 'make test' either, but building
>> packages surely shifts my perspective toward more verbose
>> build logs than someone hacking on git regularly and reading
>> the 'make test' output.
> 
> The problem with those files is that a successful 'make test'
> automatically and unconditionally removes the whole 'test-results'
> directory at the end.  So a separate and optional 'make test ; make
> show-missed-prereqs' wouldn't have worked, that's why I did it this
> way.
> 
> I think it would be better if we kept the 'test-results' directory
> even after a successful 'make test', there are some interesting things
> to be found there:
> 
>   https://public-inbox.org/git/CAM0VKjkVreBKQsvMZ=pEE0NN5gG0MM+XJ0MzCbw1rxi_pR+FXQ@mail.gmail.com/

Maybe that's something which could be controlled with a make
var, to allow folks like us to keep the tests but clean them
up by default for everyone else?

Though even in the fedora package builds, I don't have
access to poke around in test-results and likely wouldn't
want to make the effort to extract the results and dump them
into the build logs (like ci/util/extract-trash-dirs.sh does
for the trash dirs).

>> I can certainly live with setting '--root' to a shorter path
>> and waiting to see if GnuPG upstream will come up with
>> something a little more friendly to users like us - running
>> gpg in a test suite.
> 
> Are they aware of the issue?

Yeah, it was filed as https://dev.gnupg.org/T2964, as a
result of the gnupg-users thread you mention below.  There
hasn't been any progress on it since 2017 though, so it's
doubtful that upstream will fix it anytime soon.  If (or
when) it's resolved, I wouldn't be surprised if only gnupg
>= 2.3 included the fix.  So we'll probably have numerous
systems with this issue for quite some time.

>   https://lists.gnupg.org/pipermail/gnupg-users/2017-January/057451.html
> 
> suggests to put the socket in '/var/run/user/$(id -u)', but that's for
> the "regular" use case, and we should take extra steps to prevent the
> tests' gpg from interfering with the gpg of the user running the
> tests.  Not sure it would work on macOS.  And ultimately it's not much
> different from your GIT_TEST_GNUPGHOME_ROOT suggestion.
> 
> Then I stumbled on these patches patches:
> 
>   https://lists.zx2c4.com/pipermail/password-store/2017-May/002932.html
> 
> suggesting that at least one other project is working around this
> issue instead of waiting for upstream to come up with something.

Heh, the gnupg ticket mentions that the notmuch project
similarly had to work around gpg2's socket handling for
their tests:

    https://notmuchmail.org/pipermail/notmuch/2017/024148.html

>> Though if we do just wait it out,
>> maybe we could/should add a note in t/README or t/lib-gpg.sh
>> about this to warn others?
> 
> Well, a comment could help others to not waste time on figuring out
> this "path is too long for a unix domains socket" issue...  but now
> they will be able to find this thread in the list archives as well :)

True.  Will they curse us for not adding a comment to save
them some effort or can we just say we were waiting for them
to submit such a patch? ;)

> On a related note: did you happen to notice occasional failures with
> gpg2 on Fedora builds?  I observed some lately in tests like
> './t7004-tag.sh' or 't7030-verify-tag.sh' on the Travis CI macOS
> builds: it appears as if the gpg process were to die mid-verification.
> Couldn't make any sense of it yet, though didn't tried particularly
> hard either.

I have not seen any such failures.  I've done a few dozen
test builds on fedora systems where /bin/gpg is gnupg-2.2
and other than the socket path issues, the tests have all
worked well.  But I'll be sure to mention it if I do run
into any such failures.

-- 
Todd

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Jeff King]

On Sat, Feb 09, 2019 at 03:06:05PM +0100, SZEDER Gábor wrote:

> On Thu, Feb 07, 2019 at 10:17:44PM -0500, Todd Zullinger wrote:
> > Looking through the build logs for the fedora git packages, I noticed it
> > was missing the GPGSM prereq.
> 
> Just curious: how did you noticed the missing GPGSM prereq?
> 
> I'm asking because I use a patch for a good couple of months now that
> collects the prereqs missed by test cases and prints them at the end
> of 'make test'.  Its output looks like this:
> 
>   https://travis-ci.org/szeder/git/jobs/490944032#L2358
> 
> Since you seem to be interested in that sort of thing as well, perhaps
> it would be worth to have something like this in git.git?  It's just
> that I have been too wary of potentially annoying other contributors
> by adding (what might be perceived as) clutter to their 'make test'
> output :)

At first I thought your script found tests which _should_ have been
marked with a particular prereq but weren't. And I scratched my head
about how you would find that automatically. If we could, that would be
amazing. ;)

But it looks from the output like it just mentions every prereq that
wasn't satisfied. I don't think that's particularly useful to show for
all users, since most of them are platform things that cannot be changed
(and you'd never get the list to zero, since some of them are mutually
exclusive).

-Peff

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But it looks from the output like it just mentions every prereq that
> wasn't satisfied. I don't think that's particularly useful to show for
> all users, since most of them are platform things that cannot be changed
> (and you'd never get the list to zero, since some of them are mutually
> exclusive).

The only thing that could be of interest is "We are skipping tests
around $SVN, because your Git was built without a feature that
requires platform support of feature $SVN, which in turn was caused
because NO_$SVN=UnfortunatelyYes was in your config.mak.autogen
file.  You could 'apt-get install $SVN' to lift this artificial
limitation in your Git if you wanted to".

But the test prereq is probably a bit too roundabout way for that
kind of information ;-)

Re: [PATCH 0/2] t/lib-gpg: a gpgsm fix, a minor improvement, and a question

[SZEDER Gábor]

On Mon, Feb 11, 2019 at 07:44:33PM -0500, Jeff King wrote:
> On Sat, Feb 09, 2019 at 03:06:05PM +0100, SZEDER Gábor wrote:
> 
> > On Thu, Feb 07, 2019 at 10:17:44PM -0500, Todd Zullinger wrote:
> > > Looking through the build logs for the fedora git packages, I noticed it
> > > was missing the GPGSM prereq.
> > 
> > Just curious: how did you noticed the missing GPGSM prereq?
> > 
> > I'm asking because I use a patch for a good couple of months now that
> > collects the prereqs missed by test cases and prints them at the end
> > of 'make test'.  Its output looks like this:
> > 
> >   https://travis-ci.org/szeder/git/jobs/490944032#L2358

> But it looks from the output like it just mentions every prereq that
> wasn't satisfied. I don't think that's particularly useful to show for
> all users, since most of them are platform things that cannot be changed
> (and you'd never get the list to zero, since some of them are mutually
> exclusive).

The idea was that people might notice when a new unmet prereq pops up
all of a sudden, because they modified something on their setup, or
because a new prereq was recently introduced, e.g. PERLJSON.  Or they
might notice that a prereq necessary to test a fundamental feature is
missing on their setup that they haven't been aware of before, e.g.
TTY.