Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARGV: freeze when using CLI::Parser. #5765

Merged
merged 1 commit into from
Feb 18, 2019

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Feb 18, 2019

Otherwise Homebrew.args values may not match those of modified ARGV.

CC @BenMusch

  • 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?

@BenMusch
Copy link
Contributor

BenMusch commented Feb 18, 2019

Not sure if you looked into it yet, but it unfortunately seems like the IRB code that ruby ships with modifies ARGV, which is causing those failures (for example, all the shift calls: https://github.com/ruby/ruby/blob/eda970cfe231797fcf19d14a8dce3a9b49880708/lib/irb/init.rb#L158)

Also, though the most recent version of Ruby added an argv: keyword argument in IRB.setup, (where we could theoretically pass ARGV.dup as the argument), this argument doesn't exist in the Homebrew ruby version.

Not really sure how to handle this. Is there any way to pass IRB its own ARGV? I guess there can be a flag in the *_args methods that we can use to prevent argv from freezing, but that seems like something that should be avoided

Edit: Looks like we can stop the ARGV mutation by adding def IRB.parse_opts; end in the IRB module in debrew/irb. Not sure if there are any important implications of doing that (or why the Module.extend version doesn't override it)

Edit 2: Nevermind, the solution you added is definitely the best bet here

Otherwise `Homebrew.args` values may not match those of modified `ARGV`.
@BenMusch
Copy link
Contributor

Also, curious: what do you think about overriding the operators that mutate ARGV to delegate to CLI::Parser and update Homebrew.args, instead of freezing ARGV?

@MikeMcQuaid MikeMcQuaid merged commit ef5366d into Homebrew:master Feb 18, 2019
@MikeMcQuaid MikeMcQuaid deleted the argv-freeze branch February 18, 2019 19:06
@MikeMcQuaid
Copy link
Member Author

Also, curious: what do you think about overriding the operators that mutate ARGV to delegate to CLI::Parser and update Homebrew.args, instead of freezing ARGV?

@BenMusch Thanks for the help with irb! I think we need both; it's not feasible to override e.g. << without monkeypatching and I'd rather avoid having any more of those.

@BenMusch
Copy link
Contributor

Sounds good, thanks for the help w/ this PR!

fredemmott added a commit to hhvm/homebrew-hhvm that referenced this pull request Feb 19, 2019
- hopefully unneeded after Homebrew/brew#5429 (update default `-march`)
- this approach is banned after Homebrew/brew#5765 (freeze `ARGV`)
@scpeters
Copy link
Member

It looks like there are some formulae that mutate ARGV as well: #5770

@fredemmott
Copy link
Contributor

That was done for a 10x faster binary, and not being able to find a better way to set a default bottle arch; I've rebuilt without that code now, and (probably due to #5429) it's no longer significant.

@fredemmott
Copy link
Contributor

I'll follow up on a separate issue, but that's not accurate. we're seeing 19x slowdowns on some real workloads with nehalem vs sandybridge

@MikeMcQuaid
Copy link
Member Author

@fredemmott Suggestion for that issue would be to check the value of the build arch and fail unless a correct value has been passed in.

@MikeMcQuaid
Copy link
Member Author

@fredemmott but I'm also open to having a way to make formulae depend on a specific CPU architecture (or newer) or CPU features. In general while we're made Homebrew/homebrew-core less generic I'm happy for Homebrew itself to allow things like that to be done in external formulae as long as it's not adding huge amounts of code (and this wouldn't).

@lock lock bot added the outdated PR was locked due to age label Mar 22, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants