Re: RFR: 8250876: Build system preparation to macos on aarch64

[David Holmes]

Thanks Bernhard. General cross-compilation improvement are a good thing.

Cheers,
David
-

On 5/08/2020 12:02 am, Bernhard Urban-Forster wrote:

Good observation David, the change in adlc is just fixing a symptom. The 
difference to a regular macOS build is that technically, despite running on the 
same machine, it's actually cross compiling due to Rosetta being the 
--build=x86_64 system.

Being a cross-compile, we therefore hit this case:
https://github.com/openjdk/jdk/blob/b0ceab23dd4176329cbf3a95f21e8e9ac2d8723f/make/autoconf/toolchain.m4#L905-L921

And thus infers `/usr/bin/CC` for CXX.

I guess cross compiling hasn't been a thing on macOS yet. I tried the following 
and it passes the adlc build:

--- a/make/autoconf/toolchain.m4
+++ b/make/autoconf/toolchain.m4
@@ -917,7 +917,7 @@ AC_DEFUN_ONCE([TOOLCHAIN_SETUP_BUILD_COMPILERS],
  # find the build compilers in the tools dir, if needed.
  UTIL_REQUIRE_PROGS(BUILD_CC, [cl cc gcc])
  UTIL_FIXUP_EXECUTABLE(BUILD_CC)
-UTIL_REQUIRE_PROGS(BUILD_CXX, [cl CC g++])
+UTIL_REQUIRE_PROGS(BUILD_CXX, [clang++ cl CC g++])
  UTIL_FIXUP_EXECUTABLE(BUILD_CXX)
  UTIL_PATH_PROGS(BUILD_NM, nm gcc-nm)
  UTIL_FIXUP_EXECUTABLE(BUILD_NM)

Although I'm not sure about its cleanliness :-)

-Bernhard


From: build-dev  on behalf of David Holmes 

Sent: Tuesday, August 4, 2020 00:46
To: Erik Joelsson; Vladimir Kempik; build-dev
Cc: Anton Kozlov; Alexander Ioffe; Andrew Brygin; Andrey Petushkov
Subject: Re: RFR: 8250876: Build system preparation to macos on aarch64

On 3/08/2020 10:57 pm, Erik Joelsson wrote:

Hello Vladimir,

These changes look innocent enough to me. They aren't actually adding
macosx-aarch64 support, they are just removing two minor (and more
likely OS version related) hurdles from the build. You still have to
provide the actual configuration on the configure command line as is
shown in your example. Before we can call build system support, we would
need configure to automatically setup those flags and add a separate
parameter for the JNF framework. So, given that, I don't think this
change warrants a JEP in itself.


Of course this change doesn't warrant a JEP in itself :) My point is
that until we have a JEP for the platform that is being targeted then we
shouldn't be making changes to provide support for that platform.

That said I didn't actually look at the changes but focused on the
larger stated aim, so apologies for that.

The actual changes here are small and not obviously related to
supporting macOS-Aarch64. But I'm unclear on this change as it affects
all macOS builds:

42   else ifeq ($(call isBuildOs, macosx), true)
43 ADLC_LDFLAGS := -lc++

if this was fixing a bug as indicated, why do we not see this bug in
regular builds?

Thanks,
David
-



My only complaint is that you revert jib-profiles.js. That file is only
used internally at Oracle. If/when we need it to support macosx-aarch64,
we will provide those changes.

I must say I'm happy to see you managed to get a working build
configuration with just this though!

/Erik

On 2020-08-01 00:24, Vladimir Kempik wrote:

Hello

Please review this change for JDK-8250876

This changeset adds support for macos/aarch64 into build system.
It will allow to crosscompile for macos/aarch64 using intel mac as well.

This changeset does NOT address some arm specific issues in the macos
related code, we plan to do that in s separate commit.

An example of configure to cross-compile for macos/arm64:

--with-boot-jdk=/path/to/java/
--with-build-jdk=/path/to/same/java/as/compiled
--disable-warnings-as-errors --with-jvm-variants=zero
--openjdk-target=aarch64-apple-darwin --with-extra-cflags='-arch
arm64' --with-extra-ldflags='-arch arm64
-F/Path/To/Folder/Containing/JNF_framework/'
—with-extra-cxxflags='-arch arm64’

JNF.framework is missing arm64 part as of next macos release, but
Apple has opensourced it.

Fix to adlc were needed due to it using symbols from stdc++ and not
linking to it, so it fails when doing make images.

The webrev: 
https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~vkempik%2F8250876%2Fwebrev.00%2Fdata=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=HpXJmHXbuawTdExWESK9ssesYTuPTj7N6inXjaHfVaM%3Dreserved=0
The bug: 
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8250876data=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=9z2Nw8d0pa5huxUKOYorMOVy6SBo7o%2FhDT1EmgOhxQ8%3Dreserved=0

Testing: jdk/submit.

Thanks, Vladimir.

Re: RFR: JDK-8248158 Configure fails with autoconf not found even though it's installed

[Erik Joelsson]

I ran this patch through our internal system (corresponds to the submit 
repo), so it's working for us at least.


/Erik

On 2020-08-04 06:25, Galder Zamarreno wrote:

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 , do you need to verify again?

Galder


On Tue, Aug 4, 2020 at 2:40 PM Erik Joelsson > 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
mailto: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
mailto:gal...@redhat.com>> wrote:
>>
>>>
>>> On Mon, Jul 6, 2020 at 10:50 PM Simon Tooke mailto: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

Re: RFR: 8250876: Build system preparation to macos on aarch64

[Erik Joelsson]

That's a good find! You are correct in that we haven't cross compiled in 
any direction involving Macosx before.


The preferred patch would be a bit more elaborate than that. Ideally we 
need better control over the toolchain type of the BUILD_* compiler 
settings. For now, I think just forcing clang/clang++ if BUILD_OS is 
macosx is good enough.


/Erik

On 2020-08-04 07:02, Bernhard Urban-Forster wrote:

Good observation David, the change in adlc is just fixing a symptom. The 
difference to a regular macOS build is that technically, despite running on the 
same machine, it's actually cross compiling due to Rosetta being the 
--build=x86_64 system.

Being a cross-compile, we therefore hit this case:
https://github.com/openjdk/jdk/blob/b0ceab23dd4176329cbf3a95f21e8e9ac2d8723f/make/autoconf/toolchain.m4#L905-L921

And thus infers `/usr/bin/CC` for CXX.

I guess cross compiling hasn't been a thing on macOS yet. I tried the following 
and it passes the adlc build:

--- a/make/autoconf/toolchain.m4
+++ b/make/autoconf/toolchain.m4
@@ -917,7 +917,7 @@ AC_DEFUN_ONCE([TOOLCHAIN_SETUP_BUILD_COMPILERS],
  # find the build compilers in the tools dir, if needed.
  UTIL_REQUIRE_PROGS(BUILD_CC, [cl cc gcc])
  UTIL_FIXUP_EXECUTABLE(BUILD_CC)
-UTIL_REQUIRE_PROGS(BUILD_CXX, [cl CC g++])
+UTIL_REQUIRE_PROGS(BUILD_CXX, [clang++ cl CC g++])
  UTIL_FIXUP_EXECUTABLE(BUILD_CXX)
  UTIL_PATH_PROGS(BUILD_NM, nm gcc-nm)
  UTIL_FIXUP_EXECUTABLE(BUILD_NM)

Although I'm not sure about its cleanliness :-)

-Bernhard


From: build-dev  on behalf of David Holmes 

Sent: Tuesday, August 4, 2020 00:46
To: Erik Joelsson; Vladimir Kempik; build-dev
Cc: Anton Kozlov; Alexander Ioffe; Andrew Brygin; Andrey Petushkov
Subject: Re: RFR: 8250876: Build system preparation to macos on aarch64

On 3/08/2020 10:57 pm, Erik Joelsson wrote:

Hello Vladimir,

These changes look innocent enough to me. They aren't actually adding
macosx-aarch64 support, they are just removing two minor (and more
likely OS version related) hurdles from the build. You still have to
provide the actual configuration on the configure command line as is
shown in your example. Before we can call build system support, we would
need configure to automatically setup those flags and add a separate
parameter for the JNF framework. So, given that, I don't think this
change warrants a JEP in itself.

Of course this change doesn't warrant a JEP in itself :) My point is
that until we have a JEP for the platform that is being targeted then we
shouldn't be making changes to provide support for that platform.

That said I didn't actually look at the changes but focused on the
larger stated aim, so apologies for that.

The actual changes here are small and not obviously related to
supporting macOS-Aarch64. But I'm unclear on this change as it affects
all macOS builds:

42   else ifeq ($(call isBuildOs, macosx), true)
43 ADLC_LDFLAGS := -lc++

if this was fixing a bug as indicated, why do we not see this bug in
regular builds?

Thanks,
David
-



My only complaint is that you revert jib-profiles.js. That file is only
used internally at Oracle. If/when we need it to support macosx-aarch64,
we will provide those changes.

I must say I'm happy to see you managed to get a working build
configuration with just this though!

/Erik

On 2020-08-01 00:24, Vladimir Kempik wrote:

Hello

Please review this change for JDK-8250876

This changeset adds support for macos/aarch64 into build system.
It will allow to crosscompile for macos/aarch64 using intel mac as well.

This changeset does NOT address some arm specific issues in the macos
related code, we plan to do that in s separate commit.

An example of configure to cross-compile for macos/arm64:

--with-boot-jdk=/path/to/java/
--with-build-jdk=/path/to/same/java/as/compiled
--disable-warnings-as-errors --with-jvm-variants=zero
--openjdk-target=aarch64-apple-darwin --with-extra-cflags='-arch
arm64' --with-extra-ldflags='-arch arm64
-F/Path/To/Folder/Containing/JNF_framework/'
—with-extra-cxxflags='-arch arm64’

JNF.framework is missing arm64 part as of next macos release, but
Apple has opensourced it.

Fix to adlc were needed due to it using symbols from stdc++ and not
linking to it, so it fails when doing make images.

The webrev: 
https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~vkempik%2F8250876%2Fwebrev.00%2Fdata=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=HpXJmHXbuawTdExWESK9ssesYTuPTj7N6inXjaHfVaM%3Dreserved=0
The bug: 
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8250876data=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=9z2Nw8d0pa5huxUKOYorMOVy6SBo7o%2FhDT1EmgOhxQ8%3Dreserved=0

Re: RFR: 8250876: Build system preparation to macos on aarch64

[Bernhard Urban-Forster]

Good observation David, the change in adlc is just fixing a symptom. The 
difference to a regular macOS build is that technically, despite running on the 
same machine, it's actually cross compiling due to Rosetta being the 
--build=x86_64 system.

Being a cross-compile, we therefore hit this case:
https://github.com/openjdk/jdk/blob/b0ceab23dd4176329cbf3a95f21e8e9ac2d8723f/make/autoconf/toolchain.m4#L905-L921

And thus infers `/usr/bin/CC` for CXX.

I guess cross compiling hasn't been a thing on macOS yet. I tried the following 
and it passes the adlc build:

--- a/make/autoconf/toolchain.m4
+++ b/make/autoconf/toolchain.m4
@@ -917,7 +917,7 @@ AC_DEFUN_ONCE([TOOLCHAIN_SETUP_BUILD_COMPILERS],
 # find the build compilers in the tools dir, if needed.
 UTIL_REQUIRE_PROGS(BUILD_CC, [cl cc gcc])
 UTIL_FIXUP_EXECUTABLE(BUILD_CC)
-UTIL_REQUIRE_PROGS(BUILD_CXX, [cl CC g++])
+UTIL_REQUIRE_PROGS(BUILD_CXX, [clang++ cl CC g++])
 UTIL_FIXUP_EXECUTABLE(BUILD_CXX)
 UTIL_PATH_PROGS(BUILD_NM, nm gcc-nm)
 UTIL_FIXUP_EXECUTABLE(BUILD_NM)

Although I'm not sure about its cleanliness :-)

-Bernhard


From: build-dev  on behalf of David Holmes 

Sent: Tuesday, August 4, 2020 00:46
To: Erik Joelsson; Vladimir Kempik; build-dev
Cc: Anton Kozlov; Alexander Ioffe; Andrew Brygin; Andrey Petushkov
Subject: Re: RFR: 8250876: Build system preparation to macos on aarch64

On 3/08/2020 10:57 pm, Erik Joelsson wrote:
> Hello Vladimir,
>
> These changes look innocent enough to me. They aren't actually adding
> macosx-aarch64 support, they are just removing two minor (and more
> likely OS version related) hurdles from the build. You still have to
> provide the actual configuration on the configure command line as is
> shown in your example. Before we can call build system support, we would
> need configure to automatically setup those flags and add a separate
> parameter for the JNF framework. So, given that, I don't think this
> change warrants a JEP in itself.

Of course this change doesn't warrant a JEP in itself :) My point is
that until we have a JEP for the platform that is being targeted then we
shouldn't be making changes to provide support for that platform.

That said I didn't actually look at the changes but focused on the
larger stated aim, so apologies for that.

The actual changes here are small and not obviously related to
supporting macOS-Aarch64. But I'm unclear on this change as it affects
all macOS builds:

   42   else ifeq ($(call isBuildOs, macosx), true)
   43 ADLC_LDFLAGS := -lc++

if this was fixing a bug as indicated, why do we not see this bug in
regular builds?

Thanks,
David
-


> My only complaint is that you revert jib-profiles.js. That file is only
> used internally at Oracle. If/when we need it to support macosx-aarch64,
> we will provide those changes.
>
> I must say I'm happy to see you managed to get a working build
> configuration with just this though!
>
> /Erik
>
> On 2020-08-01 00:24, Vladimir Kempik wrote:
>> Hello
>>
>> Please review this change for JDK-8250876
>>
>> This changeset adds support for macos/aarch64 into build system.
>> It will allow to crosscompile for macos/aarch64 using intel mac as well.
>>
>> This changeset does NOT address some arm specific issues in the macos
>> related code, we plan to do that in s separate commit.
>>
>> An example of configure to cross-compile for macos/arm64:
>>
>> --with-boot-jdk=/path/to/java/
>> --with-build-jdk=/path/to/same/java/as/compiled
>> --disable-warnings-as-errors --with-jvm-variants=zero
>> --openjdk-target=aarch64-apple-darwin --with-extra-cflags='-arch
>> arm64' --with-extra-ldflags='-arch arm64
>> -F/Path/To/Folder/Containing/JNF_framework/'
>> —with-extra-cxxflags='-arch arm64’
>>
>> JNF.framework is missing arm64 part as of next macos release, but
>> Apple has opensourced it.
>>
>> Fix to adlc were needed due to it using symbols from stdc++ and not
>> linking to it, so it fails when doing make images.
>>
>> The webrev: 
>> https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~vkempik%2F8250876%2Fwebrev.00%2Fdata=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=HpXJmHXbuawTdExWESK9ssesYTuPTj7N6inXjaHfVaM%3Dreserved=0
>> The bug: 
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8250876data=02%7C01%7Cbeurba%40microsoft.com%7C0c8d58d5eb9144e8717f08d837ff3736%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637320916565796801sdata=9z2Nw8d0pa5huxUKOYorMOVy6SBo7o%2FhDT1EmgOhxQ8%3Dreserved=0
>>
>> Testing: jdk/submit.
>>
>> Thanks, Vladimir.

Re: RFR: JDK-8248158 Configure fails with autoconf not found even though it's installed

[Galder Zamarreno]

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 , do you need to verify again?

Galder


On Tue, Aug 4, 2020 at 2:40 PM Erik Joelsson 
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 
> 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 
> wrote:
> >>
> >>>
> >>> On Mon, Jul 6, 2020 at 10:50 PM Simon Tooke  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
> 
>
>

Re: RFR: JDK-8248158 Configure fails with autoconf not found even though it's installed

[Erik Joelsson]

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  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  wrote:



On Mon, Jul 6, 2020 at 10:50 PM Simon Tooke  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

Re: RFR: JDK-8248158 Configure fails with autoconf not found even though it's installed

[Galder Zamarreno]

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  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  wrote:
>
>>
>>
>> On Mon, Jul 6, 2020 at 10:50 PM Simon Tooke  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
>>> >>
>>>
>>>