Update documentation for `brew install` and FAQ regarding `HOMEBREW_NO_INSTALL_FROM_API` #14570

s4nji · 2023-02-09T17:02:11Z

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 was very confused why brew install was seemingly ignoring the changes I did through brew edit, only to find out after quite some time that it doesn't use the local git repository/formula anymore unless a flag is provided: #14473

This PR:

Adds a line regarding HOMEBREW_NO_INSTALL_FROM_API to brew install help documentation
Correct a (now misleading) FAQ entry on how to edit a formula, adding HOMEBREW_NO_INSTALL_FROM_API=1 flag to the brew install snippet.

MikeMcQuaid · 2023-02-10T08:53:58Z

Thanks for the PR @s4nji! I think we'll want to think a little more about the best flow here. Any thoughts @carlocab @Rylan12 @Bo98?

carlocab · 2023-02-10T10:15:57Z

Library/Homebrew/cmd/install.rb

+
+        Unless `HOMEBREW_NO_INSTALL_FROM_API` is set, `brew install` will ignore your locally edited formula
+        done using `brew edit`.


I think this should be documented in brew edit instead. Most users of brew install probably aren't interested in this information, but all users of brew edit will probably need to know it.

Maybe brew edit can print a warning that can be silenced with HOMEBREW_NO_ENV_HINTS? We could probably also drop the reference to HOMEBREW_NO_INSTALL_FROM_API in the FAQ below if we do this.

I have removed the message from install and added the warning in edit in 34a19cf.
I still think the FAQ should be updated since the command mentioned in it is flat-out wrong; I suppose people might notice the warning when they run brew edit but still...

But let me know if you'd like me to / feel free to drop 14927bb

carlocab · 2023-02-10T13:20:25Z

Library/Homebrew/dev-cmd/edit.rb

+        Unless `HOMEBREW_NO_INSTALL_FROM_API` is set when running
+        `brew install`, it will ignore your locally edited formula.


Suggested change

Unless `HOMEBREW_NO_INSTALL_FROM_API` is set when running

`brew install`, it will ignore your locally edited formula.

For your changes to take effect, you must set `HOMEBREW_NO_INSTALL_FROM_API`.

I'd also mention this only applies to core formulae/casks too.

Ideally this message would also only display if you pass an arg to a core formula/cask and not when you edit a tap one.

@Bo98 Done in 79beebe (first time writing ruby I hope it's good 👀)

@carlocab I think brew install should be mentioned explicitly since it's otherwise unclear/potentially misleading since this is printed when you run brew edit (but again please feel free to edit directly)

MikeMcQuaid · 2023-02-10T17:15:39Z

@carlocab @Bo98 if/when either of you are happy: feel free to merge!

apainintheneck

This definitely needs to be updated. Thanks for taking a stab at it.

Library/Homebrew/dev-cmd/edit.rb

apainintheneck · 2023-02-11T08:01:20Z

docs/FAQ.md

@@ -140,7 +140,7 @@ If all maintainer feedback has been addressed and all tests are passing, bump it

 ## Can I edit formulae myself?

-Yes! It’s easy! Just `brew edit <formula>`. You don’t have to submit modifications back to `homebrew/core`, just edit the formula to what you personally need and `brew install <formula>`. As a bonus, `brew update` will merge your changes with upstream so you can still keep the formula up-to-date **with** your personal modifications!
+Yes! It’s easy! Just `brew edit <formula>`. You don’t have to submit modifications back to `homebrew/core`, just edit the formula to what you personally need and `HOMEBREW_NO_INSTALL_FROM_API=1 brew install <formula>`. As a bonus, `brew update` will merge your changes with upstream so you can still keep the formula up-to-date **with** your personal modifications!


Stupid question: does brew update need HOMEBREW_NO_INSTALL_FROM_API=1 here too?

~~Nope, HOMEBREW_NO_INSTALL_FROM_API needs to be set only for the install.~~
~~I don't think it does anything to update.~~

I misunderstood and also now question if the local changes are also kept (like the FAQ entry claimed) when update is done with the API 🤔
However I think this could be a separate issue/pr if turns out to be incorrect

This piece of documentation is out-of-date so I'm not sure I would trust it in this case. The brew update command doesn't update the homebrew/core and homebrew/cask taps when using the API so your changes shouldn't get overwritten but no upstream changes will get merged in either.

brew/Library/Homebrew/cmd/update-report.rb

Line 159 in 1cf53b4

next if (tap.core_tap? || tap == "homebrew/cask") && Homebrew::EnvConfig.install_from_api?

To be clear I'm not sure that changing that is within the scope of this PR but it does make me wonder how we should handle these things going forward. Passing HOMEBREW_NO_INSTALL_FROM_API implicitly every time the brew edit command is called seems to make sense until you realize that it causes unexpected behavior when using other commands.

@apainintheneck @EricFromCanada any suggestions on how to update this documentation to be both correct and helpful would be handy.

Passing HOMEBREW_NO_INSTALL_FROM_API implicitly every time the brew edit command is called seems to make sense until you realize that it causes unexpected behavior when using other commands.

Agreed this could be tweaked/improved. Don't think it's worth reverting this, though.

docs/FAQ.md

Library/Homebrew/dev-cmd/edit.rb

Co-authored-by: Kevin <[email protected]>

MikeMcQuaid · 2023-02-14T14:13:31Z

Thanks for the PR @s4nji, you rock!

carlocab reviewed Feb 10, 2023

View reviewed changes

MikeMcQuaid approved these changes Feb 10, 2023

View reviewed changes

apainintheneck reviewed Feb 11, 2023

View reviewed changes

apainintheneck reviewed Feb 14, 2023

View reviewed changes

docs/FAQ.md Outdated Show resolved Hide resolved

MikeMcQuaid reviewed Feb 14, 2023

View reviewed changes

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

MikeMcQuaid approved these changes Feb 14, 2023

View reviewed changes

s4nji and others added 2 commits February 14, 2023 12:04

edit: add HOMEBREW_NO_INSTALL_FROM_API warning.

2daae5e

docs/FAQ: correct an entry regarding simple formula edit

c7b8b3a

Co-authored-by: Kevin <[email protected]>

MikeMcQuaid enabled auto-merge February 14, 2023 12:05

carlocab approved these changes Feb 14, 2023

View reviewed changes

MikeMcQuaid merged commit cedd4d3 into Homebrew:master Feb 14, 2023

github-actions bot added the outdated PR was locked due to age label Mar 18, 2023

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation for `brew install` and FAQ regarding `HOMEBREW_NO_INSTALL_FROM_API` #14570

Update documentation for `brew install` and FAQ regarding `HOMEBREW_NO_INSTALL_FROM_API` #14570

s4nji commented Feb 9, 2023 •

edited

Loading

MikeMcQuaid commented Feb 10, 2023

carlocab Feb 10, 2023 •

edited

Loading

s4nji Feb 10, 2023 •

edited

Loading

carlocab Feb 10, 2023

Bo98 Feb 10, 2023 •

edited

Loading

s4nji Feb 10, 2023 •

edited

Loading

MikeMcQuaid commented Feb 10, 2023

apainintheneck left a comment

apainintheneck Feb 11, 2023

s4nji Feb 11, 2023 •

edited

Loading

apainintheneck Feb 11, 2023

apainintheneck Feb 11, 2023

MikeMcQuaid Feb 13, 2023

MikeMcQuaid commented Feb 14, 2023


		Unless `HOMEBREW_NO_INSTALL_FROM_API` is set, `brew install` will ignore your locally edited formula
		done using `brew edit`.

		Unless `HOMEBREW_NO_INSTALL_FROM_API` is set when running
		`brew install`, it will ignore your locally edited formula.

	Unless `HOMEBREW_NO_INSTALL_FROM_API` is set when running
	`brew install`, it will ignore your locally edited formula.
	For your changes to take effect, you must set `HOMEBREW_NO_INSTALL_FROM_API`.

Update documentation for brew install and FAQ regarding HOMEBREW_NO_INSTALL_FROM_API #14570

Update documentation for brew install and FAQ regarding HOMEBREW_NO_INSTALL_FROM_API #14570

Conversation

s4nji commented Feb 9, 2023 • edited Loading

MikeMcQuaid commented Feb 10, 2023

carlocab Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

s4nji Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

carlocab Feb 10, 2023

Choose a reason for hiding this comment

Bo98 Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

s4nji Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 10, 2023

apainintheneck left a comment

Choose a reason for hiding this comment

apainintheneck Feb 11, 2023

Choose a reason for hiding this comment

s4nji Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

apainintheneck Feb 11, 2023

Choose a reason for hiding this comment

apainintheneck Feb 11, 2023

Choose a reason for hiding this comment

MikeMcQuaid Feb 13, 2023

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 14, 2023

Update documentation for `brew install` and FAQ regarding `HOMEBREW_NO_INSTALL_FROM_API` #14570

Update documentation for `brew install` and FAQ regarding `HOMEBREW_NO_INSTALL_FROM_API` #14570

s4nji commented Feb 9, 2023 •

edited

Loading

carlocab Feb 10, 2023 •

edited

Loading

s4nji Feb 10, 2023 •

edited

Loading

Bo98 Feb 10, 2023 •

edited

Loading

s4nji Feb 10, 2023 •

edited

Loading

s4nji Feb 11, 2023 •

edited

Loading