git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] test-lib: try harder to ensure a working jgit
@ 2019-05-14  2:05 Todd Zullinger
  2019-05-14  2:14 ` brian m. carlson
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Todd Zullinger @ 2019-05-14  2:05 UTC (permalink / raw)
  To: git

The JGIT prereq uses 'type jgit' to determine whether jgit is present.
While this should be sufficient, if the jgit found is broken we'll waste
time running tests which fail due to no fault of our own.

Use 'jgit --version' instead, to catch some badly broken jgit
installations.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
I ran into such a broken jgit on Fedora >= 30¹.  This is clearly a
problem in the Fedora jgit package which will hopefully be resolved
soon.  But it may be good to avoid wasting time debugging tests which
fail due to a broken tool outside of our control.

¹ https://bugzilla.redhat.com/1709624

 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 908ddb9c46..599fd70e14 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
 '
 
 test_lazy_prereq JGIT '
-	type jgit
+	jgit --version
 '
 
 # SANITY is about "can you correctly predict what the filesystem would
-- 
Todd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  2:05 [PATCH] test-lib: try harder to ensure a working jgit Todd Zullinger
@ 2019-05-14  2:14 ` brian m. carlson
  2019-05-14  8:45   ` Jeff King
  2019-05-14  2:32 ` Jonathan Nieder
  2019-05-15  1:36 ` [PATCH v2] " Todd Zullinger
  2 siblings, 1 reply; 11+ messages in thread
From: brian m. carlson @ 2019-05-14  2:14 UTC (permalink / raw)
  To: Todd Zullinger; +Cc: git

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 908ddb9c46..599fd70e14 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
>  '
>  
>  test_lazy_prereq JGIT '
> -	type jgit
> +	jgit --version
>  '

I think this is an improvement, not only because of the reasons you
mentioned, but because we remove the use of "type", which is not
guaranteed to be present in a POSIX shell.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  2:05 [PATCH] test-lib: try harder to ensure a working jgit Todd Zullinger
  2019-05-14  2:14 ` brian m. carlson
@ 2019-05-14  2:32 ` Jonathan Nieder
  2019-05-14  8:09   ` Ævar Arnfjörð Bjarmason
  2019-05-15  1:36 ` [PATCH v2] " Todd Zullinger
  2 siblings, 1 reply; 11+ messages in thread
From: Jonathan Nieder @ 2019-05-14  2:32 UTC (permalink / raw)
  To: Todd Zullinger; +Cc: git

Todd Zullinger wrote:

> The JGIT prereq uses 'type jgit' to determine whether jgit is present.
> While this should be sufficient, if the jgit found is broken we'll waste
> time running tests which fail due to no fault of our own.
>
> Use 'jgit --version' instead, to catch some badly broken jgit
> installations.
>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> ---
> I ran into such a broken jgit on Fedora >= 30¹.  This is clearly a
> problem in the Fedora jgit package which will hopefully be resolved
> soon.  But it may be good to avoid wasting time debugging tests which
> fail due to a broken tool outside of our control.
>
> ¹ https://bugzilla.redhat.com/1709624

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

It would be nice to describe that bug in the commit message, to save
readers some head scratching.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  2:32 ` Jonathan Nieder
@ 2019-05-14  8:09   ` Ævar Arnfjörð Bjarmason
  2019-05-15  1:18     ` Todd Zullinger
  0 siblings, 1 reply; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2019-05-14  8:09 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Todd Zullinger, git


On Tue, May 14 2019, Jonathan Nieder wrote:

> Todd Zullinger wrote:
>
>> The JGIT prereq uses 'type jgit' to determine whether jgit is present.
>> While this should be sufficient, if the jgit found is broken we'll waste
>> time running tests which fail due to no fault of our own.
>>
>> Use 'jgit --version' instead, to catch some badly broken jgit
>> installations.
>>
>> Signed-off-by: Todd Zullinger <tmz@pobox.com>
>> ---
>> I ran into such a broken jgit on Fedora >= 30¹.  This is clearly a
>> problem in the Fedora jgit package which will hopefully be resolved
>> soon.  But it may be good to avoid wasting time debugging tests which
>> fail due to a broken tool outside of our control.
>>
>> ¹ https://bugzilla.redhat.com/1709624
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> It would be nice to describe that bug in the commit message, to save
> readers some head scratching.

FWIW the jgit in Debian testing/unstable is similarly broken right now:

    $ apt policy jgit-cli
    jgit-cli:
      Installed: 3.7.1-6
      Candidate: 3.7.1-6
      Version table:
     *** 3.7.1-6 900
            900 http://ftp.nl.debian.org/debian testing/main amd64 Packages
            800 http://ftp.nl.debian.org/debian unstable/main amd64 Packages
            100 /var/lib/dpkg/status
         3.7.1-4 700
            700 http://ftp.nl.debian.org/debian stable/main amd64 Packages
    $ jgit --version; echo $?
    Error: A JNI error has occurred, please check your installation and try again
    Exception in thread "main" java.lang.NoClassDefFoundError: org/kohsuke/args4j/CmdLineException
            at java.lang.Class.getDeclaredMethods0(Native Method)
            at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
            at java.lang.Class.privateGetMethodRecursive(Class.java:3048)
            at java.lang.Class.getMethod0(Class.java:3018)
            at java.lang.Class.getMethod(Class.java:1784)
            at sun.launcher.LauncherHelper.validateMainClass(LauncherHelper.java:544)
            at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:526)
    Caused by: java.lang.ClassNotFoundException: org.kohsuke.args4j.CmdLineException
            at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
            at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
            at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
            at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
            ... 7 more
    1

So rather than describe specific bugs on RedHat/Debian maybe just say:

    This guards against cases where jgit is present on the system, but
    will fail to run, e.g. because of some JRE issue, or missing Java
    dependencies. Seeing if it gets far enough to process the
    "--version" argument isn't perfect, but seems to be good enough in
    practice. It's also consistent with how we detect some other
    dependencies, see e.g. the CURL and UNZIP prerequisites.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  2:14 ` brian m. carlson
@ 2019-05-14  8:45   ` Jeff King
  2019-05-15  0:52     ` Junio C Hamano
                       ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jeff King @ 2019-05-14  8:45 UTC (permalink / raw)
  To: brian m. carlson, Todd Zullinger, git

On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:

> On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 908ddb9c46..599fd70e14 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
> >  '
> >  
> >  test_lazy_prereq JGIT '
> > -	type jgit
> > +	jgit --version
> >  '
> 
> I think this is an improvement, not only because of the reasons you
> mentioned, but because we remove the use of "type", which is not
> guaranteed to be present in a POSIX shell.

Isn't it? I have always treated it as the most-portable option for this
(compared to, say, `which`).  It is in POSIX as a utility (albeit marked
with XSI), which even says (in APPLICATION USAGE):

  Since type must be aware of the contents of the current shell
  execution environment (such as the lists of commands, functions, and
  built-ins processed by hash), it is always provided as a shell regular
  built-in.

All that said, I think Todd's patch makes perfect sense even without
wanting to avoid "type".

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  8:45   ` Jeff King
@ 2019-05-15  0:52     ` Junio C Hamano
  2019-05-15  1:12     ` Todd Zullinger
  2019-05-15 23:20     ` brian m. carlson
  2 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-05-15  0:52 UTC (permalink / raw)
  To: Jeff King; +Cc: brian m. carlson, Todd Zullinger, git

Jeff King <peff@peff.net> writes:

> All that said, I think Todd's patch makes perfect sense even without
> wanting to avoid "type".

Same here.  t/lib-bash.sh seems to use "if type bash" to see if one
is available on $PATH; I've never felt the need to avoid "type".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  8:45   ` Jeff King
  2019-05-15  0:52     ` Junio C Hamano
@ 2019-05-15  1:12     ` Todd Zullinger
  2019-05-15 23:20     ` brian m. carlson
  2 siblings, 0 replies; 11+ messages in thread
From: Todd Zullinger @ 2019-05-15  1:12 UTC (permalink / raw)
  To: Jeff King; +Cc: brian m. carlson, Junio C Hamano, git

Hi,

Jeff King wrote:
> On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:
> 
>> On Mon, May 13, 2019 at 10:05:20PM -0400, Todd Zullinger wrote:
>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>> index 908ddb9c46..599fd70e14 100644
>>> --- a/t/test-lib.sh
>>> +++ b/t/test-lib.sh
>>> @@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
>>>  '
>>>  
>>>  test_lazy_prereq JGIT '
>>> -	type jgit
>>> +	jgit --version
>>>  '
>> 
>> I think this is an improvement, not only because of the reasons you
>> mentioned, but because we remove the use of "type", which is not
>> guaranteed to be present in a POSIX shell.
>
> Isn't it?

I wondered the same thing, but I know I am not nearly as
familiar with the POSIX rules as any of you.

> I have always treated it as the most-portable option for this
> (compared to, say, `which`).  It is in POSIX as a utility (albeit marked
> with XSI), which even says (in APPLICATION USAGE):
> 
>   Since type must be aware of the contents of the current shell
>   execution environment (such as the lists of commands, functions, and
>   built-ins processed by hash), it is always provided as a shell regular
>   built-in.
> 
> All that said, I think Todd's patch makes perfect sense even without
> wanting to avoid "type".

Yeah, `which` surely isn't a portable option.  I presumed
`type` must be fairly widely available since it was in the
test suite since you added it way back in 212f2ffbf0 ("t:
add basic bitmap functionality tests", 2013-12-21).

I usually make use of `command -p -v $foo` in scripts that
need to be portable across systems.  But I don't have access
to many esoteric systems.

Based on Junio's follow-up, I think we can avoid adding
anything to the commit message about the use of `type` here.
That way no one will take it as a sign that we should remove
other uses of it just for conformance.  (I will send a
follow-up with an update based on Jonathan and Ævar's
comments.)

Thanks to all of you.

-- 
Todd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  8:09   ` Ævar Arnfjörð Bjarmason
@ 2019-05-15  1:18     ` Todd Zullinger
  2019-05-15  2:13       ` Junio C Hamano
  0 siblings, 1 reply; 11+ messages in thread
From: Todd Zullinger @ 2019-05-15  1:18 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jonathan Nieder, git

Hi,

Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, May 14 2019, Jonathan Nieder wrote:
> 
>> Todd Zullinger wrote:
>>
>>> The JGIT prereq uses 'type jgit' to determine whether jgit is present.
>>> While this should be sufficient, if the jgit found is broken we'll waste
>>> time running tests which fail due to no fault of our own.
>>>
>>> Use 'jgit --version' instead, to catch some badly broken jgit
>>> installations.
>>>
>>> Signed-off-by: Todd Zullinger <tmz@pobox.com>
>>> ---
>>> I ran into such a broken jgit on Fedora >= 30¹.  This is clearly a
>>> problem in the Fedora jgit package which will hopefully be resolved
>>> soon.  But it may be good to avoid wasting time debugging tests which
>>> fail due to a broken tool outside of our control.
>>>
>>> ¹ https://bugzilla.redhat.com/1709624
>>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>>
>> It would be nice to describe that bug in the commit message, to save
>> readers some head scratching.
> 
> FWIW the jgit in Debian testing/unstable is similarly broken right now:
[...]

Hah, small world. :)

> So rather than describe specific bugs on RedHat/Debian maybe just say:
> 
>     This guards against cases where jgit is present on the system, but
>     will fail to run, e.g. because of some JRE issue, or missing Java
>     dependencies. Seeing if it gets far enough to process the
>     "--version" argument isn't perfect, but seems to be good enough in
>     practice. It's also consistent with how we detect some other
>     dependencies, see e.g. the CURL and UNZIP prerequisites.

Well said.  I indeed avoided putting the detail into the
commit message because it was such a Fedora-specific bug.
I'll update the commit message to add more details though,
borrowing liberally from^W^W^Wperhaps stealing your
suggested wording.

Thanks!

-- 
Todd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] test-lib: try harder to ensure a working jgit
  2019-05-14  2:05 [PATCH] test-lib: try harder to ensure a working jgit Todd Zullinger
  2019-05-14  2:14 ` brian m. carlson
  2019-05-14  2:32 ` Jonathan Nieder
@ 2019-05-15  1:36 ` " Todd Zullinger
  2 siblings, 0 replies; 11+ messages in thread
From: Todd Zullinger @ 2019-05-15  1:36 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, brian m. carlson,
	Jeff King, Junio C Hamano, Jonathan Nieder

The JGIT prereq uses `type jgit` to determine whether jgit is present.
While this is usually sufficient, it won't help if the jgit found is
badly broken.  This wastes time running tests which fail due to no fault
of our own.

Use `jgit --version` instead, to guard against cases where jgit is
present on the system, but will fail to run, e.g. because of some JRE
issue, or missing Java dependencies.  Checking that it gets far enough
to process the '--version' argument isn't perfect, but seems to be good
enough in practice.  It's also consistent with how we detect some other
dependencies, see e.g. the CURL and UNZIP prerequisites.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
As promised, I stole the second paragraph from Ævar nearly verbatim. :)

 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 908ddb9c46..599fd70e14 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1522,7 +1522,7 @@ test_lazy_prereq NOT_ROOT '
 '
 
 test_lazy_prereq JGIT '
-	type jgit
+	jgit --version
 '
 
 # SANITY is about "can you correctly predict what the filesystem would
-- 
Todd

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-15  1:18     ` Todd Zullinger
@ 2019-05-15  2:13       ` Junio C Hamano
  0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2019-05-15  2:13 UTC (permalink / raw)
  To: Todd Zullinger
  Cc: Ævar Arnfjörð Bjarmason, Jonathan Nieder, git

Todd Zullinger <tmz@pobox.com> writes:

>>     This guards against cases where jgit is present on the system, but
>>     will fail to run, e.g. because of some JRE issue, or missing Java
>>     dependencies. Seeing if it gets far enough to process the
>>     "--version" argument isn't perfect, but seems to be good enough in
>>     practice. It's also consistent with how we detect some other
>>     dependencies, see e.g. the CURL and UNZIP prerequisites.
>
> Well said.  I indeed avoided putting the detail into the
> commit message because it was such a Fedora-specific bug.
> I'll update the commit message to add more details though,
> borrowing liberally from^W^W^Wperhaps stealing your
> suggested wording.
>
> Thanks!

Thanks, all.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] test-lib: try harder to ensure a working jgit
  2019-05-14  8:45   ` Jeff King
  2019-05-15  0:52     ` Junio C Hamano
  2019-05-15  1:12     ` Todd Zullinger
@ 2019-05-15 23:20     ` brian m. carlson
  2 siblings, 0 replies; 11+ messages in thread
From: brian m. carlson @ 2019-05-15 23:20 UTC (permalink / raw)
  To: Jeff King; +Cc: Todd Zullinger, git

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

On Tue, May 14, 2019 at 04:45:34AM -0400, Jeff King wrote:
> On Tue, May 14, 2019 at 02:14:19AM +0000, brian m. carlson wrote:
> > I think this is an improvement, not only because of the reasons you
> > mentioned, but because we remove the use of "type", which is not
> > guaranteed to be present in a POSIX shell.
> 
> Isn't it? I have always treated it as the most-portable option for this
> (compared to, say, `which`).  It is in POSIX as a utility (albeit marked
> with XSI), which even says (in APPLICATION USAGE):
> 
>   Since type must be aware of the contents of the current shell
>   execution environment (such as the lists of commands, functions, and
>   built-ins processed by hash), it is always provided as a shell regular
>   built-in.

It's an XSI extension, while "command -v" is not. "type" may be more
common for historical reasons, but if you have systems that don't
implement XSI, "command -v" is the way to go.

I suppose this doesn't matter unless we have people try to build with
Debian's posh package, which only implements the minimum requirements
for a Debian /bin/sh (which don't include XSI extensions, but do include
local).

> All that said, I think Todd's patch makes perfect sense even without
> wanting to avoid "type".

I agree this is an improvement either way.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14  2:05 [PATCH] test-lib: try harder to ensure a working jgit Todd Zullinger
2019-05-14  2:14 ` brian m. carlson
2019-05-14  8:45   ` Jeff King
2019-05-15  0:52     ` Junio C Hamano
2019-05-15  1:12     ` Todd Zullinger
2019-05-15 23:20     ` brian m. carlson
2019-05-14  2:32 ` Jonathan Nieder
2019-05-14  8:09   ` Ævar Arnfjörð Bjarmason
2019-05-15  1:18     ` Todd Zullinger
2019-05-15  2:13       ` Junio C Hamano
2019-05-15  1:36 ` [PATCH v2] " Todd Zullinger

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox