RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Mikael Vidstedt
This adds the necessary macosx-aarch64 support to the build configurations at Oracle.

-------------

Commit messages:
 - 8264224: Add macosx-aarch64 to Oracle build configurations

Changes: https://git.openjdk.java.net/jdk/pull/3258/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3258&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264224
  Stats: 55 lines in 1 file changed: 38 ins; 0 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3258.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3258/head:pull/3258

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Sergey Bylokhov-2
On Tue, 30 Mar 2021 06:25:21 GMT, Mikael Vidstedt <[hidden email]> wrote:

> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.

make/conf/jib-profiles.js line 456:

> 454:             configure_args: concat(common.configure_args_64bit, "--with-zlib=system",
> 455:                 "--with-macosx-version-max=11.00.00",
> 456:                 "SETFILE=/usr/bin/SetFile"),

"SetFile" from the devkit still does not work on the latest macOS?

make/conf/jib-profiles.js line 1049:

> 1047:     var devkit_platform_revisions = {
> 1048:         linux_x64: "gcc10.2.0-OL6.4+1.0",
> 1049:         macosx: "Xcode12.4+1.0",

XCode bump w/o check for aarch?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Magnus Ihse Bursie-3
On Tue, 30 Mar 2021 06:42:39 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.
>
> make/conf/jib-profiles.js line 1049:
>
>> 1047:     var devkit_platform_revisions = {
>> 1048:         linux_x64: "gcc10.2.0-OL6.4+1.0",
>> 1049:         macosx: "Xcode12.4+1.0",
>
> XCode bump w/o check for aarch?

This seems to be used at line 1133 where 11.3.1 is hard-coded for x64. So I guess it works but I'm not very fond of this solution. @vidmik Why don't you put both x64 and aarch64 revisions here?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Magnus Ihse Bursie-3
In reply to this post by Sergey Bylokhov-2
On Tue, 30 Mar 2021 06:40:12 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.
>
> make/conf/jib-profiles.js line 456:
>
>> 454:             configure_args: concat(common.configure_args_64bit, "--with-zlib=system",
>> 455:                 "--with-macosx-version-max=11.00.00",
>> 456:                 "SETFILE=/usr/bin/SetFile"),
>
> "SetFile" from the devkit still does not work on the latest macOS?

I'm a bit surprised at this myself. If it is needed, I will accept this patch, but then I think we need to fix that properly and submit a follow-up patch.

I also note that on macos-x64 we add `--enable-compatible-cds-alignment` as well. I wonder if we should not do that here as well..? (Although I must admit I did not fully understand when and how the CDS alignment rules applied.)

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Erik Joelsson-2
On Tue, 30 Mar 2021 11:44:29 GMT, Magnus Ihse Bursie <[hidden email]> wrote:

>> make/conf/jib-profiles.js line 456:
>>
>>> 454:             configure_args: concat(common.configure_args_64bit, "--with-zlib=system",
>>> 455:                 "--with-macosx-version-max=11.00.00",
>>> 456:                 "SETFILE=/usr/bin/SetFile"),
>>
>> "SetFile" from the devkit still does not work on the latest macOS?
>
> I'm a bit surprised at this myself. If it is needed, I will accept this patch, but then I think we need to fix that properly and submit a follow-up patch.
>
> I also note that on macos-x64 we add `--enable-compatible-cds-alignment` as well. I wonder if we should not do that here as well..? (Although I must admit I did not fully understand when and how the CDS alignment rules applied.)

I don't think we need to set SETFILE anymore. This used to be needed because the SetFile binary in older Xcode versions was 32 bit and Catalina no longer supports 32 bit apps. This should definitely not be a problem on aarch64.

>> make/conf/jib-profiles.js line 1049:
>>
>>> 1047:     var devkit_platform_revisions = {
>>> 1048:         linux_x64: "gcc10.2.0-OL6.4+1.0",
>>> 1049:         macosx: "Xcode12.4+1.0",
>>
>> XCode bump w/o check for aarch?
>
> This seems to be used at line 1133 where 11.3.1 is hard-coded for x64. So I guess it works but I'm not very fond of this solution. @vidmik Why don't you put both x64 and aarch64 revisions here?

As I understand it, the intention is to bump the Xcode version for x64 as well, though I must say I would prefer that change to go in separately.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Erik Joelsson-2
On Tue, 30 Mar 2021 12:29:49 GMT, Erik Joelsson <[hidden email]> wrote:

>> I'm a bit surprised at this myself. If it is needed, I will accept this patch, but then I think we need to fix that properly and submit a follow-up patch.
>>
>> I also note that on macos-x64 we add `--enable-compatible-cds-alignment` as well. I wonder if we should not do that here as well..? (Although I must admit I did not fully understand when and how the CDS alignment rules applied.)
>
> I don't think we need to set SETFILE anymore. This used to be needed because the SetFile binary in older Xcode versions was 32 bit and Catalina no longer supports 32 bit apps. This should definitely not be a problem on aarch64.

I don't think we need the compatible cds alignment here. This was added for x64 mainly to make it run correctly in rosetta IIRC. Macos aarch64 binaries will most likely run well in their native environment without this.

>> This seems to be used at line 1133 where 11.3.1 is hard-coded for x64. So I guess it works but I'm not very fond of this solution. @vidmik Why don't you put both x64 and aarch64 revisions here?
>
> As I understand it, the intention is to bump the Xcode version for x64 as well, though I must say I would prefer that change to go in separately.

To answer Magnus, on line 1133, we define a different dependency, called "lldb". That dependency is an older devkit which only purpose is to provide lldb for the failure handler when running tests. We need this because we are still running tests on Macos < 10.15, but the new devkit requires 10.15.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations

Kevin Rushforth-2
On Tue, 30 Mar 2021 12:34:50 GMT, Erik Joelsson <[hidden email]> wrote:

>> As I understand it, the intention is to bump the Xcode version for x64 as well, though I must say I would prefer that change to go in separately.
>
> To answer Magnus, on line 1133, we define a different dependency, called "lldb". That dependency is an older devkit which only purpose is to provide lldb for the failure handler when running tests. We need this because we are still running tests on Macos < 10.15, but the new devkit requires 10.15.

I agree that it would be ideal for the update to Xcode 12.x to be done separately. Is there a JBS issue filed for this update? Alternatively, you could use `/issue add nnnnnnn` to add that JBS issue to this PR.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations [v2]

Mikael Vidstedt
In reply to this post by Mikael Vidstedt
> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.

Mikael Vidstedt has updated the pull request incrementally with one additional commit since the last revision:

  Remove SETFILE override

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3258/files
  - new: https://git.openjdk.java.net/jdk/pull/3258/files/dbfe5dd7..6673a4f9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3258&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3258&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3258.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3258/head:pull/3258

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations [v2]

Mikael Vidstedt
In reply to this post by Erik Joelsson-2
On Tue, 30 Mar 2021 12:31:25 GMT, Erik Joelsson <[hidden email]> wrote:

>> I don't think we need to set SETFILE anymore. This used to be needed because the SetFile binary in older Xcode versions was 32 bit and Catalina no longer supports 32 bit apps. This should definitely not be a problem on aarch64.
>
> I don't think we need the compatible cds alignment here. This was added for x64 mainly to make it run correctly in rosetta IIRC. Macos aarch64 binaries will most likely run well in their native environment without this.

I'll drop the SETFILE override - I removed it, ran a test job and everything seems to work just as expected.

I agree with Erik that the CDS alignment shouldn't be an issue here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations [v2]

Mikael Vidstedt
In reply to this post by Kevin Rushforth-2
On Tue, 30 Mar 2021 14:42:22 GMT, Kevin Rushforth <[hidden email]> wrote:

>> To answer Magnus, on line 1133, we define a different dependency, called "lldb". That dependency is an older devkit which only purpose is to provide lldb for the failure handler when running tests. We need this because we are still running tests on Macos < 10.15, but the new devkit requires 10.15.
>
> I agree that it would be ideal for the update to Xcode 12.x to be done separately. Is there a JBS issue filed for this update? Alternatively, you could use `/issue add nnnnnnn` to add that JBS issue to this PR.

Thank you for catching the accidental Xcode update for macosx-x64. It was not my intention to include that as part of this change.

That said, right now it's looking highly likely that we will have bumped it for macosx-x64 before this PR is ready for integration anyway. Therefore, for now I will keep it as-is, but integration of this PR is effectively blocked until it has been resolved - either by making it conditional on the architecture or by the switch to "Xcode12.4+1.0" for macosx-x64 making it into mainline.

As Erik mentions, the new "lldb" dependency is needed to support running the failure handler+lldb when testing on older macOS versions which do not support Xcode 12.4. Assuming the switch to Xcode 12.4 as devkit for macosx-x64 makes it into mainline I believe the logic is correct.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations [v3]

Mikael Vidstedt
In reply to this post by Mikael Vidstedt
> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.

Mikael Vidstedt has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:

 - Merge branch 'master' into 8264224-macosx-aarch64
 - Remove SETFILE override
 - 8264224: Add macosx-aarch64 to Oracle build configurations

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3258/files
  - new: https://git.openjdk.java.net/jdk/pull/3258/files/6673a4f9..20fdddad

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3258&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3258&range=01-02

  Stats: 3451 lines in 160 files changed: 2380 ins; 555 del; 516 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3258.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3258/head:pull/3258

PR: https://git.openjdk.java.net/jdk/pull/3258
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264224: Add macosx-aarch64 to Oracle build configurations [v3]

Sergey Bylokhov-2
On Wed, 31 Mar 2021 19:02:46 GMT, Mikael Vidstedt <[hidden email]> wrote:

>> This adds the necessary macosx-aarch64 support to the build configurations at Oracle.
>
> Mikael Vidstedt has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
>  - Merge branch 'master' into 8264224-macosx-aarch64
>  - Remove SETFILE override
>  - 8264224: Add macosx-aarch64 to Oracle build configurations

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3258