Cleanup ARGV usage

[MikeMcQuaid]

Now we have a proper argument parser (thanks @GauthamGoli!) we should replace all uses of ARGV in the Homebrew/brew codebase with calls to Homebrew.args instead.

extend/ARGV.rb should be moved into https://github.com/Homebrew/brew/blob/master/Library/Homebrew/cli/args.rb where needed or removed where not. This may warrant changes to formulae and odeprecated.

Note formula_installer.rb and build.rb should be handled last as they read from and modify ARGV:

brew/Library/Homebrew/formula_installer.rb

Lines 678 to 751 in b4f73e6

	def sanitized_argv_options
	args = []
	args << "--ignore-dependencies" if ignore_deps?
	if build_bottle?
	args << "--build-bottle"
	args << "--bottle-arch=#{ARGV.bottle_arch}" if ARGV.bottle_arch
	end
	args << "--git" if git?
	args << "--interactive" if interactive?
	args << "--verbose" if verbose?
	args << "--debug" if debug?
	args << "--cc=#{ARGV.cc}" if ARGV.cc
	args << "--default-fortran-flags" if ARGV.include? "--default-fortran-flags"
	args << "--keep-tmp" if ARGV.keep_tmp?
	if ARGV.env
	args << "--env=#{ARGV.env}"
	elsif formula.env.std? || formula.deps.select(&:build?).any? { |d| d.name == "scons" }
	args << "--env=std"
	end
	if formula.head?
	args << "--HEAD"
	elsif formula.devel?
	args << "--devel"
	end
	formula.options.each do |opt|
	name = opt.name[/^([^=]+)=$/, 1]
	value = ARGV.value(name) if name
	args << "--#{name}=#{value}" if value
	end
	args
	end
	def build_argv
	sanitized_argv_options + options.as_flags
	end
	def build
	FileUtils.rm_rf(formula.logs)
	@start_time = Time.now
	# 1. formulae can modify ENV, so we must ensure that each
	# installation has a pristine ENV when it starts, forking now is
	# the easiest way to do this
	args = %W[
	nice #{RUBY_PATH}
	-W0
	-I #{$LOAD_PATH.join(File::PATH_SEPARATOR)}
	--
	#{HOMEBREW_LIBRARY_PATH}/build.rb
	#{formula.specified_path}
	].concat(build_argv)
	Utils.safe_fork do
	if Sandbox.formula?(formula)
	sandbox = Sandbox.new
	formula.logs.mkpath
	sandbox.record_log(formula.logs/"build.sandbox.log")
	sandbox.allow_write_path(ENV["HOME"]) if ARGV.interactive?
	sandbox.allow_write_temp_and_cache
	sandbox.allow_write_log(formula)
	sandbox.allow_cvs
	sandbox.allow_fossil
	sandbox.allow_write_xcode
	sandbox.allow_write_cellar(formula)
	sandbox.exec(*args)
	else
	exec(*args)

No need to say "I'm planning on working on this" or similar in this issue but feel free to open a PR that doesn't work (yet!) and ask for help.

[BenMusch]

This seems like something I'd be happy to help out with, though it seems like an incremental approach to this is best (rather than getting rid of it all in one PR)

Would this be a reasonable way to do that?

1. Add a nested Homebrew::CLI::Parser::ArgvExtension module
2. Add ARGV.extend(Homebrew::CLI::Parser::ArgvExtension) to global.rb (but do not remove the other ARGV.extend call)
3. From this point, extract logic to the new ArgvExtension module when needed, and replace ARGV.* methods with Homebrew.args.* when possible
4. Remove HomebrewArgvExtension entirely

And then after repeating step 3 on all the various ARGV calls, it would eventually be possible to remove the old HomebrewArgvExtension

One other clarification: Is the goal to add more info into the Homebrew.args struct, or just to add the ARGV logic into the cli_parser file?

For example, we could pre-compute the named arguments and add it as a field to Homebrew.args, and then delete ARGV.named. But doing this for every ARGV method would make the Homebrew.args struct somewhat bloated (which might still be worth it!)

[MikeMcQuaid]

@BenMusch Sounds great!

For example, we could pre-compute the named arguments and add it as a field to Homebrew.args, and then delete ARGV.named. But doing this for every ARGV method would make the Homebrew.args struct somewhat bloated (which might still be worth it!)

I think we can avoid precomputing but we could cache it intelligently.

Part of the motivation for this is that I don't think we will need to do it for every ARGV method; some of them will be able to be removed or replaced with better APIs. Any that aren't used in Homebrew/brew or Homebrew/homebrew-core can be considered internal APIs and removed. Even among those it may be they can be refactored so they aren't needed.

The easy ones to start with should be those like ARGV.build_bottle?. That should be able to be replaced with according args.build_bottle? and the method deleted without any code needing to be added.

[MikeMcQuaid]

I've updated the original issue a bit now #6021 is merged which should simplify things a bit.

[jeduardo824]

Could I start to work on this?

[MikeMcQuaid]

@jeduardo824 Yes, please!

[mofo37]

Hi! I'd like to work on this issue, but I'm having trouble running the tests. What is the command to run them? I tried ruby collector_spec.rb and a couple other file names/folder structure paths, but I'm getting require errors. Thanks!

[GauthamGoli]

Tests can be run using brew tests command

[GauthamGoli]

Hi @TedTran2019! I am working on this in my free time, but you can work on this too, #6433 is part of the on-going effort. You can take a look at the other PRs referenced here to get an idea.

[goibon]

In a tap of mine (homebrew-upup) I use the flags? method that ARGV used to be extended with to test for flags. Since #6857 this method has been made private and no longer accessible as ARGV.flags?.
I can see that you are moving away from use of ARGV and to use Homebrew.args instead and I was wondering what the proper way of parsing args is now 🤔
Will it be possible for me to use Homebrew.args and if so how?
Or will I have to parse args myself?

[dawidd6]

@goibon You can just replace flag? with include?. Should work the same way.

[GauthamGoli]

@goibon Use Homebrew::CLI::Parser to declare args.
Take a look at https://github.com/Homebrew/brew/blob/master/Library/Homebrew/dev-cmd/tests.rb
and
https://github.com/Homebrew/brew/tree/master/Library/Homebrew/dev-cmd
to get an idea.

[MikeMcQuaid]

@goibon Sorry for the inconvenience but that was essentially a private API 😭. I hope in the near future to be able to provide an example in e.g. Homebrew/bundle of using Homebrew::CLI::Parser in a tap.

[dawidd6]

@issyl0 already made first steps, to provide an example how to use it in a tap: https://github.com/Homebrew/homebrew-linux-dev/pull/137.

[MikeMcQuaid]

Note to @goibon and @GauthamGoli: all Homebrew organisation tap commands are now using Homebrew::CLI::Parser.

[goibon]

@MikeMcQuaid @GauthamGoli Thanks for the pointers, I switched to using Homebrew::CLI::Parser now and it seems to work 👏
However I'm not sure how to get the usage banner to show 🤔
When I invoke my command like brew upup --help or brew upup -h I see the usage banner for brew itself. Oddly though if I invoke my command like brew upup -help with one dash, it actually shows my usage banner. When using -help I don't get an error message at the bottom like when using unknown options 🤷‍♂
Can you see anything wrong with the way I'm implementing it?

require "cli/parser"

def announce_args
  Homebrew::CLI::Parser.new do
    usage_banner <<~EOS
    `upup` [`--cleanup`][`--quiet`]
    EOS
    switch "--cleanup",
      description: "Calls brew cleanup after the last step"
    switch "--quiet",
      description: "Runs quietly with no output unless something goes wrong"
  end
end
announce_args.parse

[MikeMcQuaid]

@goibon Check the other PRs. TL;DR you need to:

• rename e.g. brew-services.rb to services.rb
• move everything into the Homebrew module
• use services_args and services methods; the latter will be called to run and the prior to generate help text etc.
• ensure services calls services_args.parse