Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

RubyGems pre_install hook now uses require 'devkit' #107

Merged
merged 1 commit into from

3 participants

@Azolo
Owner

Use require 'devkit' in operating_system.rb instead of duplicating the code that is already defined in devkit.rb

Original conception from:

https://groups.google.com/d/msg/rubyinstaller/39-GKsIC5mw/Kj24coVS6Q0J

@Azolo
Owner

Can anyone spot any unforeseen problems with this?

I was able to build Ruby 1.9.3 and 1.8.7 then use the newly built Devkit to install rdiscount and rails.

But I also don't know the internals well enough to say this wouldn't cause side effects.

@luislavena luislavena merged commit ded3058 into oneclick:master
@luislavena
Owner

@Azolo actually no issue with this.

Your change landed in ded3058

Thank you!

@jonforums
Owner

I may change it to use load rather than require. I see no reason why $LOADED_FEATURES should be checked.

Thanks!

@luislavena
Owner

@jonforums me no follows. Where $LOADED_FEATURES is checked? This is only triggered during gem installation, so no application will be running that mess or want to mess with $LOADED_FEATURES

@jonforums
Owner

I injected a puts $LOADED_FEATURES as the first line in the pre_install hook and saw a bunch of features (118) on my machine during a native gem install. Won't require have to scan $LOADED_FEATURES before loading devkit.rb, but load will not?

@luislavena
Owner

@jonforums please forgive me stupidity. Today is friday, end of a really long week and brain at this time is almost burnt.

I have problems understanding why the single require will have any issues? Also, using load will force you to specify the full path as devkit.rb is not in the same directory of operating_system.rb

Don't see a huge performance issue on this since time is actually spent more in gem download and extension compilation than the require itself.

@Azolo
Owner

Actually load won't make you specify the absolute path, since it looks in $LOAD_PATH too. You do still need to provide the file extension though, devkit.rb.

But in the case where you need to use the DevKit more than once during a gem install process, I think it would actually be better to scan $LOADED_FEATURES than to search the $LOAD_PATH.

HOWEVER, right now it doesn't matter. The current implementation will fail when you try to install multiple DevKit gems.

Ends up, since require returns false if the file is already loaded. This causes the Gem.pre_install block to return false on a second run that requires 'devkit'; then everything proceeds to break.

That being said, require is still much more flexible than load. But we would have to make the pre_install block return something other than false since we're actually accomplishing our goal of making sure the environment is set up.

@jonforums
Owner

@luislavena I agree and also don't see any functional problems (or real performance gains) with using require but I prefer load because it does less. It's a style issue...less is cleaner.

I've replaced require 'devkit' with load "devkit.rb" and things seem to be working fine wrt pathing. Still testing...

Also, the following needs to be refactored back in as it was the 1 user request I got when writing the original code.

Gem.ui.say 'Temporarily enhancing PATH to include DevKit...' if Gem.configuration.verbose

...a simple change to push into devkit.rb and guard with something like if defined?(Gem). I'll test a bit and update the mode.

@jonforums
Owner

HOWEVER, right now it doesn't matter. The current implementation will fail when you try to install multiple DevKit gems.

@Azolo that's not good. What's the specifics on how you get the current impl to fail?

@Azolo
Owner

Just run gem i rdiscount json

It fails on json at the moment.

@luislavena
Owner
@jonforums
Owner

With this as my test operating_system.rb I can't repro.

# :DK-BEG: override 'gem install' to enable RubyInstaller DevKit usage
Gem.pre_install do |gem_installer|
  load 'devkit.rb' unless gem_installer.spec.extensions.empty?
end
# :DK-END:

C:\Users\Jon\Documents>gem i rdiscount json
Fetching: rdiscount-1.6.8.gem (100%)
Temporarily enhancing PATH to include DevKit...
Building native extensions. This could take a while...
Successfully installed rdiscount-1.6.8
Fetching: json-1.6.6.gem (100%)
Building native extensions. This could take a while...
Successfully installed json-1.6.6
2 gems installed

...and I also can't repro using the old version before the latest mods. Odd...dammit nothing's ever easy...maybe we're still to close to the 13th ;)

@Azolo
Owner

Put that out of devkit and only in operating_system.rb

Actually that would solve the problem of failing after require...

Gem.pre_install do |gem_installer|
  unless gem_installer.spec.extensions.empty?
    if require 'devkit'
      Gem.ui.say 'Temporarily enhancing PATH to include DevKit...' if Gem.configuration.verbose
    end
  end
end
@Azolo
Owner

@jonforums Actually it only fails with require your ugly looking load works fine. :tongue:

@Azolo
Owner

Here's a gist of the error:

https://gist.github.com/2384960

@jonforums
Owner

@Azolo hehe..fugly. how about new tweaks and we'll all test until we agree it's solid?

@jonforums
Owner

Here's a gist of the error:

https://gist.github.com/2384960

Does it also fail when you remove --no-ri --no-rdoc from the cmd line and push those to a %USERPROFILE%\.gemrc containing gem: --no-ri --no-rdoc?

@Azolo
Owner

Sure, do you still want to use load? It would make it harder to make the Gem.ui.say message conditional.

@Azolo
Owner

Does it also fail when you remove --no-ri --no-rdoc from the cmd line and push those to a %USERPROFILE%\.gemrc containing gem: --no-ri --no-rdoc?

Yeah, this is why it currently fails.

https://gist.github.com/2385177

So when require gets called on subsequent calls it cause's pre_install, it stops the installation.

@jonforums
Owner

Sure, do you still want to use load? It would make it harder to make the Gem.ui.say message conditional.

testing your latest pull request...i still prefer load but frankly, it's probably a nit vs. require

But...I really like that load works like this:

C:\Users\Jon>irb
irb(main):001:0> require 'devkit'
Temporarily enhancing PATH to include DevKit...
=> true
irb(main):002:0> require 'devkit'
=> false
irb(main):003:0> load 'devkit.rb'
=> true
irb(main):004:0> load 'devkit.rb'
=> true
irb(main):005:0> load 'devkit.rb'
=> true
irb(main):006:0> load 'duvtukit.rb'
LoadError: cannot load such file -- duvtukit.rb
        from (irb):6:in `load'
        from (irb):6
        from C:/ruby193/bin/irb:12:in `<main>'
irb(main):007:0>
@luislavena
Owner

@jonforums and @Azolo I want my bikeshed purple...

load will always attempt to load and modify the path... require will only do it once.

We just need to ensure that Gem.pre_install returns true and let's move to something more important (like rdoc and CHM updates).

@jonforums
Owner

whoa...put that bikeshed toy away, you're going to hurt yourself.

we rarely release devkit updates so it makes sense to spend a little more time ensuring we've got a mod that we agree is as solid as we can make it and doesn't cause a bunch of support issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 13, 2012
  1. @Azolo
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 8 deletions.
  1. +1 −8 resources/devkit/dk.rb.erb
View
9 resources/devkit/dk.rb.erb
@@ -49,14 +49,7 @@ EOT
# #{DEVKIT_START} override 'gem install' to enable RubyInstaller DevKit usage
Gem.pre_install do |gem_installer|
unless gem_installer.spec.extensions.empty?
- unless ENV['PATH'].include?('#{d}\\\\mingw\\\\bin') then
- Gem.ui.say 'Temporarily enhancing PATH to include DevKit...' if Gem.configuration.verbose
- ENV['PATH'] = '#{d}\\\\bin;#{d}\\\\mingw\\\\bin;' + ENV['PATH']
- end
- ENV['RI_DEVKIT'] = '#{d}'
-<% tool_names.each_pair do |k,v| -%>
- <%= 'ENV[\'%s\'] = \'%s\'' % [k,v] %>
-<% end -%>
+ require 'devkit'
end
end
# #{DEVKIT_END}
Something went wrong with that request. Please try again.