ARGV cleanup: Deprecate ARGV methods that are never used internally #5762

BenMusch · 2019-02-17T06:32:34Z

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?

Addresses part of #5730

This PR deletes ARGV methods which are never used internally in ARGV (for example, it keeps build_head? because it is used in build_stable?, and refactoring methods which have internal usage is a bit more complex of a question in terms of how it is refactored. Will definitely look into cleaning that up later, but wanted to open this PR for all the work which could be done without too much thought

BenMusch · 2019-02-17T06:33:29Z

Library/Homebrew/test/formula_installer_spec.rb

-    cc_arg = "--cc=clang"
-    ARGV << cc_arg
+    expect(Homebrew.args.cc).to be nil
+    allow(Homebrew.args).to receive(:cc).and_return("clang")


Just wanna make sure: This is a valid replacement in tests for adding values to ARGV, right?

BenMusch · 2019-02-17T06:34:08Z

Library/Homebrew/test/sandbox_spec.rb

@@ -47,7 +48,7 @@
    end

    it "ignores bogus Python error" do
-      ENV["HOMEBREW_VERBOSE"] = "1"


Same here: Wanna make sure it's okay to replace env var values with Homebrew.args stubs

BenMusch · 2019-02-17T06:37:25Z

Not really sure I follow why tests are failing (brew tests passes locally), but it definitely seems like it may be related to my changes. Looks like brew test bot might have to be updated? https://github.com/Homebrew/homebrew-test-bot/blob/master/cmd/brew-test-bot.rb#L274 If so, are there any other packages this might affect?

MikeMcQuaid · 2019-02-17T12:58:44Z

Looks like brew test bot might have to be updated? https://github.com/Homebrew/homebrew-test-bot/blob/master/cmd/brew-test-bot.rb#L274 If so, are there any other packages this might affect?

Good catch! The following will likely need updates:
https://github.com/Homebrew/homebrew-test-bot
https://github.com/Homebrew/homebrew-services
https://github.com/Homebrew/homebrew-bundle

scpeters · 2019-02-17T13:10:04Z

Also https://github.com/Homebrew/homebrew-livecheck

Perhaps we could deprecate these old methods to give downstream taps a chance to notice the changes (see #5477 for an example). Then we could merge this without being blocked or breaking homebrew commands in external taps.

MikeMcQuaid · 2019-02-17T15:04:23Z

Perhaps we could deprecate these old methods to give downstream taps a chance to notice the changes (see #5477 for an example).

This seems like a good call 👍 . For those that we notice have been used anywhere outside Homebrew/brew an odeprecated and odisabled make sense before their removal. For consistency they could also call back to Homebrew.args.foo rather than querying ARGV.foo directly.

BenMusch · 2019-02-17T18:31:02Z

Looks like the deprecations are still failing the build -- should I be doing this differently? https://dev.azure.com/Homebrew/Homebrew/_build/results?buildId=4180

jonchang · 2019-02-18T04:47:57Z

Looks like the deprecations are still failing the build -- should I be doing this differently? https://dev.azure.com/Homebrew/Homebrew/_build/results?buildId=4180

The issue is in https://github.com/Homebrew/homebrew-test-bot/, not your pull request.

I believe you'll have to do the pull request in several parts: first to add the new API, then a pull request to test-bot to use the new API, then the deprecations in a final pull request to deprecate the old API. Probably a pull request to homebrew-core fixing all uses of the old API should be in there somewhere as well. brew test-bot runs with HOMEBREW_DEVELOPER=1 set I believe so warnings (including deprecations) become errors.

Longer term we should probably look at moving the homebrew/test-bot code into homebrew/brew.

jonchang · 2019-02-18T04:49:40Z

Actually it looks like you're not adding any new APIs here, just deprecating old ones. So a pull request to homebrew-test-bot fixing its calls should be sufficient.

BenMusch · 2019-02-18T05:22:30Z

Ah thanks @jonchang! Didn't see the line about throwing an error when homebrew_developer? is true. Opened a PR to change the test-bot: Homebrew/homebrew-test-bot#238

MikeMcQuaid · 2019-02-18T08:56:16Z

@BenMusch Some failing tests but not brew test-bot any more 🎉

MikeMcQuaid · 2019-02-18T12:22:04Z

Cross-posting from Homebrew/homebrew-test-bot#240:

I would suggest making use of ARGV.freeze to catch these cases (both here and in Homebrew/brew) and fix/refactor them before any more ARGV to Homebrew.args conversion is done.

BenMusch · 2019-02-18T21:38:53Z

@MikeMcQuaid should we also find a way to prevent modification of ENV (hopefully without interfering with anything non-argv-related values), since Homebrew.args values are also controlled by that?

MikeMcQuaid · 2019-02-19T08:10:59Z

@MikeMcQuaid should we also find a way to prevent modification of ENV (hopefully without interfering with anything non-argv-related values), since Homebrew.args values are also controlled by that?

@BenMusch Good question. I don't believe there are (m)any cases where ENV is overridden and the expectation is that ARGV.whatever? will change so we're fine from that perspective. Also, we relatively often need to modify ENV for subprocesses so I think we're left with what we have for now (without introducing a new API for ENV which also updates CLI::Parser which I think would be overkill for now).

BenMusch · 2019-02-19T16:32:00Z

Sounds good. Just posting this as a heads up: probably won't be able to make progress on the PR for a week or so due to some schoolwork that's come up. But definitely intend on getting back to it once I'm free

MikeMcQuaid · 2019-02-19T16:43:48Z

No worries, thanks @BenMusch!

BenMusch · 2019-03-06T19:08:40Z

Just a heads up: Busier with schoolwork than I thought so it might not be a while until I pick this up again. Will check-in periodically to keep the PR up-to-date with master, and anyone who wants to take over the work on this should feel welcome to if my timeliness becomes an issue!

MikeMcQuaid · 2019-03-06T19:11:34Z

@BenMusch Would you be able to extract the minimal version that still passes tests (perhaps into a new PR)? Thanks!

BenMusch · 2019-03-06T19:27:56Z

@MikeMcQuaid Definitely, would a minimal version of this be: not deprecating existing methods, and only changing the ARGV.* to Homebrew.args.* when it doesn't result in test failures?

scpeters · 2019-03-06T19:44:04Z

would a minimal version of this be: not deprecating existing methods, and only changing the ARGV.* to Homebrew.args.* when it doesn't result in test failures?

that sounds good to me

stale · 2019-03-27T20:16:33Z

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

ARGV cleanup: Replace ARGV methods with no internal ARGV usage

e2138de

BenMusch commented Feb 17, 2019

View reviewed changes

BenMusch added 2 commits February 17, 2019 13:18

Deprecate instead of deleting

2f617d6

More info in deprecations + add a replacement

6dbe8a5

BenMusch changed the title ~~ARGV cleanup: Replace ARGV methods that are never used internally~~ ARGV cleanup: Deprecate ARGV methods that are never used internally Feb 17, 2019

BenMusch mentioned this pull request Feb 18, 2019

Remove calls to deprecated ARGV methods Homebrew/homebrew-test-bot#238

Merged

Forgot two methods with the macro

e7a997e

BenMusch force-pushed the argv-cleanup branch from ed0eeb8 to e7a997e Compare February 18, 2019 06:20

Merge branch 'master' into argv-cleanup

3f91972

BenMusch mentioned this pull request Mar 10, 2019

[WIP] ARGV cleanup: replace uses of ARGV methods with Homebrew.args #5856

Closed

6 tasks

stale bot added the stale No recent activity label Mar 27, 2019

stale bot closed this Apr 3, 2019

lock bot added the outdated PR was locked due to age label May 3, 2019

lock bot locked as resolved and limited conversation to collaborators May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARGV cleanup: Deprecate ARGV methods that are never used internally #5762

ARGV cleanup: Deprecate ARGV methods that are never used internally #5762

BenMusch commented Feb 17, 2019 •

edited

Loading

BenMusch Feb 17, 2019

MikeMcQuaid Feb 17, 2019

BenMusch Feb 17, 2019

MikeMcQuaid Feb 17, 2019

BenMusch commented Feb 17, 2019 •

edited

Loading

MikeMcQuaid commented Feb 17, 2019

scpeters commented Feb 17, 2019

MikeMcQuaid commented Feb 17, 2019

BenMusch commented Feb 17, 2019

jonchang commented Feb 18, 2019 •

edited

Loading

jonchang commented Feb 18, 2019

BenMusch commented Feb 18, 2019

MikeMcQuaid commented Feb 18, 2019

MikeMcQuaid commented Feb 18, 2019

BenMusch commented Feb 18, 2019 •

edited

Loading

MikeMcQuaid commented Feb 19, 2019

BenMusch commented Feb 19, 2019

MikeMcQuaid commented Feb 19, 2019

BenMusch commented Mar 6, 2019

MikeMcQuaid commented Mar 6, 2019

BenMusch commented Mar 6, 2019

scpeters commented Mar 6, 2019

stale bot commented Mar 27, 2019

ARGV cleanup: Deprecate ARGV methods that are never used internally #5762

ARGV cleanup: Deprecate ARGV methods that are never used internally #5762

Conversation

BenMusch commented Feb 17, 2019 • edited Loading

BenMusch Feb 17, 2019

Choose a reason for hiding this comment

MikeMcQuaid Feb 17, 2019

Choose a reason for hiding this comment

BenMusch Feb 17, 2019

Choose a reason for hiding this comment

MikeMcQuaid Feb 17, 2019

Choose a reason for hiding this comment

BenMusch commented Feb 17, 2019 • edited Loading

MikeMcQuaid commented Feb 17, 2019

scpeters commented Feb 17, 2019

MikeMcQuaid commented Feb 17, 2019

BenMusch commented Feb 17, 2019

jonchang commented Feb 18, 2019 • edited Loading

jonchang commented Feb 18, 2019

BenMusch commented Feb 18, 2019

MikeMcQuaid commented Feb 18, 2019

MikeMcQuaid commented Feb 18, 2019

BenMusch commented Feb 18, 2019 • edited Loading

MikeMcQuaid commented Feb 19, 2019

BenMusch commented Feb 19, 2019

MikeMcQuaid commented Feb 19, 2019

BenMusch commented Mar 6, 2019

MikeMcQuaid commented Mar 6, 2019

BenMusch commented Mar 6, 2019

scpeters commented Mar 6, 2019

stale bot commented Mar 27, 2019

BenMusch commented Feb 17, 2019 •

edited

Loading

BenMusch commented Feb 17, 2019 •

edited

Loading

jonchang commented Feb 18, 2019 •

edited

Loading

BenMusch commented Feb 18, 2019 •

edited

Loading