cmd/install: add messaging for installing edited formulae under API

[Bo98]

--build-from-source intentionally takes a remote copy of the source code, for security & performance reasons (and because we don't actually update local clones anymore). For most people this works great.

There is however a little confusing for contributors to Homebrew/core who are editing formulae. The workflow has changed slightly, as reflected in #14740.

This PR is to provide the same notice of change, but in env hint form.

This hint displays if brew install is being run under these conditions:

• --build-from-source is passed, and
• HOMEBREW_NO_INSTALL_FROM_API is not set, and
• HOMEBREW_DEVELOPER is not set, and
• HOMEBREW_NO_ENV_HINTS is not set, and
• a Homebrew/core formula is one of the formulae is being installed, and
• Homebrew/core is tapped, and
• either the current Homebrew/core git branch is not the same as the origin default (master) or there are uncommitted changes.

If we want, we could possibly make it a little bit smarter by checking if the uncommitted changes apply to the formulae in question or not.

[BrewTestBot]

Review period skipped due to critical label.

[Rylan12]

This looks great, thanks @Bo98!

It would be cool to check to see whether the modified files match the formulae being installed, but it definitely not worth it if it's more than a few extra lines of code.

---

To clarify, is the reason we're not defaulting to a local install in these cases because of security?

[Bo98]

To clarify, is the reason we're not defaulting to a local install in these cases because of security?

The main reason is because we optimise for the other use cases, which are far more common (most people don't edit formulae so requiring a few minute long clone for a --HEAD install is excessive - libimobiledevice gets 1k-2k HEAD installs per month alone, neovim 4k).

I suppose we could make it so these specific set of conditions (if tightened a bit more) used a local install. Not sure how complex that would be. I think the way it would work would be we would "re-load" the formula wrapped in the HOMEBREW_NO_INSTALL_FROM_API env before passing it over to the formula installer.

Though there might be cases of people editing a formula years ago and forgetting about it. Because the tap doesn't get updated anymore it'll remain on the old version forever rather than regularly rebased.

[Rylan12]

Pre 4.0.0, modifying a formula file meant you got a modified install, so feel like it makes sense to do that here too in these particular cases. I would like to hear what others think since this is yet another behavior change.

Basically, I think I'm suggesting something like this (not complete code or tested)

    if args.build_from_source? && !Homebrew::EnvConfig.no_install_from_api? && CoreTap.instance.installed?
      core_path = CoreTap.instance.path
      if ((core_path.git_origin_branch.present? && !core_path.git_default_origin_branch?) || core_path.git_dirty?) &&
         formulae.any? { |f| f.core_formula? && # check whether the formula is the one modified }
        if Homebrew::EnvConfig.developer? || ENV["HOMEBREW_DEV_CMD_RUN"].present?
          opoo "Local changes to Homebrew/core detected for the following formulae: #{formulae.map(:name)}"
          puts "Using the local definitions instead of the API."
          puts "If you don't know what these edits are, run `brew untap Homebrew/core`."
          # Reload the formulae that need it
        elsif !Homebrew::EnvConfig.no_env_hints?
          opoo "Local changes to Homebrew/core detected."
          puts "Homebrew no longer reads from local Homebrew/core clones by default."
          puts "If you are intentionally making an edit to Homebrew/core formula,"
          puts "set HOMEBREW_NO_INSTALL_FROM_API for your local clone to be used."
          puts "If you don't know what these edits are, run `brew untap Homebrew/core`."
        end
      end
    end

So, modifications for a formula being installed result in developers using the local version and everyone else using the API version, with appropriate messaging either way.

Is that too complicated/confusing?

[apainintheneck]

I really like this change. It'd be nice if it could be more targeted to the specific formula that is being installed but it might not be worth the added complexity.

@Rylan12 brings up a good point about this being a breaking change. Have people been complaining about the change so far? When was it released?

[Bo98]

It shipped with 4.0.0 and the only "complaints" (more confusion really) are from those who followed the old formula cookbook since we didn't update that initially. Well, there was one complaint that HOMEBREW_NO_INSTALL_FROM_API was "too long".

It's a workflow change we made ourselves too by adding that env to Homebrew/core CI.

Overall aside from the cookbook confusion, it's gone well. I've yet to see any reports of a --HEAD build failing due to the API so we've succeeded there. A whole class of problems we used to see - reports of people installing from stale git branches after contributing to Homebrew/core - have been entirely eliminated (we usually got a report about that once
every month or two).

There is also the --local idea Rylan had. Not sure if that makes things easier or not - it would still require informing people. Though it might be a good idea anyway because we want to encourage temporary usage of the environment variable rather than permanent.

If not then yeah automatic checking based on modified files sounds like the way to go, if paired with the warning like Rylan has proposed (don't want to reintroduce the "forgotten modification" problem given we don't auto-rebase anymore)

[apainintheneck]

Thanks for the explaining all that. In that case, I think this is the right approach. I'd rather have the user explicitly choose to use their local changes instead of doing it automatically which might end up even being more confusing in some cases.

The --local option is interesting but I don't think it needs to be handled here. That would probably need to be added to a few commands, right?

[MikeMcQuaid]

I've yet to see any reports of a --HEAD build failing due to the API so we've succeeded there.

We tell people to never file reports for --HEAD builds.

There is also the --local idea Rylan had. Not sure if that makes things easier or not - it would still require informing people. Though it might be a good idea anyway because we want to encourage temporary usage of the environment variable rather than permanent.

Yeh, I really like this idea. I think it makes much more sense than replacing a random command. I think it also might be nice to always require it for brew create and brew edit to just really hammer home that it's going to be needed for brew install too.

The --local option is interesting but I don't think it needs to be handled here. That would probably need to be added to a few commands, right?

Agreed, shouldn't block this PR.

[MikeMcQuaid]

Looks good so far!

[MikeMcQuaid]

Suggested change

	puts "If you don't know what these edits are, run `brew untap Homebrew/core`."
	puts "If you don't know what these edits are, run:"
	puts " brew untap homebrew/core"

[MikeMcQuaid]

Feels like this should maybe be in Library/Homebrew/install.rb so it can be used in both cmd/upgrade.rb and cmd/reinstall.rb.

[MikeMcQuaid]

Suggested change

	puts "If you are intentionally making an edit to Homebrew/core formula,"
	puts "set HOMEBREW_NO_INSTALL_FROM_API for your local clone to be used."
	puts "If you are intentionally making an edit to Homebrew/core formula,"
	puts "for your local clone to be used run:"
	puts " export HOMEBREW_NO_INSTALL_FROM_API=1"

[MikeMcQuaid]

Would be nicer to use <<~EOS rather than a bunch of puts

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.