Hi all, Could someone have a look at my updated webrev below?
Thanks Galder On Fri, Jul 10, 2020 at 6:48 PM Galder Zamarreno <gal...@redhat.com> wrote: > Hi again, > > I've got a new webrev for this: > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8248158/02/webrev/ > > It further expands `type -p` to replace `command -v` uses. It also > replaces $WHICH usages and removes the check for `which` as well. > > I tried Simon's suggestions, but getting that right complicated the patch. > > Galder > > > On Tue, Jul 7, 2020 at 4:31 PM Galder Zamarreno <gal...@redhat.com> wrote: > >> >> >> On Mon, Jul 6, 2020 at 10:50 PM Simon Tooke <sto...@redhat.com> wrote: >> >>> (Disclaimer: I am not a reviewer, so this is an opinion, not a review) >>> >>> I have tested this on Windows and it built without issue, although the >>> submit repo should be the final gate. I'd also like to add my void to >>> simply redefining 'WHICH' as it leads to less changes in the source >>> code, which would make life easier should this be backported to 11u >>> and/or 8u. >> >> >> So, you would just switch the UTIL_REQUIRE_PROGS call for WHICH to be >> `type ...` instead? >> >> >>> >>> -Simon >>> >>> On 2020-07-02 4:22 a.m., Galder Zamarreno wrote: >>> > On Thu, Jul 2, 2020 at 12:37 AM Magnus Ihse Bursie < >>> > magnus.ihse.bur...@oracle.com> wrote: >>> > >>> >> >>> >> On 2020-07-01 12:05, Galder Zamarreno wrote: >>> >>> Using `which` to check whether commands exist can result in confusing >>> >>> errors when `which` itself is not installed in the system. This is >>> the >>> >> case >>> >>> with `autoconf`, where if `autoconf` is present but `which` isn't, >>> the >>> >>> build system says that `autoconf` is missing, when in reality it is >>> >> `which` >>> >>> which is missing. The fix switches autoconf uses of `which` for >>> `type -p` >>> >>> instead, which is a Bash built-in command. >>> >>> >>> >>> I've tested the fix with a fedora docker container that had >>> `autoconf` >>> >>> installed but `which`. When using `type -p` it correctly detects >>> >> `autoconf` >>> >>> installed and eventually fails saying that `which` is not installed, >>> >> which >>> >>> is the expected behaviour. >>> >>> >>> >>> `which` is still in use in make/autoconf/util_windows.m4. A possible >>> >> future >>> >>> improvement would be to see if `which` use there could be replaced as >>> >> well. >>> >>> Eventually, when no `which` uses remain, the presence check for >>> `which` >>> >>> could be removed. >>> >> I agree that we should replace "which" with "type -p" everywhere. The >>> >> best way to do this is probably to replace the value of $WHICH with >>> >> "type -p". It's a bash built-in, so we can count on its presence. If >>> you >>> >> want to fix that as part of this bug, I'm ok with it, otherwise we >>> >> should open a new bug to track this. I think there is also one or two >>> >> instances of "command" recently added as (better, but not as good as >>> >> "type -p") replacements for which. >>> >> >>> > I discovered one place in util.m4 where command was being used. >>> > >>> > There are other places outside of make/ where command is used but I >>> feel >>> > those are a bit out of scope here. >>> > >>> > My main objective is that from a configure perspective, we'd try to >>> reduce >>> > the number of dependencies needed to build things. >>> > >>> > I'll send an updated webrev shortly. >>> > >>> > >>> >> /Magnus >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8248158 >>> >>> WebRev: >>> >> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8248158/01/webrev/ >>> >>> Galder >>> >> >>> >>>