argv: move formulae to cli/args by GauthamGoli · Pull Request #6433 · Homebrew/brew

GauthamGoli · 2019-09-08T14:29:36Z

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 tests with your changes locally?

#5730
Please do not merge until #6411 gets merged.

MikeMcQuaid · 2019-09-09T12:19:01Z

@resolved_formulae ||= (downcased_unique_named - casks).map do |name| and remove TODO

I've already tried exactly this but it was breaking a lot of tests. Let me try again.

You may need to add a Homebrew.args.reset or similar which will reset the instance variables and have it in https://github.com/Homebrew/brew/blob/master/Library/Homebrew/test/spec_helper.rb#L135-L208

In general there's some ARGV stuff there that may need similar code for Homebrew.args

I think we've hit a cyclic dependency here.
formula_options method is used to allow extra arguments for formulae see - deps.rb#L55 and install.rb#L90 but formula_options itself needs to know what is passed on command line to define what options can be allowed on command line!

In that case you might want to allow e.g. ARGV to be passed in in some circumstances for initialisation so it's not operating on itself? Would that work?

Yeah, if we limit the usage of ARGV to this one method we could make this work.

I would suggest it's passed in as an argument (a default one if necessary) to make testing easier and just treated as an Array. The issue we're trying to address isn't so much the use of ARGV but removing our patches on ARGV.

GauthamGoli · 2019-10-27T11:42:54Z

@MikeMcQuaid https://github.com/Homebrew/brew/runs/276504778#step:5:373
Its weird that --ignore-dependencies is being passed even without the changes in this PR, but its causing the test to fail here.

MikeMcQuaid · 2019-10-28T09:08:21Z

@GauthamGoli Are you stuck/blocked on this now? If so, can take a look and try to fix the tests. If it's not being passed but showing up that sounds like something being cached between tests that needs reset.

GauthamGoli · 2019-10-28T09:10:47Z

--ignore-dependencies is being passed, but on master branch, the warning doesn't show up in the test. So its not something about cache.
Anyways, please take a look.

MikeMcQuaid · 2019-10-28T09:13:05Z

--ignore-dependencies is being passed, but on master branch, the warning doesn't show up in the test. So its not something about cache.

@GauthamGoli Maybe this needs rebased on master? I've changed that code recently.

GauthamGoli · 2019-10-28T09:17:49Z

Tests fail even after rebase.

MikeMcQuaid · 2019-10-30T13:57:26Z

It looks like this is not being detected properly.

MikeMcQuaid · 2019-10-30T13:58:14Z

--ignore-dependencies is being passed, but on master branch, the warning doesn't show up in the test. So its not something about cache.

Had a look: the issue isn't that --ignore-dependencies is being passed but that the output is not matching Cloning into or testball1/HEAD because it's ignoring/filtering out --HEAD for some reason.

GauthamGoli · 2019-10-30T14:55:25Z

Got it! Thanks. Its the chicken egg problem of args not being populated.

GauthamGoli · 2019-11-02T17:43:16Z

Tests pass now.
@MikeMcQuaid I think the code might need a bit of cleanup/simplification; can you please review, thanks!

MikeMcQuaid

A few minor questions but looks really good, nice work @GauthamGoli!

MikeMcQuaid · 2019-11-04T10:13:13Z

Think it would be good to handle both the above at once and perhaps even return @args, @args_parsed, @parser all from this method to be able to make these attr_reader instead of attr_accessor.

I'm not sure I understand this completely. Could you please add in an example code snippet here. Thanks!

Suggested change

Homebrew.args.args_parsed = @args_parsed

MikeMcQuaid · 2019-11-04T10:14:47Z

Should/could this logic live inside HEAD??

HEAD? is one of the keys in OpenStruct, so I don't think so it possible.
We could extract this logic out to another private method named HEAD

Yeh, a private method for this sounds good 👍

MikeMcQuaid · 2019-11-04T10:14:55Z

zachauten · 2019-11-07T00:08:25Z

@GauthamGoli @MikeMcQuaid So I guess I didn't notice that this PR moved the kegs method to args.rb when I started on #6622, should I just close that one and suggest changes here for the files that use ARGV.kegs?

GauthamGoli · 2019-11-07T06:57:50Z

@zachauten #6622 need not be closed, after this PR is merged, you can rebase on master.

MikeMcQuaid · 2019-11-11T09:03:02Z

Suggested change

@args_parsed = true

@args.args_parsed = @args_parsed = true

MikeMcQuaid · 2019-11-11T09:03:19Z

Suggested change

Homebrew.args.args_parsed = @args_parsed

MikeMcQuaid · 2019-11-11T09:03:52Z

Suggested change

attr_accessor :processed_options, :args_parsed

attr_reader :processed_options, :args_parsed

https://github.com/Homebrew/brew/pull/6433/files#diff-e0e03670c451d934ae1d79adfff63cacR144
cannot be executed with only attr_reader

Ok but I think it would be better to have a custom method that allows them to only be set once and then be frozen/not allow future changes to be made rather than using attr_accessor

@MikeMcQuaid What do you think about this approach?

MikeMcQuaid

Looking good! A couple of final tiny things then good to merge.

Another thing I noticed is that it'd be nice to have https://github.com/Homebrew/brew/blob/86607314eabc7ed11a1860c3f88c517d83f3d113/Library/Homebrew/cli/parser.rb#L143 (@args_parsed) moved until as late as possible in the method just so it isn't set prematurely if something fails.

MikeMcQuaid · 2019-12-11T09:05:35Z

I like this a lot more 👍. How about making it freeze_processed_options!(processed_options) to make it clearer it can only be called once?

Yep, updated.

MikeMcQuaid · 2019-12-11T09:05:46Z

Suggested change

@processed_options.concat(processed_options)

@processed_options += processed_options

MikeMcQuaid · 2019-12-11T09:06:04Z

I'd suggest putting this last just to avoid it being set if e.g. the freeze fails.

MikeMcQuaid · 2019-12-11T11:38:04Z

Thanks again for more great work @GauthamGoli 🎉

GauthamGoli · 2019-12-11T11:42:05Z

We need to watch this closely for a few days for any bugs. I will be watching the issues 👍

GauthamGoli added the do not merge label Sep 8, 2019

MikeMcQuaid reviewed Sep 9, 2019

View reviewed changes

Comment thread Library/Homebrew/cli/args.rb Outdated

MikeMcQuaid reviewed Sep 9, 2019

View reviewed changes

Comment thread Library/Homebrew/cli/args.rb Outdated

MikeMcQuaid reviewed Sep 9, 2019

View reviewed changes

Comment thread Library/Homebrew/cli/parser.rb Outdated

GauthamGoli force-pushed the argv-cleanup-3 branch 3 times, most recently from 34b8fe8 to 2cd58ca Compare September 10, 2019 20:18

GauthamGoli removed the do not merge label Sep 10, 2019

GauthamGoli force-pushed the argv-cleanup-3 branch 3 times, most recently from 633383b to 2e3a7dc Compare September 11, 2019 06:07

GauthamGoli added the do not merge label Sep 23, 2019

GauthamGoli force-pushed the argv-cleanup-3 branch from 2e3a7dc to 6c0b87c Compare October 27, 2019 10:03

GauthamGoli force-pushed the argv-cleanup-3 branch from 6c0b87c to 01e9682 Compare October 28, 2019 09:15

MikeMcQuaid reviewed Oct 30, 2019

View reviewed changes

Comment thread Library/Homebrew/cli/args.rb Outdated

Copy link
Copy Markdown

Member

MikeMcQuaid Oct 30, 2019

Choose a reason for hiding this comment

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

It looks like this is not being detected properly.

GauthamGoli force-pushed the argv-cleanup-3 branch from 01e9682 to a09f246 Compare October 30, 2019 14:54

GauthamGoli removed the do not merge label Nov 2, 2019

MikeMcQuaid reviewed Nov 4, 2019

View reviewed changes

GauthamGoli mentioned this pull request Nov 5, 2019

Cleanup ARGV usage #5730

Closed

MikeMcQuaid reviewed Nov 11, 2019

View reviewed changes

GauthamGoli force-pushed the argv-cleanup-3 branch from a09f246 to 224a9e3 Compare November 16, 2019 19:13

MikeMcQuaid reviewed Dec 11, 2019

View reviewed changes

Gautham Goli added 3 commits December 11, 2019 16:36

argv: move formulae to cli/args

22e25dd

cli_args: Remove cyclic dependency on ARGV

28e62b5

CLI::Args: Make processed_options read-only

2b95755

GauthamGoli force-pushed the argv-cleanup-3 branch from 8660731 to 2b95755 Compare December 11, 2019 11:11

MikeMcQuaid approved these changes Dec 11, 2019

View reviewed changes

MikeMcQuaid merged commit 277a8b2 into Homebrew:master Dec 11, 2019

lock bot added the outdated PR was locked due to age label Jan 10, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020

	attr_accessor :processed_options, :args_parsed
	attr_reader :processed_options, :args_parsed

	@processed_options.concat(processed_options)
	@processed_options += processed_options

Uh oh!

Conversation

GauthamGoli commented Sep 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GauthamGoli Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GauthamGoli commented Oct 27, 2019

Uh oh!

MikeMcQuaid commented Oct 28, 2019

Uh oh!

GauthamGoli commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeMcQuaid commented Oct 28, 2019

Uh oh!

GauthamGoli commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Oct 30, 2019

Uh oh!

GauthamGoli commented Oct 30, 2019

Uh oh!

GauthamGoli commented Nov 2, 2019

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GauthamGoli Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachauten commented Nov 7, 2019

Uh oh!

GauthamGoli commented Nov 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

GauthamGoli Sep 26, 2019 •

edited

Loading

GauthamGoli commented Oct 28, 2019 •

edited

Loading

GauthamGoli commented Oct 28, 2019 •

edited

Loading

GauthamGoli Nov 10, 2019 •

edited

Loading