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

brew.rb: tell users to fix head issues with inreplace #13623

Conversation

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

WIP to ensure not only build command failures tell users of --HEAD to make a pull request

@BrewTestBot
Copy link
Member

Review period will end on 2022-08-02 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 30, 2022
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.

Good idea 👍🏻

@Bo98
Copy link
Member

Bo98 commented Aug 1, 2022

Not convinced on making a parent FormulaError considering inreplace is not formula specific (it's used in bump-cask-pr etc. too).

Probably best to keep the error as it was and either:

  • just add a generic object variable, or;
  • have Formula-specific inreplace and inreplace_pairs methods which call super but catches the error and converts it to a BuildError or something similar. This would allow a lot of BuildError-specific logic to be reused and also does not affect how errors are handled in non-formula contexts.

@carlocab
Copy link
Member

carlocab commented Aug 1, 2022

Yea, catching the inreplace error throwing a BuildError seems much nicer.

@SMillerDev
Copy link
Member Author

And that's why it was a draft, thanks @Bo98. I'll check that later

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 2, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@SMillerDev SMillerDev force-pushed the fix/exceptions/report_issues_inreplace_head branch from 22c3230 to 4a95d09 Compare August 7, 2022 11:26
@SMillerDev SMillerDev marked this pull request as ready for review August 7, 2022 11:26
@SMillerDev
Copy link
Member Author

A lot later, but it's done now.

@SMillerDev SMillerDev force-pushed the fix/exceptions/report_issues_inreplace_head branch from 4a95d09 to 03a489b Compare August 7, 2022 11:35
Library/Homebrew/formula.rb Show resolved Hide resolved
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Fantastic. This is exactly how I hoped it would look.

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.

Tiny style suggestion but: looks good to me!

Library/Homebrew/utils/inreplace.rb Outdated Show resolved Hide resolved
Library/Homebrew/utils/inreplace.rb Outdated Show resolved Hide resolved
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@SMillerDev SMillerDev merged commit 8e49be5 into Homebrew:master Aug 8, 2022
@bayandin
Copy link
Member

bayandin commented Aug 9, 2022

It looks like something went wrong, see Homebrew/homebrew-core#107575:

There's no error in the install block, but a problem with inreplace appears in ==> ENV block

 ==> ENV
  /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/build_environment.rb:60:in `keys': undefined method `keys' for nil:NilClass (NoMethodError)
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/build_environment.rb:65:in `dump'
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/exceptions.rb:505:in `dump'
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:148:in `rescue in <main>'
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/brew.rb:136:in `<main>'
  /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formula.rb:2222:in `rescue in inreplace': Failed executing: inreplace configure (BuildError)
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/formula.rb:2219:in `inreplace'
  	from /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/nvc.rb:44:in `install'

https://github.com/Homebrew/homebrew-core/runs/7729673651?check_suite_focus=true#step:7:93

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
@SMillerDev SMillerDev deleted the fix/exceptions/report_issues_inreplace_head branch September 21, 2022 13:19
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

6 participants