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

Revert "Revert "Split prof gems into their own group"" #15183

Merged
merged 3 commits into from Apr 11, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Apr 10, 2023

Reland this so we can release a new portable Ruby, this time fixing brew typecheck --update to install all gems.

Obsoletes the fix in #15134 so we shouldn't need to reland that.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good so far.

@@ -12,6 +12,8 @@ module Homebrew
# After updating this, run `brew vendor-gems --update=--bundler`.
HOMEBREW_BUNDLER_VERSION = "2.3.26"

VALID_GEM_GROUPS = ["sorbet", "prof"].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Could this be read from the Gemfile instead of being hardcoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will have a look and see if there's a public API for that.

Copy link
Member

Choose a reason for hiding this comment

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

We already use some Gem methods so should be doable.

@@ -45,10 +45,11 @@ def self.typecheck_args
def self.typecheck
args = typecheck_args.parse

Homebrew.install_bundler_gems!(groups: ["sorbet"])
update = args.update? || args.update_all?
Homebrew.install_bundler_gems!(groups: update ? VALID_GEM_GROUPS : ["sorbet"])
Copy link
Member

Choose a reason for hiding this comment

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

Could this get split out into a named variable? A bit hard to parse as-is.

Library/Homebrew/dev-cmd/vendor-gems.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/typecheck.rb Show resolved Hide resolved
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great thanks!

@Bo98 Bo98 merged commit 448bf1a into Homebrew:master Apr 11, 2023
26 checks passed
@Bo98 Bo98 deleted the prof-group2 branch April 11, 2023 14:12
@github-actions github-actions bot added the outdated PR was locked due to age label May 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants