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

[master] Allow Homebrew package manager in all systems but Windows #66609

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdalvaro
Copy link
Contributor

@cdalvaro cdalvaro commented Jun 1, 2024

What does this PR do?

This PR removes the restriction for using mac_brew_pkg module only on macOS systems, since Homebrew can be run in Linux too: https://docs.brew.sh/Homebrew-on-Linux

I think that the module could be renamed to brew_pkg instead of mac_brew_pkg. But I need some help here to understand what changes are needed for this to be made.

Merge requirements satisfied?

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices, including the
PR Guidelines.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@cdalvaro cdalvaro requested a review from a team as a code owner June 1, 2024 17:37
@cdalvaro cdalvaro requested a review from dwoz June 1, 2024 17:37
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title feat(mac_brew_pkg): Allow homebrew in all systems [master] feat(mac_brew_pkg): Allow homebrew in all systems Jun 1, 2024
@cdalvaro cdalvaro force-pushed the feature/allow_homebrew_in_all_systems branch 2 times, most recently from 4c49cdc to 389dd3c Compare June 2, 2024 08:49
@cdalvaro cdalvaro changed the title [master] feat(mac_brew_pkg): Allow homebrew in all systems [master] Allow Homebrew package manager in all systems but Windows Jun 3, 2024
@cdalvaro cdalvaro changed the title [master] Allow Homebrew package manager in all systems but Windows [master] WIP: Allow Homebrew package manager in all systems but Windows Jun 3, 2024
@cdalvaro cdalvaro force-pushed the feature/allow_homebrew_in_all_systems branch from cda68ec to 64e7a45 Compare June 3, 2024 15:33
@cdalvaro cdalvaro changed the title [master] WIP: Allow Homebrew package manager in all systems but Windows [master] Allow Homebrew package manager in all systems but Windows Jun 3, 2024
@@ -231,6 +231,14 @@ def is_aarch64():
return platform.machine().startswith("aarch64")


@real_memoize
def is_arm64():
Copy link
Contributor Author

@cdalvaro cdalvaro Jun 4, 2024

Choose a reason for hiding this comment

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

Not really sure about adding this new method instead of modifying the existing aarch64 with something like this:

return re.search(r'^(aarch|arm)64', platform.machine()) is not None

I think my initial approach is more conservative, since I'm not modifying the current behavior of the is_aarch64 method. But essentially, Aarch64 and ARM64 are two different ways to refer the same thing.

TL;DR: Aarch64 is how the LLVM community refers to this architecture while Apple calls it ARM64.

Copy link
Contributor Author

@cdalvaro cdalvaro Jun 4, 2024

Choose a reason for hiding this comment

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

I would like to ask the opinion of @s0undt3ch and @krionbsd. Since Kirill implemented the original is_aarch64 method and Pedro is one of the main developers for macOS support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant