{"thread":{"id":"51087","subject":"[PATCH] test-lib: try harder to ensure a working jgit","startedAt":"2019-05-14T02:05:30Z","lastAt":"2019-05-15T23:22:45Z","messageCount":11,"participants":["Todd Zullinger","brian m. carlson","Jonathan Nieder","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"375459","messageId":"20190514020520.GI3654@pobox.com","threadId":"51087","inReplyTo":null,"subject":"[PATCH] test-lib: try harder to ensure a working jgit","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-14T02:05:20Z","receivedAt":"2019-05-14T02:05:30Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"The JGIT prereq uses 'type jgit' to determine whether jgit is present.\nWhile this should be sufficient, if the jgit found is broken we'll waste\ntime running tests which fail due to no fault of our own.\n\nUse 'jgit --version' instead, to catch some badly broken jgit\ninstallations.\n\nSigned-off-by: Todd Zullinger \n---\nI ran into such a broken jgit on Fedora >= 30¹. This is clearly a\nproblem in the Fedora jgit package which will hopefully be resolved\nsoon. But it may be good to avoid wasting time debugging tests which\nfail due to a broken tool outside of our control.\n\n¹ https://bugzilla.redhat.com/1709624\n\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 908ddb9c46..599fd70e14 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '\n '\n \n test_lazy_prereq JGIT '\n-\ttype jgit\n+\tjgit --version\n '\n \n # SANITY is about \"can you correctly predict what the filesystem would\n-- \nTodd\n"},{"id":"375460","messageId":"20190514021419.GI7458@genre.crustytoothpaste.net","threadId":"51087","inReplyTo":"20190514020520.GI3654@pobox.com","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-14T02:14:19Z","receivedAt":"2019-05-14T02:14:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 908ddb9c46..599fd70e14 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '\n> '\n> \n> test_lazy_prereq JGIT '\n> -\ttype jgit\n> +\tjgit --version\n> '\n\nI think this is an improvement, not only because of the reasons you\nmentioned, but because we remove the use of \"type\", which is not\nguaranteed to be present in a POSIX shell.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"375462","messageId":"20190514023245.GB34155@google.com","threadId":"51087","inReplyTo":"20190514020520.GI3654@pobox.com","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-05-14T02:32:45Z","receivedAt":"2019-05-14T02:32:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Todd Zullinger wrote:\n\n> The JGIT prereq uses 'type jgit' to determine whether jgit is present.\n> While this should be sufficient, if the jgit found is broken we'll waste\n> time running tests which fail due to no fault of our own.\n>\n> Use 'jgit --version' instead, to catch some badly broken jgit\n> installations.\n>\n> Signed-off-by: Todd Zullinger \n> ---\n> I ran into such a broken jgit on Fedora >= 30¹. This is clearly a\n> problem in the Fedora jgit package which will hopefully be resolved\n> soon. But it may be good to avoid wasting time debugging tests which\n> fail due to a broken tool outside of our control.\n>\n> ¹ https://bugzilla.redhat.com/1709624\n\nReviewed-by: Jonathan Nieder \n\nIt would be nice to describe that bug in the commit message, to save\nreaders some head scratching.\n\nThanks,\nJonathan\n"},{"id":"375473","messageId":"87o945wj7w.fsf@evledraar.gmail.com","threadId":"51087","inReplyTo":"20190514023245.GB34155@google.com","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-14T08:09:23Z","receivedAt":"2019-05-14T08:09:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 14 2019, Jonathan Nieder wrote:\n\n> Todd Zullinger wrote:\n>\n>> The JGIT prereq uses 'type jgit' to determine whether jgit is present.\n>> While this should be sufficient, if the jgit found is broken we'll waste\n>> time running tests which fail due to no fault of our own.\n>>\n>> Use 'jgit --version' instead, to catch some badly broken jgit\n>> installations.\n>>\n>> Signed-off-by: Todd Zullinger \n>> ---\n>> I ran into such a broken jgit on Fedora >= 30¹. This is clearly a\n>> problem in the Fedora jgit package which will hopefully be resolved\n>> soon. But it may be good to avoid wasting time debugging tests which\n>> fail due to a broken tool outside of our control.\n>>\n>> ¹ https://bugzilla.redhat.com/1709624\n>\n> Reviewed-by: Jonathan Nieder \n>\n> It would be nice to describe that bug in the commit message, to save\n> readers some head scratching.\n\nFWIW the jgit in Debian testing/unstable is similarly broken right now:\n\n $ apt policy jgit-cli\n jgit-cli:\n Installed: 3.7.1-6\n Candidate: 3.7.1-6\n Version table:\n *** 3.7.1-6 900\n 900 http://ftp.nl.debian.org/debian testing/main amd64 Packages\n 800 http://ftp.nl.debian.org/debian unstable/main amd64 Packages\n 100 /var/lib/dpkg/status\n 3.7.1-4 700\n 700 http://ftp.nl.debian.org/debian stable/main amd64 Packages\n $ jgit --version; echo $?\n Error: A JNI error has occurred, please check your installation and try again\n Exception in thread \"main\" java.lang.NoClassDefFoundError: org/kohsuke/args4j/CmdLineException\n at java.lang.Class.getDeclaredMethods0(Native Method)\n at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)\n at java.lang.Class.privateGetMethodRecursive(Class.java:3048)\n at java.lang.Class.getMethod0(Class.java:3018)\n at java.lang.Class.getMethod(Class.java:1784)\n at sun.launcher.LauncherHelper.validateMainClass(LauncherHelper.java:544)\n at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:526)\n Caused by: java.lang.ClassNotFoundException: org.kohsuke.args4j.CmdLineException\n at java.net.URLClassLoader.findClass(URLClassLoader.java:382)\n at java.lang.ClassLoader.loadClass(ClassLoader.java:424)\n at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)\n at java.lang.ClassLoader.loadClass(ClassLoader.java:357)\n ... 7 more\n 1\n\nSo rather than describe specific bugs on RedHat/Debian maybe just say:\n\n This guards against cases where jgit is present on the system, but\n will fail to run, e.g. because of some JRE issue, or missing Java\n dependencies. Seeing if it gets far enough to process the\n \"--version\" argument isn't perfect, but seems to be good enough in\n practice. It's also consistent with how we detect some other\n dependencies, see e.g. the CURL and UNZIP prerequisites.\n"},{"id":"375475","messageId":"20190514084534.GA9567@sigill.intra.peff.net","threadId":"51087","inReplyTo":"20190514021419.GI7458@genre.crustytoothpaste.net","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T08:45:34Z","receivedAt":"2019-05-14T08:45:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:\n\n> On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 908ddb9c46..599fd70e14 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '\n> > '\n> > \n> > test_lazy_prereq JGIT '\n> > -\ttype jgit\n> > +\tjgit --version\n> > '\n> \n> I think this is an improvement, not only because of the reasons you\n> mentioned, but because we remove the use of \"type\", which is not\n> guaranteed to be present in a POSIX shell.\n\nIsn't it? I have always treated it as the most-portable option for this\n(compared to, say, `which`). It is in POSIX as a utility (albeit marked\nwith XSI), which even says (in APPLICATION USAGE):\n\n Since type must be aware of the contents of the current shell\n execution environment (such as the lists of commands, functions, and\n built-ins processed by hash), it is always provided as a shell regular\n built-in.\n\nAll that said, I think Todd's patch makes perfect sense even without\nwanting to avoid \"type\".\n\n-Peff\n"},{"id":"375576","messageId":"xmqqzhno8rpa.fsf@gitster-ct.c.googlers.com","threadId":"51087","inReplyTo":"20190514084534.GA9567@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-15T00:52:17Z","receivedAt":"2019-05-15T00:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> All that said, I think Todd's patch makes perfect sense even without\n> wanting to avoid \"type\".\n\nSame here. t/lib-bash.sh seems to use \"if type bash\" to see if one\nis available on $PATH; I've never felt the need to avoid \"type\".\n"},{"id":"375577","messageId":"20190515011217.GN3654@pobox.com","threadId":"51087","inReplyTo":"20190514084534.GA9567@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-15T01:12:18Z","receivedAt":"2019-05-15T01:12:27Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:\n> \n>> On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:\n>>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>>> index 908ddb9c46..599fd70e14 100644\n>>> --- a/t/test-lib.sh\n>>> +++ b/t/test-lib.sh\n>>> @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '\n>>> '\n>>> \n>>> test_lazy_prereq JGIT '\n>>> -\ttype jgit\n>>> +\tjgit --version\n>>> '\n>> \n>> I think this is an improvement, not only because of the reasons you\n>> mentioned, but because we remove the use of \"type\", which is not\n>> guaranteed to be present in a POSIX shell.\n>\n> Isn't it?\n\nI wondered the same thing, but I know I am not nearly as\nfamiliar with the POSIX rules as any of you.\n\n> I have always treated it as the most-portable option for this\n> (compared to, say, `which`). It is in POSIX as a utility (albeit marked\n> with XSI), which even says (in APPLICATION USAGE):\n> \n> Since type must be aware of the contents of the current shell\n> execution environment (such as the lists of commands, functions, and\n> built-ins processed by hash), it is always provided as a shell regular\n> built-in.\n> \n> All that said, I think Todd's patch makes perfect sense even without\n> wanting to avoid \"type\".\n\nYeah, `which` surely isn't a portable option. I presumed\n`type` must be fairly widely available since it was in the\ntest suite since you added it way back in 212f2ffbf0 (\"t:\nadd basic bitmap functionality tests\", 2013-12-21).\n\nI usually make use of `command -p -v $foo` in scripts that\nneed to be portable across systems. But I don't have access\nto many esoteric systems.\n\nBased on Junio's follow-up, I think we can avoid adding\nanything to the commit message about the use of `type` here.\nThat way no one will take it as a sign that we should remove\nother uses of it just for conformance. (I will send a\nfollow-up with an update based on Jonathan and Ævar's\ncomments.)\n\nThanks to all of you.\n\n-- \nTodd\n"},{"id":"375580","messageId":"20190515011851.GP3654@pobox.com","threadId":"51087","inReplyTo":"87o945wj7w.fsf@evledraar.gmail.com","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-15T01:18:51Z","receivedAt":"2019-05-15T01:18:56Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n> \n> On Tue, May 14 2019, Jonathan Nieder wrote:\n> \n>> Todd Zullinger wrote:\n>>\n>>> The JGIT prereq uses 'type jgit' to determine whether jgit is present.\n>>> While this should be sufficient, if the jgit found is broken we'll waste\n>>> time running tests which fail due to no fault of our own.\n>>>\n>>> Use 'jgit --version' instead, to catch some badly broken jgit\n>>> installations.\n>>>\n>>> Signed-off-by: Todd Zullinger \n>>> ---\n>>> I ran into such a broken jgit on Fedora >= 30¹. This is clearly a\n>>> problem in the Fedora jgit package which will hopefully be resolved\n>>> soon. But it may be good to avoid wasting time debugging tests which\n>>> fail due to a broken tool outside of our control.\n>>>\n>>> ¹ https://bugzilla.redhat.com/1709624\n>>\n>> Reviewed-by: Jonathan Nieder \n>>\n>> It would be nice to describe that bug in the commit message, to save\n>> readers some head scratching.\n> \n> FWIW the jgit in Debian testing/unstable is similarly broken right now:\n[...]\n\nHah, small world. :)\n\n> So rather than describe specific bugs on RedHat/Debian maybe just say:\n> \n> This guards against cases where jgit is present on the system, but\n> will fail to run, e.g. because of some JRE issue, or missing Java\n> dependencies. Seeing if it gets far enough to process the\n> \"--version\" argument isn't perfect, but seems to be good enough in\n> practice. It's also consistent with how we detect some other\n> dependencies, see e.g. the CURL and UNZIP prerequisites.\n\nWell said. I indeed avoided putting the detail into the\ncommit message because it was such a Fedora-specific bug.\nI'll update the commit message to add more details though,\nborrowing liberally from^W^W^Wperhaps stealing your\nsuggested wording.\n\nThanks!\n\n-- \nTodd\n"},{"id":"375583","messageId":"20190515013633.GQ3654@pobox.com","threadId":"51087","inReplyTo":"20190514020520.GI3654@pobox.com","subject":"[PATCH v2] test-lib: try harder to ensure a working jgit","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2019-05-15T01:36:33Z","receivedAt":"2019-05-15T01:36:37Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"The JGIT prereq uses `type jgit` to determine whether jgit is present.\nWhile this is usually sufficient, it won't help if the jgit found is\nbadly broken. This wastes time running tests which fail due to no fault\nof our own.\n\nUse `jgit --version` instead, to guard against cases where jgit is\npresent on the system, but will fail to run, e.g. because of some JRE\nissue, or missing Java dependencies. Checking that it gets far enough\nto process the '--version' argument isn't perfect, but seems to be good\nenough in practice. It's also consistent with how we detect some other\ndependencies, see e.g. the CURL and UNZIP prerequisites.\n\nSigned-off-by: Todd Zullinger \n---\nAs promised, I stole the second paragraph from Ævar nearly verbatim. :)\n\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 908ddb9c46..599fd70e14 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '\n '\n \n test_lazy_prereq JGIT '\n-\ttype jgit\n+\tjgit --version\n '\n \n # SANITY is about \"can you correctly predict what the filesystem would\n-- \nTodd\n"},{"id":"375590","messageId":"xmqqtvdw79dh.fsf@gitster-ct.c.googlers.com","threadId":"51087","inReplyTo":"20190515011851.GP3654@pobox.com","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-15T02:13:30Z","receivedAt":"2019-05-15T02:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Todd Zullinger writes:\n\n>> This guards against cases where jgit is present on the system, but\n>> will fail to run, e.g. because of some JRE issue, or missing Java\n>> dependencies. Seeing if it gets far enough to process the\n>> \"--version\" argument isn't perfect, but seems to be good enough in\n>> practice. It's also consistent with how we detect some other\n>> dependencies, see e.g. the CURL and UNZIP prerequisites.\n>\n> Well said. I indeed avoided putting the detail into the\n> commit message because it was such a Fedora-specific bug.\n> I'll update the commit message to add more details though,\n> borrowing liberally from^W^W^Wperhaps stealing your\n> suggested wording.\n>\n> Thanks!\n\nThanks, all.\n"},{"id":"375666","messageId":"20190515232032.GN7458@genre.crustytoothpaste.net","threadId":"51087","inReplyTo":"20190514084534.GA9567@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: try harder to ensure a working jgit","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-05-15T23:20:32Z","receivedAt":"2019-05-15T23:22:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, May 14, 2019 at 04:45:34AM -0400, Jeff King wrote:\n> On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:\n> > I think this is an improvement, not only because of the reasons you\n> > mentioned, but because we remove the use of \"type\", which is not\n> > guaranteed to be present in a POSIX shell.\n> \n> Isn't it? I have always treated it as the most-portable option for this\n> (compared to, say, `which`). It is in POSIX as a utility (albeit marked\n> with XSI), which even says (in APPLICATION USAGE):\n> \n> Since type must be aware of the contents of the current shell\n> execution environment (such as the lists of commands, functions, and\n> built-ins processed by hash), it is always provided as a shell regular\n> built-in.\n\nIt's an XSI extension, while \"command -v\" is not. \"type\" may be more\ncommon for historical reasons, but if you have systems that don't\nimplement XSI, \"command -v\" is the way to go.\n\nI suppose this doesn't matter unless we have people try to build with\nDebian's posh package, which only implements the minimum requirements\nfor a Debian /bin/sh (which don't include XSI extensions, but do include\nlocal).\n\n> All that said, I think Todd's patch makes perfect sense even without\n> wanting to avoid \"type\".\n\nI agree this is an improvement either way.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}