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

Improve building Homebrew*.pkg #16077

Merged
merged 1 commit into from Oct 4, 2023
Merged

Improve building Homebrew*.pkg #16077

merged 1 commit into from Oct 4, 2023

Conversation

MikeMcQuaid
Copy link
Member

  • split jobs into build/test/deploy
  • test package on both macOS Intel and Apple Silicon
  • cleanup some argument handling
  • use HOMEBREW_MACOS_OLDEST_SUPPORTED naming to be consistent with brew.sh
  • note in brew.sh that Distribution.xml also needs updated (and do so)
  • various other little bits of style cleanup

- split jobs into build/test/deploy
- test package on both macOS Intel and Apple Silicon
- cleanup some argument handling
- use `HOMEBREW_MACOS_OLDEST_SUPPORTED` naming to be consistent with
  `brew.sh`
- note in `brew.sh` that `Distribution.xml` also needs updated (and do
  so)
- various other little bits of style cleanup
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Oct 4, 2023
@MikeMcQuaid
Copy link
Member Author

Marking as critical to unblock and be able to merge merge conflicts in #16073.

@MikeMcQuaid MikeMcQuaid merged commit f61e4ee into master Oct 4, 2023
31 checks passed
@MikeMcQuaid MikeMcQuaid deleted the build_pkg_improvements branch October 4, 2023 09:34
.github/workflows/pkg-installer.yml Show resolved Hide resolved
Library/Homebrew/brew.sh Show resolved Hide resolved
@@ -27,7 +27,7 @@
<license file="LICENSE.rtf"/>
<conclusion file="CONCLUSION.rtf" />
<allowed-os-versions>
<os-version min="11.0"/>
<os-version min="12.0.0"/>
Copy link
Member

@Bo98 Bo98 Oct 4, 2023

Choose a reason for hiding this comment

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

I think it's worth investigating whether these are necessary, or if the pkgbuild --min-os-version is sufficient. Might fail at different times of the install process, or simply not at all, so worth comparing by setting to 15.0 or whatever.

If it is needed: maybe make this like an .in file and make this @@HOMEBREW_MACOS_OLDEST_SUPPORTED@@.0 and do sed in the workflow. I do similar things in the ci-orchestrator deploy workflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd really like to avoid a .in flow here for a number that's changed once a year.

I'm game for you to remove this if you verify --min-os-version is sufficient.

git -C /usr/local/Homebrew checkout --force master
else
mkdir -vp /usr/local/bin
mv "${homebrew_directory}" "/usr/local/Homebrew/"
Copy link
Member

Choose a reason for hiding this comment

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

This has different behaviour if /usr/local/Homebrew/ is already created, which could happen if the /usr/local/bin/brew was missing (e.g. failed previous install or improper uninstall).

I'd consider changing the -f "/usr/local/bin/brew" && -d "/usr/local/Homebrew" to just -d "/usr/local/Homebrew" and consider bin/brew separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

(e.g. failed previous install or improper uninstall).

If we're in this state: this installer is practically guaranteed to break.

This change was made because the cp -a command was no longer reliable and was failing consistently on reinstall. I'm game to iterate on what this is here but I'm personally not gonna spend much more time on this unless we get user reports.

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants