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

Add flag --overwrite to brew install to govern the keg-linking step #12691

Merged

Conversation

boblail
Copy link
Contributor

@boblail boblail commented Jan 10, 2022

Allows you to avoid the Keg::ConflictError recommending that you invoke brew link --overwrite in scenarios when you know that that's how you'd proceed anyway.

  • 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?

I didn't see a test in formula_spec.rb that exercised the Keg::ConflictError error — that I could copy and adapt. keg_spec.rb already covers the logic of the overwrite option.

I validated this change by doing:

brew uninstall git
touch /usr/local/bin/git
brew install --overwrite git

Allows you to avoid the `Keg::ConflictError` recommending that you invoke `brew link --overwrite` in scenarios when you know that that's how you'd proceed anyway.
@boblail boblail force-pushed the lail/add-overwrite-flag-to-brew-install branch from e8a6fa5 to 9b678c3 Compare January 10, 2022 20:24
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.

Makes sense to me. Can you explain the case where you know there will be conflicts that you want to overwrite?

Library/Homebrew/cmd/install.rb Outdated Show resolved Hide resolved
@boblail
Copy link
Contributor Author

boblail commented Jan 11, 2022

Makes sense to me. Can you explain the case where you know there will be conflicts that you want to overwrite?

Sure!

For the most part, Square engineers can install what they like on their laptops; but we have a Bootstrap script that ensures (forces) you to have Homebrew, Homebrew-installed Git, some SSH configs, and some system Git configs. Currently, we upsert those system Git configs into $(brew --prefix)/etc/gitconfig — and that's why we expect git to be installed by Homebrew.

About 0.5% of engineers running this Bootstrap script have /usr/local/bin/git symlinked elsewhere. After troubleshooting several of those cases manually, the resolution has consistently been to run brew link --overwrite so now we're just looking to move that fix into Bootstrap.

@MikeMcQuaid
Copy link
Member

About 0.5% of engineers running this Bootstrap script have /usr/local/bin/git symlinked elsewhere. After troubleshooting several of those cases manually, the resolution has consistently been to run brew link --overwrite so now we're just looking to move that fix into Bootstrap.

Hmm, a couple of questions from this:

  • so you're not actually sure there's something to overwrite, this is just a typical enough debugging step that you want to do it unconditionally?
  • why does your Bootstrap script not call brew link --overwrite instead?
  • have you investigated using brew bundle for this instead? It supports the behaviour you seem to want here

@boblail
Copy link
Contributor Author

boblail commented Jan 11, 2022

  • so you're not actually sure there's something to overwrite, this is just a typical enough debugging step that you want to do it unconditionally?

In most cases there won't be something to overwrite. When there is, we would like to.

  • why does your Bootstrap script not call brew link --overwrite instead?

It could!

It's a brittler API, though. We'd be scraping the output from brew install to tease out the case when we need to run brew link --overwrite from other failures.

On the other hand, supporting --overwrite in brew install allows us to express our policy to Homebrew rather than writing a procedure for handling a specific error case.

Both work, but — if this PR lands — the latter's cleaner for us 😄

  • have you investigated using brew bundle for this instead? It supports the behaviour you seem to want here

We did! We started with brew bundle and we departed from it in order to get finer-grained control of the output (we show a spinner for each step, hide output on success, and display only the output relevant to a given step on failure) and of exception-reporting (e.g. we don't have to scrape output to make distinct exception reports for different steps).

@MikeMcQuaid
Copy link
Member

Cool, thanks for explaining. I'm going to pause this for now; personally I feel like this is stepping into brew bundle territory, particularly given this is already supported there, but I'm open to input from other maintainers who disagree.

@boblail
Copy link
Contributor Author

boblail commented Jan 12, 2022

Hm. I hadn't thought of brew and brew bundle as competing. I figured brew was a low-level imperative API and brew bundle was a high-level declarative API layered on top.

But (I might be mistaken) I don't think brew bundle currently supports this feature. If you do something like this:

brew uninstall git
touch "$(brew --prefix)/bin/git"

echo 'brew "homebrew/core/git", link: true' > Brewfile
brew bundle

you will still get the error:

Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/git
Target /usr/local/bin/git
already exists. You may want to remove it:
  rm '/usr/local/bin/git'

To force the link and overwrite all conflicting files:
  brew link --overwrite git

To list all files that would be deleted:
  brew link --overwrite --dry-run git

Possible conflicting files are:
/usr/local/bin/git

It looks like link: true in a Brewfile governs the --force flag, allowing keg-only formulae to be linked; but there doesn't appear to be a way to express the --overwrite flag in a Brewfile. 🤔

Would it make sense to add support for this second flag in both places?

@MikeMcQuaid
Copy link
Member

It looks like link: true in a Brewfile governs the --force flag, allowing keg-only formulae to be linked; but there doesn't appear to be a way to express the --overwrite flag in a Brewfile. 🤔

Gotcha, my mistake then and I'm happy to merge.

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 again @boblail!

@MikeMcQuaid MikeMcQuaid merged commit f11a752 into Homebrew:master Jan 13, 2022
@boblail
Copy link
Contributor Author

boblail commented Jan 13, 2022

Thanks, @MikeMcQuaid! I appreciate that you maintain Homebrew thoughtfully and critically — the project is undoubtedly better for it!

@MikeMcQuaid
Copy link
Member

@boblail 😊 thanks for the kind words!

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2022
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

2 participants