jimweirich / rake

A make-like build utility for Ruby.
http://rake.rubyforge.org/
1.1k stars 10 forks source link

rake eating ARGV #277

Open mpalmer opened 10 years ago

mpalmer commented 10 years ago

Somewhere in the last few years, since 41a334b4 (which came from this mailing list post, I believe), ARGV has once again started getting mangled by option parsing. Since I've recently started avoiding the need to bundle exec rake everywhere (by re-execing rake if it isn't already running inside bundler) I need access to the entire ARGV to pass back to exec.

I haven't attached a patch, because the only solution I've got so far is duping ARGV into a temporary variable before calling init in Rake::Application#run, then restoring ARGV (with ARGV.replace(orig_argv)) afterwards -- and I'm not sure that's particularly kosher. If, on the other hand, you'd be happy to merge such a patch, let me know and I'll put it together and whack in some test cases.

drbrain commented 9 years ago

This line is doing it, but has been doing that since 2009 (maybe earlier as that commit moves things around).

This should be easy to fix.

mpalmer commented 9 years ago

Thanks for fixing this, @drbrain. That's a great way to fix it, too -- not using ARGV directly is always a good way to go, I've found.

drbrain commented 9 years ago

No problem!

drbrain commented 9 years ago

I had to revert this fix as it causes problems such as rails/spring#366 and there doesn't seem to be two ways to do this.

I'm not sure there is an easy alternative that won't involve some manual work by you to re-exec rake.

mpalmer commented 9 years ago

Aaaah, bummer. I don't entirely understand the referenced bug report -- it sounds like spring is manually futzing with ARGV itself to inject extra arguments into Rake, which seems like a Bad Idea, overall, but there's no denying that the change to Rake did seem to break other people's stuff, and I think it was the right call to revert this change.

Unfortunately, I can't see any way of getting around this problem without a change in rake. By the time the first line of my code gets executed (in my Rakefile), the damage is already done -- ARGV has been modified, and I'm boned. My original "dodgy hack", of wrapping the call to init in Rake::Application#run with a copy/replace of ARGV does work -- it's what I've been running on my local machine since I opened this bug -- but, as I said originally, I didn't submit it as a PR because I felt it was an excessively ugly way of fixing the problem. Your approach was much better, but appears to violate the expectations of existing consumers of the Rake API.

I can think of a couple of ways forward:

  1. Deprecate the existing way that people are late-passing parameters into Rake.application via ARGV. Provide a new way to pass arguments into Rake.application, possibly as simple as just documenting the argv accessor. Detect when ARGV has changed between when Rake::Application was instantiated and when #run is called (take a private copy of ARGV at the same time as @argv = ARGV, and compare it to ARGV) and emit a deprecation warning, with the intent to remove looking at ARGV at all during #run in Rake 11, some months from now. The logic for selecting whether to use @argv or ARGV for arguments will involve checking the private dup of ARGV made in Rake::Application#initialize.
  2. Move to Rake 11 now, with your previous patch re-applied. Non-backwards-compatible change, etc etc. Brutal, but effective.
  3. Use my dodgy hack of duping then restoring ARGV around the call to #init in #run. Nasty, and will require some comments to explain to future generations what the deuce is happening here, but it'll solve the problem and doesn't require anyone else's ARGV-mangling code to ever need to know anything changed.

If 1 or 3 sound like the best way forward to you, I'm happy to put together a PR for consideration, just let me know and I'll break out the text editor.

drbrain commented 9 years ago

It's not necessarily spring by itself, but spring + new relic or something else that calls Rake.application and gets captured.

With Ruby 2.2 coming soon releasing a Rake 11 now is a difficult thing to support.

I prefer something along the lines of option 3 but don't yet have an idea that's easy for future generations to understand. I gave some thought to this before reverting it but wasn't coming up with any good names or any way to explain it without several paragraphs.

mpalmer commented 9 years ago

Well... we might just need several paragraphs then. I'll put together a PR for option 3 and see what I can come up with.

mpalmer commented 9 years ago

Bah, I've just done a bit more digging, and it looks like just passing ARGV.dup) to the call to OptionParser#parse! in Rake::Application#handle_options seems to do the trick quite nicely. Not sure why I didn't dig that deep previously. PR incoming.

bretweinraub commented 9 years ago

ah i see this thread now.

maybe its my own doing .... but 10.4.2 has really blown up my world because of this [probably how our tool was built; we really depending on knowing which command line args rake cared about via the edited ARGV].

But its a weird thing .... I just seem to get 10.4.2 off a prod box .... its like some kind of sticky. Gem list doesn't show it but ARGV non-eating cannot be removed..... server down. Not your fault really. Don't code to undoc-ed features....

bretweinraub commented 9 years ago

rvm implode-d # seems to work

mpalmer commented 9 years ago

Sorry that my change broke your application, @bretweinraub. Would an instance method on Rake::Application that returned the arguments that Rake didn't "eat" as a result of option parsing satisfy your requirements? The information about what arguments were treated as options and which weren't is still kept within Rake::Application, it just isn't exposed somewhere.

If such an instance method would suit you, let me know and I'll whip up a PR for @drbrain to merge. If that wouldn't suit for some reason, we can discuss what it is you do need, exactly, and I'll come up with a proposed fix one way or another.