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

🔖 Release 1.8.7 #506

Open
wants to merge 166 commits into
base: main
Choose a base branch
from
Open

🔖 Release 1.8.7 #506

wants to merge 166 commits into from

Conversation

phatblat
Copy link
Member

@phatblat phatblat commented Nov 25, 2023

Release branch for 1.8.7.

This branch includes the new release workflow #510.

@phatblat phatblat added 🧽 chore Administrative task: documentation, build, test, release, git, etc. 🍺 homebrew labels Nov 25, 2023
@phatblat phatblat self-assigned this Nov 25, 2023
script/bottle Outdated Show resolved Hide resolved
script/bottle Outdated Show resolved Hide resolved
@phatblat
Copy link
Member Author

I've published a build from this branch on the releases page:
https://github.com/mas-cli/mas/releases/tag/v1.8.7-beta.1

@rgoldberg
Copy link
Contributor

rgoldberg commented Jan 1, 2024

@phatblat

Not sure if you saw my comment:

#505 (comment)

I think testing & releasing #496 or #503 (the 1-line code change is exactly the same in both of them) should solve the main problem of iOS app versions showing up as Mac app versions, while 1.8.7 beta 1 didn't fix it for me (on macOS 12.7.x on an Intel).

Let me know if I can help get some or all of the fixes out.

Happy new year.

Thanks.

@phatblat
Copy link
Member Author

phatblat commented Nov 3, 2024

The release run for v1.8.7-beta.51 was successful, skipping the homebrew-core job since this was a pre-release.

@rgoldberg I resolved the lint errors while waiting on release builds.

I'm going to kick off the real v1.8.7 release and see how this goes 🤞🏻

@rgoldberg
Copy link
Contributor

@phatblat I don't see any PR (open or closed) for mas 1.8.7 in the Homebrew core repo.

I assume the formula updates are done via a PR.

I see the PR in the mas tap. For some reason, it's marked as a WIP. I assume it should be switched to be a normal PR.

@toobuntu
Copy link

toobuntu commented Nov 3, 2024

perhaps the brew audit command isn't aware of the audit_exceptions/github_prerelease_allowlist.json file.

This file path was added here the mas repo, but it belongs in the local tap. Even there, it would apply only to the formula in that tap but not the official formula in homebrew-core. For that, you would have to add the exception to https://github.com/Homebrew/homebrew-core/blob/master/audit_exceptions/github_prerelease_allowlist.json.

Also, my understanding is that homebrew uses the brew bump command as a frontend for both brew bump-formula-pr and brew bump-cask-pr.

@phatblat
Copy link
Member Author

phatblat commented Nov 3, 2024

@phatblat I don't see any PR (open or closed) for mas 1.8.7 in the Homebrew core repo.

I assume the formula updates are done via a PR.

The PR for the new version didn't work. I'll investigate and retry.

I see the PR in the mas tap. For some reason, it's marked as a WIP. I assume it should be switched to be a normal PR.

Yeah, it should but I forgot I left --draft in the gh pr create command. I'll fix that.

@phatblat
Copy link
Member Author

phatblat commented Nov 3, 2024

The release failed again. Looks like the issue was with the --fork-org mas-cli option.

+ brew bump-formula-pr --tag=v1.8.7 --revision=4405807010987802c0967bbf349c08808062b824 --strict --verbose --online --no-browse --fork-org mas-cli mas
/opt/homebrew/Library/Taps/homebrew/homebrew-core ~/work/mas/mas
Updating homebrew/core formula with a PR
/opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.3.5/bin/bundle clean
==> replace /tag:(\s+")v1.8.6(?=")/ with "tag:\\1v1.8.7\\2"
==> replace "560c89af2c1fdf0da9982a085e19bb6f5f9ad2d0" with "4405807010987802c0967bbf349c08808062b824"
Error: Unable to fork: GitHub API Error: Must have admin rights to Repository.
HOMEBREW_GITHUB_API_TOKEN may be invalid or expired; check:
  https://github.com/settings/tokens
!
Error: Process completed with exit code 1.

@toobuntu
Copy link

toobuntu commented Nov 3, 2024

In mas-cli/homebrew-tap#45 is the vv correct?

-    root_url "https://github.com/mas-cli/mas/releases/download/v1.8.7-beta.1"
+    root_url "https://github.com/mas-cli/mas/releases/download/vv1.8.7"

@phatblat
Copy link
Member Author

phatblat commented Nov 3, 2024

In mas-cli/homebrew-tap#45 is the vv correct?

Nope. Thanks for calling this out, @toobuntu!

@phatblat
Copy link
Member Author

phatblat commented Nov 3, 2024

Got a little bit further by adding repo the scopes on the PAT I created for the HOMEBREW_GITHUB_API_TOKEN secret.

 brew bump-formula-pr --tag=v1.8.7 --revision=4405807010987802c0967bbf349c08808062b824 --strict --verbose --online --no-browse --fork-org mas-cli mas
/opt/homebrew/Library/Taps/homebrew/homebrew-core ~/work/mas/mas
Updating homebrew/core formula with a PR

/opt/homebrew/Library/Homebrew/vendor/portable-ruby/3.3.5/bin/bundle clean
==> replace /tag:(\s+")v1.8.6(?=")/ with "tag:\\1v1.8.7\\2"
==> replace "560c89af2c1fdf0da9982a085e19bb6f5f9ad2d0" with "4405807010987802c0967bbf349c08808062b824"
git add /opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/m/mas.rb
git checkout --no-track -b bump-mas-1.8.7 origin/master
Switched to a new branch 'bump-mas-1.8.7'
M	Formula/m/mas.rb
git commit --no-edit --verbose --message=mas 1.8.7 -- /opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/m/mas.rb
[bump-mas-1.8.7 2dfea1852ff] mas 1.8.7
 1 file changed, 2 insertions(+), 2 deletions(-)
remote: 
remote: Create a pull request for 'bump-mas-1.8.7' on GitHub by visiting:        
remote:      https://github.com/mas-cli/homebrew-core-1/pull/new/bump-mas-1.8.7        
remote: 
To https://github.com/mas-cli/homebrew-core-1.git
 * [new branch]              bump-mas-1.8.7 -> bump-mas-1.8.7
branch 'bump-mas-1.8.7' set up to track '***github.com/mas-cli/homebrew-core-1.git/bump-mas-1.8.7'.
git checkout --quiet -
Error: Unable to open pull request: Validation Failed: [{"resource"=>"PullRequest", "field"=>"head", "code"=>"invalid"}]!
Error: Process completed with exit code 1.

@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@mas-cli mas-cli deleted a comment from houndci-bot Nov 3, 2024
@MikeMcQuaid MikeMcQuaid mentioned this pull request Nov 4, 2024
Copy link
Contributor

@rgoldberg rgoldberg left a comment

Choose a reason for hiding this comment

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

I should investigate release.yml more, but I might not have time to do too much more with with now, so I figured I'd submit my review. I might change/add a few comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call script/bottle instead of this script?

Copy link
Contributor

Choose a reason for hiding this comment

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

This script should probably be renamed generate_brew_formula or brew_formula.

By default, it should generate the core formula, but accept an optional --tap to generate the tap formula instead.

Generating the Package.swift should be a separate script (right now, that can be done via script/version --write, but maybe we should move that functionality to a new script/generate_package_swift or script/package_swift script to keep everything modular).

Instead of always modifying an existing formula on the file system (Homebrew/mas.rb or Homebrew/mas-tap.rb), it should generate one script at a time from scratch, printing the contents to stdout, with the caller redirecting that to a file.

When our release process calls this script, each output formula should be redirected to a file under a git-ignored directory (probably just make some directory under .build/).

This will allow us to remove Homebrew/mas.rb & Homebrew/mas-tap.rb from the repo, which will prevent accidental check ins of different versioned formulae in the repo or version mismatches between varios files, git tags, or released artifacts.

This script shouldn't accept a revision. The revision should be obtained from version tag. Otherwise, things can get out of sync.

It will also make everything more modular. You should be able to generate one output by itself instead of being locking into generating all 3. You should be able to just see the output instead of needing to write a file. You should be able to write output wherever you want, instead of being constrained by what's in the script.

Also, a general note: I've normally seen only seen environment variable name in upper case, with other variables in lower case.

exit 1
fi

LOCAL_MAS_FORMULA_PATH="Homebrew/mas.rb"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename LOCAL_MAS_FORMULA_PATH as LOCAL_CORE_FORMULA_PATH. Both this and LOCAL_TAP_FORMULA_PATH are for mas, the distinguishing difference is one is for core, the other for tap.

Suggested change
LOCAL_MAS_FORMULA_PATH="Homebrew/mas.rb"
LOCAL_CORE_FORMULA_PATH="Homebrew/mas.rb"

Comment on lines +58 to +60
sd '( +tag: +)"[^"]+"' "\$1\"${MAS_VERSION}\"" "${file}"
sd '( +revision: +)"[^"]+"' "\$1\"${REVISION}\"" "${file}"
sd '( +root_url "https://github.com/mas-cli/mas/releases/download/).+' "\${1}${MAS_VERSION}\"" "${file}"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we generate output from a script instead of modifying files on the file system (as per my comment on this file as a whole), we can get rid of the sd dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this from git. Generate the formula as per my file comment on script/version_bump.

script/bootstrap -f

- name: 🔧 Configure Git Author
if: env.PRE_RELEASE == 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: env.PRE_RELEASE == 'false'

ref: ${{ needs.start.outputs.release_branch }}

- name: 👢 Bootstrap
if: env.PRE_RELEASE == 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: env.PRE_RELEASE == 'false'

PRE_RELEASE: ${{ needs.start.outputs.pre_release }}
steps:
- uses: actions/checkout@v4
if: env.PRE_RELEASE == 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: env.PRE_RELEASE == 'false'

Comment on lines +224 to +225
env:
PRE_RELEASE: ${{ needs.start.outputs.pre_release }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this syntax will work.

Suggested change
env:
PRE_RELEASE: ${{ needs.start.outputs.pre_release }}
if: ${{ needs.start.outputs.pre_release }} == 'false'

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 suspect it will.

# Error: No available formula or cask with the name "mas-cli/tap/mas".
- name: 🚰 Checkout mas tap
run: |
rm -rf /opt/homebrew/Library/Taps
Copy link
Contributor

Choose a reason for hiding this comment

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

/opt/homebrew should be replaced everywhere by "$(brew --prefix)", just in case someone on Intel uses act to test actions locally, or in case brew uses some other prefix in the future.


- id: dry_run
run: |
echo "DRY_RUN=false" >>"$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is $GITHUB_OUTPUT used? the linked documentation above uses "${GITHUB_ENV}".

Suggested change
echo "DRY_RUN=false" >>"$GITHUB_OUTPUT"
echo "DRY_RUN=false" >>"${GITHUB_ENV}"


- id: mas_version
run: |
echo "MAS_VERSION=${{ github.event.release.tag_name }}" >>"$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Suggested change
echo "MAS_VERSION=${{ github.event.release.tag_name }}" >>"$GITHUB_OUTPUT"
echo "MAS_VERSION=${{ github.event.release.tag_name }}" >>"${GITHUB_ENV}"


- id: pre_release
run: |
echo "PRE_RELEASE=$(grep -q '-' <<<$MAS_VERSION && echo 'true' || echo 'false')" >>"$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Suggested change
echo "PRE_RELEASE=$(grep -q '-' <<<$MAS_VERSION && echo 'true' || echo 'false')" >>"$GITHUB_OUTPUT"
echo "PRE_RELEASE=$(grep -q '-' <<<$MAS_VERSION && echo 'true' || echo 'false')" >>"${GITHUB_ENV}"


- id: release_branch
run: |
echo "RELEASE_BRANCH=releases/release-${{ github.event.release.tag_name }}" >>"$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

Suggested change
echo "RELEASE_BRANCH=releases/release-${{ github.event.release.tag_name }}" >>"$GITHUB_OUTPUT"
echo "RELEASE_BRANCH=releases/release-${{ github.event.release.tag_name }}" >>"${GITHUB_ENV}"

@rgoldberg
Copy link
Contributor

@phatblat Probably simplest to just accept this PR to merge it into main, then to open individual PRs to make changes, instead of fixing in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧽 chore Administrative task: documentation, build, test, release, git, etc. 🍺 homebrew
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants