Hi Erik, I've been in contact with Simon (cc) who has been testing the patch on Windows and he said it all looked good.
@Simon Tooke <sto...@redhat.com>, do you need to verify again? Galder On Tue, Aug 4, 2020 at 2:40 PM Erik Joelsson <erik.joels...@oracle.com> wrote: > I think this looks ok. Have you tested it on Windows? > > /Erik > > On 2020-08-04 01:40, Galder Zamarreno wrote: > > 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 > >>>> > >
