Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test mac-os builds both on Intel and Apple M #415

Merged
merged 1 commit into from
May 28, 2024

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented May 27, 2024

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 27, 2024
@jerboaa
Copy link
Collaborator

jerboaa commented May 27, 2024

Looks like we need something like graalvm/mandrel#735 as well.

@zakkak
Copy link
Collaborator Author

zakkak commented May 27, 2024

Looks like we need something like graalvm/mandrel#735 as well.

Done in #416

@zakkak zakkak force-pushed the 2024-05-27-macos-matrix branch 3 times, most recently from 3e8f85d to 6f29620 Compare May 27, 2024 14:01
@zakkak zakkak requested a review from Karm May 27, 2024 14:29
@Karm
Copy link
Collaborator

Karm commented May 27, 2024

@zakkak I had to rename lib/static/darwin-arm64 to lib/static/darwin-aarch64 too 🤷‍♂️

working with 23.1 branch

@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

@zakkak I had to rename lib/static/darwin-arm64 to lib/static/darwin-aarch64 too 🤷‍♂️

working with 23.1 branch

That sounds like an upstream bug, shouldn't upstream use the same naming as OpenJDK?

Also any idea why the smoke tests seem to work without this renaming in this case?

@Karm
Copy link
Collaborator

Karm commented May 28, 2024

@zakkak

@zakkak I had to rename lib/static/darwin-arm64 to lib/static/darwin-aarch64 too 🤷‍♂️
working with 23.1 branch

That sounds like an upstream bug, shouldn't upstream use the same naming as OpenJDK?

I have seen these in the wild, JNI Java libs, projects, JDK, GraalVM, uname, arch etc.:

osx-aarch64
osx-arm
osx-arm64
mac-aarch64
mac-arm
mac-arm64
macos-aarch64
macos-arm
macos-arm64
darwin-aarch64
darwin-arm
darwin-arm64

The ISA is apparently interchangeably called both Aarch64 and ARM64. It seems that MacOS pretty consistently reports "arm64" though. Both via file command examining Mach-0 executables and with uname and arch.
The kernel is called "Darwin" in uname, but $(sw_vers -productName) tells you "macOS".

Temurin 21 uses "lib/static/darwin-arm64", although it names its tarballs "aarch64_mac".
GraalVM CE uses "lib/static/darwin-aarch64" and they call their tarballs "macos-aarch64".

Also any idea why the smoke tests seem to work without this renaming in this case?

My hunch is that this naming mayhem is so prevalent a problem that the GHA runners have some kind of aliasing in place to mitigate failing on such trivial naming steps? When I am done with the baremetal ones, I'll take a look.

@Karm
Copy link
Collaborator

Karm commented May 28, 2024

@jerboaa
@zakkak
What we need to agree on is whether we copy what Temurin does, both in naming the lib dir and in naming the tarballs,
or whether we copy what GraalVM CE does.

The one who suggests we solve this by creating a third naming scheme wins a t-shirt :)

@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

@jerboaa @zakkak What we need to agree on is whether we copy what Temurin does, both in naming the lib dir and in naming the tarballs, or whether we copy what GraalVM CE does.

The one who suggests we solve this by creating a third naming scheme wins a t-shirt :)

I would love to see the print on that T-shirt but I will be logical and not suggest a third naming scheme :)

Although I would like to go with the Temurin naming scheme I am afraid the easiest path is to go with the GraalVM CE naming scheme as I expect mx to expect/generate the names and I don't think it's worth the effort to try and detect where and how that happens. Furthermore, less deviation from upstream seems better to me in general (we could also ask upstream for the reasoning behind the different naming).

@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

Also any idea why the smoke tests seem to work without this renaming in this case?

It turns out it's still building for x86_64

C compiler: cc (apple, x86_64, 15.0.0)

https://github.com/graalvm/mandrel-packaging/actions/runs/9256122608/job/25461451554?pr=415#step:9:520

Switching to draft... I need to change more things (like the openjdk we download) and make sure rosetta is not taking over and emulating things.

@zakkak zakkak marked this pull request as draft May 28, 2024 09:28
@Karm
Copy link
Collaborator

Karm commented May 28, 2024

@zakkak Just to confirm, the build I tried was on a system without Rosetta, so it works, but for the naming....

@zakkak zakkak force-pushed the 2024-05-27-macos-matrix branch 5 times, most recently from 24d51ef to 8063836 Compare May 28, 2024 09:59
@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

@zakkak Just to confirm, the build I tried was on a system without Rosetta, so it works, but for the naming....

Yes, I fixed it in the github action now as well, but it looks like this renaming needs to be done by build.java instead. I'll give it some more thought and try to understand why GraalVM CE uses a different naming scheme.

Comment on lines +217 to +219
export ARCHIVE_NAME="mandrel-java23-darwin-${ARCH}-${MANDREL_VERSION_UNTIL_SPACE}.tar.gz"
mv ${ARCHIVE_NAME} mandrel-java23-darwin-${ARCH}.tar.gz
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cause (on mac x86_64):

INFO [build] Creating Archive mandrel-java23-darwin-amd64-10.9.8.7-dev.tar.gz
 mv: rename mandrel-java23-darwin-x64-10.9.8.7-dev.tar.gz to mandrel-java23-darwin-x64.tar.gz: No such file or directory
Error: Process completed with exit code 1.

@zakkak zakkak force-pushed the 2024-05-27-macos-matrix branch from 8063836 to 46f574e Compare May 28, 2024 11:23
@zakkak zakkak force-pushed the 2024-05-27-macos-matrix branch from 46f574e to 321ea92 Compare May 28, 2024 11:25
zakkak added a commit to zakkak/temurin-build that referenced this pull request May 28, 2024
This is a follow up to
adoptium#2725.

`uname -m` seems to return `arm64` on MacOS but the expected naming
seems to be `aarch64` ( which is consistend with linux and windows as
well) as observed in
graalvm/mandrel-packaging#415 (comment)
@zakkak
Copy link
Collaborator Author

zakkak commented May 28, 2024

That sounds like an upstream bug, shouldn't upstream use the same naming as OpenJDK?

So it looks like a Temurin issue after all. I opened adoptium/temurin-build#3827 to fix this.

@zakkak zakkak marked this pull request as ready for review May 28, 2024 12:26
@zakkak zakkak merged commit 2bec8c7 into graalvm:master May 28, 2024
11 checks passed
@zakkak zakkak deleted the 2024-05-27-macos-matrix branch May 28, 2024 13:01
zakkak added a commit to zakkak/temurin-build that referenced this pull request May 29, 2024
This is a follow up to
adoptium#2725.

`uname -m` seems to return `arm64` on MacOS but the expected naming
seems to be `aarch64` ( which is consistend with linux and windows as
well) as observed in
graalvm/mandrel-packaging#415 (comment)
zakkak added a commit to zakkak/temurin-build that referenced this pull request May 30, 2024
This is a follow up to
adoptium#2725.

`uname -m` seems to return `arm64` on MacOS but the expected naming
seems to be `aarch64` ( which is consistend with linux and windows as
well) as observed in
graalvm/mandrel-packaging#415 (comment)
karianna added a commit to adoptium/temurin-build that referenced this pull request Jun 1, 2024
This is a follow up to
#2725.

`uname -m` seems to return `arm64` on MacOS but the expected naming
seems to be `aarch64` ( which is consistend with linux and windows as
well) as observed in
graalvm/mandrel-packaging#415 (comment)

Co-authored-by: Martijn Verburg <martijnverburg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants