cmd/install: infer --overwrite when in GitHub Actions
#18612
Conversation
Based on discussion at Homebrew/homebrew-core#195288. I was initially inclined to scope this only to Python, but I think it makes sense to apply more generally. The issue is that users often have no control over what's already inside `HOMEBREW_PREFIX` when they run `brew install`. Given that, I think it makes sense to assume users want `--overwrite` unless configured otherwise with `HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE`.
| # Print warning only once. | ||
| github_env = ENV.fetch("GITHUB_ENV", "") | ||
| if File.exist?(github_env) && File.writable?(github_env) | ||
| File.open(github_env, "a") { |f| f.puts("HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE_WARNING=1") } | ||
| end |
There was a problem hiding this comment.
I'm 50-50 about this bit, and don't mind dropping it.
There was a problem hiding this comment.
Also don't feel strongly either way. If it's Python only: can drop.
| @@ -313,7 +339,7 @@ def run | |||
| keep_tmp: args.keep_tmp?, | |||
| debug_symbols: args.debug_symbols?, | |||
| force: args.force?, | |||
| overwrite: args.overwrite?, | |||
| overwrite:, | |||
There was a problem hiding this comment.
This approach doesn't quite work, unfortunately. This is because the --overwrite flag doesn't propagate to dependencies. Should it?
There was a problem hiding this comment.
Passed it on to dependencies in 79959ca.
There was a problem hiding this comment.
This approach doesn't quite work, unfortunately. This is because the
--overwriteflag doesn't propagate to dependencies. Should it?
I don't think it should.
And skip displaying the message if `--overwrite` was already requested.
| @@ -281,6 +281,33 @@ def run | |||
| puts "You're on your own. Failures are expected so don't create any issues, please!" | |||
| end | |||
|
|
|||
| overwrite = if !args.overwrite? && | |||
| GitHub::Actions.env_set? && | |||
| ENV["HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE"].blank? && | |||
There was a problem hiding this comment.
Should we document this in env_config.rb?
| ENV["HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE"].blank? && | |
| ENV["HOMEBREW_NO_GITHUB_ACTIONS_INSTALL_OVERWRITE"].blank? && |
would be my preferred naming format, for consistency
| ENV["HOMEBREW_GITHUB_ACTIONS"].blank? | ||
| if ENV["HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE_WARNING"].blank? | ||
| message = <<~WARNING | ||
| In GitHub Actions, `brew install` behaves the same as `brew install --overwrite`. |
There was a problem hiding this comment.
Yeh, sorry to be a pain here: this definitely feels like an overly broad change. Would like to see this scoped to only Python for now and we can consider widening it in future if desired.
| # Print warning only once. | ||
| github_env = ENV.fetch("GITHUB_ENV", "") | ||
| if File.exist?(github_env) && File.writable?(github_env) | ||
| File.open(github_env, "a") { |f| f.puts("HOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE_WARNING=1") } | ||
| end |
There was a problem hiding this comment.
Also don't feel strongly either way. If it's Python only: can drop.
| @@ -313,7 +339,7 @@ def run | |||
| keep_tmp: args.keep_tmp?, | |||
| debug_symbols: args.debug_symbols?, | |||
| force: args.force?, | |||
| overwrite: args.overwrite?, | |||
| overwrite:, | |||
There was a problem hiding this comment.
This approach doesn't quite work, unfortunately. This is because the
--overwriteflag doesn't propagate to dependencies. Should it?
I don't think it should.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Based on discussion at Homebrew/homebrew-core#195288.
I was initially inclined to scope this only to Python, but I think it
makes sense to apply more generally. The issue is that users often have
no control over what's already inside
HOMEBREW_PREFIXwhen they runbrew install. Given that, I think it makes sense to assume users want--overwriteunless configured otherwise withHOMEBREW_GITHUB_ACTIONS_NO_INSTALL_OVERWRITE.