-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
unixPb: Add role to install intel homebrew for arm64 macs #3185
Conversation
Tested. Works as expected. Ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Going forward, any brew install ansible task which intends to run on arm64 mac needs to have its path specified to |
If this gets merged now AWX will run the changes on our build machines, so i recommend it gets merged only after the coming release |
Why do we need Intel brew on Arm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A block has been put on this Pull Request as this repository is temporarily under a code freeze due to an ongoing release cycle.
If this pull request needs to be merged during the release cycle then please comment /merge
and a PMC member will be able to remove the block.
If the code freeze is over you can remove this block by commenting /thaw
.
@karianna arm64 macs need the intel library See #2536 (comment) |
/thaw |
Pull Request unblocked - code freeze is over.
Potentially related for a future change: adoptium/temurin-build#3493 (comment) |
ansible/playbooks/AdoptOpenJDK_Unix_Playbook/roles/Common/tasks/MacOSX.yml
Show resolved
Hide resolved
OK, so just checks to go green and this is good |
when: not intel_homebrew_installed.stat.exists | ||
|
||
- name: Install Intel libpng | ||
ansible.builtin.shell: arch -x86_64 yes | /usr/local/Homebrew/bin/brew install libpng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is presumably the bit that caused the problems from #3231 so I'm going to block this change on the basis that we presumably fix this PR before it goes live.
As per adoptium/temurin-build#3509 (comment), we do not need libpng installed (either arm64 or intel) on our build machines to be able to cross compile x64 mac jdk8 which means we do not need intel homebrew on our arm64 macs |
@@ -162,6 +180,14 @@ | |||
when: not pdfwriter.stat.exists | |||
tags: jck_tools | |||
|
|||
- name: Uninstall libpng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1
on this as I don't want end-users who use our playbooks to have things removed from their machine. Suggest we do this as a one off on all of our machines unless there is a specific reason for this to be in the playbooks. If it has to be, let's tag it with adoptopenjdk
.
Also bear in mind that we have a bug which is causing libpng to be picked up even when we tell it not to, which is not ideal :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Now tagged with adoptopenjdk so objection removed but we should plan to remove this in the future if we can)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think this is good to go in now :-) Thanks for the changes.
)" This reverts commit bc2a40a.
ref #2536 (comment)
Need to test before merging