Rails Ruby on Rails → 2.1.2

Comments and changes to this ticket

• You flagged this item as spam.
  

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

  I'm seeing the same thing with 2.1.1.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • fix_gem_unpack_for_2_1_1.patch 551 Bytes

• You flagged this item as spam.
  

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

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

• You flagged this item as spam.
  

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

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

• You flagged this item as spam.
  

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

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

  • fix_gem_unpack_for_2_1_1.patch 551 Bytes
• 

  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 :)

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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. :-)

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  Wincent Colaiuta September 24th, 2008 @ 06:41 PM

  Looks good!

• 

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

  • Tag changed from “2.1.1, gems” to “2.1.1 gems”

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:13 AM

  louis vuitton watches