Message ID | 20190208031746.22683-1-tmz@pobox.com (mailing list archive) |
---|---|
Headers | show |
Series | t/lib-gpg: a gpgsm fix, a minor improvement, and a question | expand |
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(-) >
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.
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.
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?
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.
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
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 ;-)
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.
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.