This project is archived and is in readonly mode.
rake gems:unpack version handling broken
-
Wincent ColaiutaI'm seeing the same thing with 2.1.1.
-
Collin VanDyckAlso have seen it. Luckily it seems to be isolated to just the unpack routines. Gem version specifications work just fine if the gem is either installed on the system or locally in the vendor/gems directory.
-
Norman ClarkeAttached is a tiny patch which fixes the issue.
Gem::GemRunner#run expects an array of unquoted argument strings, so stripping leading and tailing quotes from the members of the unpack_command array solves the problem.
This is not a problem with the install_command array because it gets executed via a shell-out rather than passed into GemRunner.
Note that removing the quotes from the unpack_command method (line 121) would also have worked, but it seemed like a sloppier solution because it would look surprisingly inconsistent with the install_command method right above it.
-
fix_gem_unpack_for_2_1_1.patch
551 Bytes
-
-
Norman Clarke
Sorry, forgot to mention above, this is in railsties/lib/rails/gem_dependency.rb
-
Wincent Colaiuta
\z is probably better than \Z in the regex because it's a stricter match.
-
Norman Clarke
Cool. Micro-patch of a micro-patch attached. :)
-
fix_gem_unpack_for_2_1_1.patch
551 Bytes
-
-
Jeremy Kemper- State changed from new to open
- Assigned user set to Jeremy Kemper
- Milestone changed from 2.x to 2.1.2
Needs a test :)
-
Wincent Colaiuta
- Assigned user cleared.
While this patch does fix the issue I think it needs to be reviewed by someone familiar with the GemDependency class.
I've looked at it and I'm not entirely sure of what kind of values might be in the @requirements instance variable. Here's the code as it currently exists:
def install_command cmd = %w(install) << @name cmd << "--version" << %("#{@requirement.to_s}") if @requirement cmd << "--source" << @source if @source cmd end def unpack_command cmd = %w(unpack) << @name cmd << "--version" << %("#{@requirement.to_s}") if @requirement cmd endThe obvious "first glance" solution would be to just drop the quotes for both cases:
def install_command cmd = %w(install) << @name cmd << "--version" << @requirement.to_s if @requirement cmd << "--source" << @source if @source cmd end def unpack_command cmd = %w(unpack) << @name cmd << "--version" << @requirement.to_s if @requirement cmd endBut seeing as I don't know the code I am wondering if @requirement could ever hold a value that would need to be quoted like that. I also wonder why @requirement is used and passed as a --version parameter instead of the @version instance variable which is set up in the initialize method as shown below and then used only in one other place (the unpacked_paths method):
@version = @requirement.instance_variable_get("@requirements").first.last if @requirementAnd furthermore when doing config.gem you can evidently pass in :requirement and :version options, but the docs and examples I've seen don't indicate what you'd use :requirement for.
Anyway, rather than floundering about in the code any further, like I said, it would good if someone who knows this stuff would look into it and review that patch.
-
Wincent Colaiuta
- Assigned user set to Jeremy Kemper
Whoops. Just noticed that when I posted my last comment the assigned user got cleared by mistake. Sorry about that. Restoring it to its former value.
-
Norman Clarke
To be honest I'm not 100% sure what is a good way to test this. There's a lot of Filesystem IO to be intercepted if you want a test for unpack_to, to me it seems like a lot of code for a ridiculously small patch.
The change the broke this feature seems to have been added only because the code looked inconsistent. Perhaps this commit should simply be reverted. There's even a comment at the bottom of the commit, made 8 days later, warning that it was introducing a bug.
I agree with Wincent it would be ideal to rexamine the code and perhaps make some organizational improvements. In the mean time though it would be useful for the feauture to go back to working - I'd hate to see this stay broken into the next version because people were waiting for the "ideal" fix to come along. :-)
-
Wincent Colaiuta
Ah, good research Norman finding that commit. I wouldn't revert the entire commit (in the "git revert" sense) seeing as it also includes whitespace fixes, but I would revert the bit which adds quoting inside the "unpack_command" method.
I think the code probably needs a comment too, explaining why it's not quoted so that the same mistake doesn't creep back in later.
Also agreed that coming up with a test for this is not too easy.
-
Michael Koziarski- State changed from open to resolved
- Tag changed from 2.1.1, gems to 2.1.1, gems
I've fixed this now in:
a78ec93 7d2201d
-
Wincent Colaiuta
Looks good!
-
-