Skip to content

bundle/commands/exec: check that Brewfile is installed with --check #19637

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

bundle/commands/exec: check that Brewfile is installed with --check #19637

wants to merge 2 commits into from
+46 −29

Conversation

carlocab
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

brew bundle exec behaves correctly only after doing brew bundle install.

Running brew bundle check can be slow, so let's add a --check flag
to brew bundle exec which will also run brew bundle check before
brew bundle exec to ensure that the Brewfile has been installed
before proceeding.

Alternative to #19636 which makes --check the default behaviour. I have a mild
preference for this version since the additional brew bundle check makes
brew bundle env and brew bundle exec true 60% slower.

Another option is to merge #19636 while adding a --no-check flag for users who
already know their Brewfile is installed and don't want to incur the overhead
of brew bundle check.

@carlocab
Copy link
Member Author

carlocab commented Mar 28, 2025 •
edited
Loading

CC @Homebrew/maintainers, @jacobbednarz, @colindean for thoughts on this vs #19636

@carlocab carlocab force-pushed the bundle-exec-with-optional-check branch from 40d385d to b1e4f09 Compare March 28, 2025 05:59
@carlocab carlocab self-assigned this Mar 28, 2025
@carlocab carlocab force-pushed the bundle-exec-with-optional-check branch 2 times, most recently from 4c9a304 to e3ea5e9 Compare March 28, 2025 06:10
MikeMcQuaid
MikeMcQuaid approved these changes Mar 28, 2025
View reviewed changes
Library/Homebrew/cmd/bundle.rb
@@ -273,6 +260,28 @@ def run
require "bundle/commands/remove"
Homebrew::Bundle::Commands::Remove.run(*named_args, type: selected_types.first, global:, file:)
end
when *BUNDLE_EXEC_COMMANDS
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to not move this so the diff is easier to follow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was moved by brew style --fix 😅

Copy link
Member

Choose a reason for hiding this comment

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

@carlocab oh no! will review harder.

Copy link
Member Author

@carlocab carlocab Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Split out the changes from brew style --fix so you can review b20a908 for the substantive changes.

@MikeMcQuaid
Copy link
Member

CC @Homebrew/maintainers, @jacobbednarz, @colindean for thoughts on this vs #19636

@carlocab and I chatted privately about the performance differences and, given that #19636 is a fairly major performance regression: I'm 👍🏻 on this approach.

Thanks for opening PRs to compare both, though ❤️

@MikeMcQuaid MikeMcQuaid mentioned this pull request Mar 28, 2025
Draft
7 tasks
@carlocab carlocab force-pushed the bundle-exec-with-optional-check branch 2 times, most recently from d0768ca to 971440d Compare March 28, 2025 09:23
carlocab added 2 commits March 28, 2025 17:25
@carlocab
bundle/commands/exec: check that Brewfile is installed with --check
b20a908 
`brew bundle exec` behaves correctly only after doing `brew bundle install`.

Running `brew bundle check` can be slow, so let's add a `--check` flag
to `brew bundle exec` which will also run `brew bundle check` before
`brew bundle exec` to ensure that the `Brewfile` has been installed
before proceeding.
@carlocab
brew style --fix
b68987e
@carlocab carlocab force-pushed the bundle-exec-with-optional-check branch from 971440d to b68987e Compare March 28, 2025 09:25
MikeMcQuaid
MikeMcQuaid approved these changes Mar 28, 2025
View reviewed changes
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 for making this easier to review! Would be great to have some more test coverage here but otherwise 🎉 ✅

colindean
colindean reviewed Mar 29, 2025
View reviewed changes
Copy link
Contributor

@colindean colindean left a comment

Choose a reason for hiding this comment

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

LGTM on a visual check. I'd prefer the behavior here to that in #19636.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colindean colindean colindean left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@carlocab carlocab

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carlocab @MikeMcQuaid @colindean