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

unixPb: Add role to install intel homebrew for arm64 macs #3185

Merged
merged 11 commits into from
Nov 6, 2023

Conversation

Haroon-Khel
Copy link
Contributor

@Haroon-Khel Haroon-Khel commented Sep 12, 2023

  • commit message has one of the standard prefixes
  • faq.md updated if appropriate
  • other documentation is changed or added (if applicable)
  • playbook changes run through VPC or QPC (if you have access)
  • VPC/QPC not applicable for this PR
  • for inventory.yml changes, bastillion/nagios/jenkins updated accordingly

ref #2536 (comment)

Need to test before merging

@Haroon-Khel
Copy link
Contributor Author

Tested. Works as expected. Ready to merge

Copy link
Contributor

@steelhead31 steelhead31 left a comment

Choose a reason for hiding this comment

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

LGTM

@Haroon-Khel
Copy link
Contributor Author

Haroon-Khel commented Sep 14, 2023

Going forward, any brew install ansible task which intends to run on arm64 mac needs to have its path specified to /opt/homebrew/bin seeing as we will now have both intel and arm64 homebrew on our arm64 mac machines

@Haroon-Khel
Copy link
Contributor Author

Haroon-Khel commented Sep 15, 2023

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

@karianna
Copy link
Contributor

Why do we need Intel brew on Arm?

Copy link

@github-actions github-actions bot left a 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.

@Haroon-Khel
Copy link
Contributor Author

@karianna arm64 macs need the intel library libpng to be able to compile freetype for x64 jdk8 which is installed using intel homebrew

See #2536 (comment)

@Haroon-Khel
Copy link
Contributor Author

/thaw

@github-actions github-actions bot dismissed their stale review October 4, 2023 11:30

Pull Request unblocked - code freeze is over.

@Haroon-Khel Haroon-Khel requested a review from sxa October 4, 2023 11:30
@sxa
Copy link
Member

sxa commented Oct 4, 2023

@karianna arm64 macs need the intel library libpng to be able to compile freetype for x64 jdk8 which is installed using intel homebrew

Potentially related for a future change: adoptium/temurin-build#3493 (comment)

@karianna
Copy link
Contributor

karianna commented Oct 5, 2023

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
Copy link
Member

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.

@Haroon-Khel
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Member

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)

Copy link
Member

@sxa sxa left a 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.

@Haroon-Khel Haroon-Khel merged commit bc2a40a into adoptium:master Nov 6, 2023
7 of 9 checks passed
Haroon-Khel added a commit that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants