This project is archived and is in readonly mode.

#1003 ✓resolved
Norman Clarke

rake gems:unpack version handling broken

Reported by Norman Clarke | September 9th, 2008 @ 04:02 PM | in 2.1.2

Rails 2.1.1. doesn't unpack gems with version strings like below:

config.gem "will_paginate", :version => ">= 2.2.2" config.gem "mocha", :version => ">= 0.9.1"

It fails with:

ERROR: While executing gem ... (ArgumentError)

Illformed requirement ["\">= 2.2.2\""]

This works properly with Rails 2.1.0.

Comments and changes to this ticket

  • Wincent Colaiuta

    Wincent Colaiuta September 10th, 2008 @ 03:06 PM

    I'm seeing the same thing with 2.1.1.

  • Collin VanDyck

    Collin VanDyck September 10th, 2008 @ 03:08 PM

    Also 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 Clarke

    Norman Clarke September 10th, 2008 @ 06:13 PM

    Attached 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.

  • Norman Clarke

    Norman Clarke September 10th, 2008 @ 06:15 PM

    Sorry, forgot to mention above, this is in railsties/lib/rails/gem_dependency.rb

  • Wincent Colaiuta

    Wincent Colaiuta September 10th, 2008 @ 06:29 PM

    \z is probably better than \Z in the regex because it's a stricter match.

  • Norman Clarke

    Norman Clarke September 10th, 2008 @ 06:35 PM

    Cool. Micro-patch of a micro-patch attached. :)

  • Jeremy Kemper

    Jeremy Kemper September 10th, 2008 @ 10:08 PM

    • 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

    Wincent Colaiuta September 10th, 2008 @ 10:11 PM

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

    The 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
          end
    

    But 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 @requirement
    

    And 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

    Wincent Colaiuta September 11th, 2008 @ 06:02 PM

    • 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

    Norman Clarke September 12th, 2008 @ 12:54 AM

    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

    Wincent Colaiuta September 12th, 2008 @ 10:03 AM

    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

    Michael Koziarski September 24th, 2008 @ 05:47 PM

    • 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
  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 09:49 PM

    • Tag changed from 2.1.1, gems to 2.1.1 gems

    Automatic cleanup of spam.

  • bingbing

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Tags

Referenced by

Pages