Support normal gem require without explicit 'gem "name"' for default gem #377

Merged
merged 12 commits into from Nov 27, 2012

6 participants

@kou

Let "test-unit" is a default gem.

Before:

require "test/unit" # -> test/unit.rb of the standard library is loaded

gem "test-unit"
require "test/unit" # -> test/unit.rb test-unit gem is loaded

After:

require "test/unit" # -> test/unit.rb of test-unit gem is loaded

Default gem related works are not completed. There are following
not implemented features:

  • Register the default gems
  • Support "gem contents" for RDoc
  • Block "gem uninstall default-gem"

This change shows what I want to do for implementing default gem. If
you accept this change, I will do the next works.

Note that I want to move Kerenel#require codes in
lib/rubygems/custom_require.rb to Gem::CustomRequire#run in the
future.

@kou

@drbrain, if you are busy to check this change, I'll continue this work a week later. If my work is bad, we can revert it later.

@drbrain drbrain commented on an outdated diff Oct 2, 2012
lib/rubygems.rb
+ ##
+ # Find a Gem::Specification of default gem from +path+
+
+ def find_unresolved_default_spec(path)
+ Gem.suffixes.each do |suffix|
+ spec = @path_to_default_spec_map["#{path}#{suffix}"]
+ return spec if spec
+ end
+ nil
+ end
+
+ ##
+ # Remove needless Gem::Specification of default gem from
+ # unresolved default gem list
+
+ def remove_unresoleved_default_spec(spec)
@drbrain
RubyGems member
drbrain added a note Oct 2, 2012

There is a typo, an extra "e" in "unresoleved", remove_unresolved_default_spec is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drbrain drbrain commented on an outdated diff Oct 2, 2012
lib/rubygems/custom_require.rb
@@ -4,6 +4,22 @@
# See LICENSE.txt for permissions.
#++
+module Gem
+ class CustomRequire
@drbrain
RubyGems member
drbrain added a note Oct 2, 2012

Can you flatten this to class Gem::CustomRequire, most other RubyGems classes are created this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drbrain drbrain commented on an outdated diff Oct 2, 2012
lib/rubygems/custom_require.rb
@@ -4,6 +4,22 @@
# See LICENSE.txt for permissions.
#++
+module Gem
+ class CustomRequire
+ def initialize(path)
+ @path = path
+ end
+
+ def run
+ spec = Gem.find_unresolved_default_spec(@path)
+ if spec
+ Gem.remove_unresoleved_default_spec(spec)
@drbrain
RubyGems member
drbrain added a note Oct 2, 2012

Typo from lib/rubygems.rb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drbrain
RubyGems member

I think this is a good start, please commit it.

Please proceed with moving the Kernel#require codes to Gem::CustomRequire

@kou
kou commented Oct 5, 2012

Thanks for your review!
I'll apply your comments and continue my work.

@evanphx
RubyGems member

Can you remove the Gem::CustomRequire class please? It's not necessary.

Also, I'd prefer to merge this in when the code that actually calls register_default_spec is also committed, so that I can get a good feeling for how it's going to be used.

@drbrain
RubyGems member

@evanphx I think having a thin Kernel#require and having the logic of require in CustomRequire is acceptable.

@kou
kou commented Oct 9, 2012

@evanphx What about the following steps?

  1. Remove Gem::CustomRequire
  2. Add registering default gems feature
  3. Merge this changes
  4. Try Gem::CustomRequire again as another pull request

I want to put default gem feature to Ruby 2.0. So I will drop Gem::CustomRequire feature if it blocks default gem feature.

@kou

I removed Gem::CustomRequire and implemented registering the default gems feature.

The changes assume that the default gems are placed at #{GEM_HOME}/specifications/default/*.gemspec. Is it right place? Should we use defaults instead of default?

If the changes are OK, I'll merge them and work on master branch.

@kou

@evanphx ping. Could you review my new changes?

@drbrain
RubyGems member

I think this is a good location. I prefer default.

What happens when the user changes ENV["GEM_HOME"]?

Will the default specifications still be discovered?

@voxik

May I ask why have you chosen such hard way? Rubygems already supports multiple GEM_PATHs, so why don't you place the default gems into different GEM_PATH? Since RubyGems can install and uninstall gems just from GEM_HOME, they will be protected from uninstallation. For that it is enough to

1) Rewrite Gem.default_path [1] to include lets say ruby/default_gems, with standard directories such as gems, specifications, docs, etc.
2) Move the parts of StdLib into that directory structure and that is it.

Actually Fedora is using this approach already, the only missing piece is blessing from upstream.

[1] https://github.com/rubygems/rubygems/blob/master/lib/rubygems/defaults.rb#L63

@voxik

Just for your curiosity, this [1] is upstream repository of Fedora's package. You can find there the .spec file and all patches applied. The .spec file is the main file, which describes Fedora's compilation process. I.e. in %prep section, there are applied patches, in %build section is executed the build process and finally, in %install section, we do installation, install custom operating_system.rb [2] and moves the part of StdLib in places where they should be if they were gems [3]. Additionally, we need to do some minor tweaks to .gemspec [4] to ensure they correctly reflect the rubygems layout.

[1] http://pkgs.fedoraproject.org/cgit/ruby.git/tree/
[2] http://pkgs.fedoraproject.org/cgit/ruby.git/tree/ruby.spec#n401
[3] http://pkgs.fedoraproject.org/cgit/ruby.git/tree/ruby.spec#n411
[4] http://pkgs.fedoraproject.org/cgit/ruby.git/tree/ruby.spec#n441

@voxik

This ensures that gem command cannot modify the system gems, since they are maintained by RPM/YUM. This also allows to update JSON gem for example, if newer release is available. Moreover, we keep only one version of gem on the system, as per the Fedoras policies.

@evanphx evanphx commented on an outdated diff Oct 24, 2012
lib/rubygems/specification.rb
+ self.dirs.each { |dir|
+ Dir[File.join(dir, gemspec_glob)].each { |path|
+ spec = Gem::Specification.load path.untaint
+ # #load returns nil if the spec is bad, so we just ignore
+ # it at this stage
+ yield(spec) if spec
+ }
+ }
+ end
+
+ def self._each_default(&block) # :nodoc:
+ _each_spec(File.join("default", "*.gemspec"), &block)
+ end
+
+ def self._each_normal(&block) # :nodoc:
+ _each_spec(File.join("*.gemspec"), &block)
@evanphx
RubyGems member
evanphx added a note Oct 24, 2012

No need for a File.join here, it's only one argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanphx evanphx commented on an outdated diff Oct 24, 2012
lib/rubygems/specification.rb
@@ -611,19 +611,35 @@ def test_files= files
attr_accessor :specification_version
+ def self._each_spec(gemspec_glob) # :nodoc:
@evanphx
RubyGems member
evanphx added a note Oct 24, 2012

If these methods are meant to be private, let's actually make them private rather than prefixing with an underscore. You can do that by putting them in a class << self block and using private inside it before the methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@evanphx
RubyGems member

@voxik The reason it's not using the normal GEM_PATH mechanism is that it actually injects the default gems "in front of" the standard library. Meaning the default gems are attempted to be activated and used before the standard library. You can see this reflect in the change to our #require.

@drbrain
RubyGems member

Also, changing the structure of the standard library is too radical a change for ruby developers at this time.

@voxik

@evanphx Gems are alway in front of standard library, since their paths are always searched sooner. The only thing is how to activate them. But once you remove the offending files from StdLib into appropriate gems, they gets activated. So no problem with that.

@drbrain How it could be radical? In what sense? I would suggest to move the gems out of stdlib into gems subdirectory of Ruby development tree, but that is all. You see that it works even without that for Fedora. What else is radical about it?

Actually the way I proposing is return to roots of RubyGems. Rake, Psych, RDoc were used to be independent gems (well they used to be even non gems I guess), but later they were crippled to be part of StdLib. Although that decision probably came earlier then the decision to make RubyGems integral part of Ruby. But instead of taking advantage of it and revert the wrong decision, you build on it and now crippling also RubyGems. These are sad days.

@drbrain
RubyGems member

The majority of the ruby committers don't want to move the files. If you want the ruby committers to move the files please take this issue up on http://bugs.ruby-lang.org or the ruby-core mailing list. This issue is not the appropriate place to discuss such issues.

I appreciate your feelings on this matter, but such a change is not going to happen in ruby 2.0, so we're going to move ahead with this solution as implemented by @kou.

@zenspider
@voxik

@drbrain If [1] is not targeted for 2.0, so why continue this way?

[1] http://bugs.ruby-lang.org/issues/5481

@drbrain
RubyGems member

To my knowledge, "gemify" has not yet meant "change the layout of the ruby repository". I have been unable to convince other ruby committers that this is the best path. Perhaps it will be possible for ruby 3.0, but it is definitely too late for ruby 2.0.

@zzak
RubyGems member

@voxik You have taken this way off-course, please keep discussion related to this ticket

@voxik

@zzak sorry, but this is very relevant ... not merging this commit and fix stdlib is very relevant

@kou

@drbrain Thanks for your comment. I will use default.

I didn't think about ENV["GEM_HOME"]. I tried it. If an user changed both ENV["GEM_HOME"] and ENV["GEM_PATH"], the current implementation doesn't work. It is better that we always search #{Gem.default_path}/specifications/default/*.gemspec. I will do it.

@kou

@evanphx Thanks for your review. I've applied your suggestions.

Can I merge this branch into master and work on master? Or should I continue to work on this branch?

@kou

I've implemented for the following features for default gem:

  • Loading normal gems rather than the default gems if a higher version normal gem exists
  • Support "gem contents default-gem"
  • Block "gem uninstall default-gem"

And I rebased on to the current master.

The following works are needed to complete default gem support:

  • Merging this changes into master
  • Putting the latest RubyGems into the Ruby's repository
  • Modifying *.gemspec in the Ruby's repository
  • Testing the latest RubyGems with RDoc

I have a problem the last work. Does someone know what RDoc command failed with empty "gem contents"? If I know it, I can test it.

kou added some commits Sep 23, 2012
@kou kou Support normal gem require without explicit 'gem "name"' for default gem
Let "test-unit" is a default gem.

Before:

    require "test/unit" # -> test/unit.rb of the standard library is loaded

    gem "test-unit"
    require "test/unit" # -> test/unit.rb test-unit gem is loaded

After:

    require "test/unit" # -> test/unit.rb of test-unit gem is loaded

Default gem related works are not completed. There are following
not implemented features:

  * Register the default gems
  * Support "gem contents" for RDoc
  * Block "gem uninstall default-gem"

This change shows what I want to do for implementing default gem. If
you accept this change, I will do the next works.

Note that I want to move Kerenel#require codes in
lib/rubygems/custom_require.rb to Gem::CustomRequire#run in the
future.
0262778
@kou kou Fix a typo
unresoleved ->
unresolved

Pointed out by @drbrain. Thanks!!!
018a90e
@kou kou Don't create new class Gem::CustomRequire
Suggested by Evan Phoenix.

I'll re-propose this change later. I don't work on it with default gem
changes.
e627cda
@kou kou Add missing "lib/" dabde2d
@kou kou Support loading the default gems automatically
The default gems should be placed at
#{GEM_HOME}/specifications/default/*.gemspec.
Is it right place? Should we use 'defaults' instead of 'default'?
66b70bb
@kou kou Remove needless File.join
Suggested by Evan Phoenix. Thanks!!!
d333341
@kou kou Use private instead of _XXX
This change does it against only Specification._each_spec,
_each_normal and _each_default. We should change other methods such as
_all and _resort! but it is not a work for this branch.

Suggested by Evan Phoenix. Thanks!!!
d598fdc
@kou kou Use GEM_HOME independent search path for default gems
Default gems should be placed at
#{Gem.default_dir}/specifications/default/*.gemspec instead of
#{GEM_HOME}/specifications/default/*.gemspec.

If a user set both of ENV["GEM_HOME"] and GEM["GEM_PATH"], default
gems couldn't be detected.

Reported by Eric Hodel. Thanks!!!
60ddd38
@kou kou Reject uninstalling a default gem
Note: Gem::Specification#default_gem? is suitable name? Should it be
private?
b948b81
@kou kou Add missing 'end's
It is merging miss...
abd9937
@kou kou Support "gem contents" for default gem 6834877
@kou

I rebased on to the current master again.

@drbrain
RubyGems member

gem contents io-console should be a good example

@kou

Thanks.
I checked it and understood that it's difficult... I'll think about it...

@kou

Could you tell me the expected output of gem contents io-console?

Here are candidates I thought:

1) It shows installed files:

% gem contents io-console
${PREFIX}/lib/ruby/2.0.0/console/size.rb
${PREFIX}/lib/ruby/2.0.0/x86_64-linux/io/console.so

2) It shows source files:

% gem contents io-console
${PREFIX}/lib/ruby/gems/2.0.0/gems/default/io-console-0.3/console.c
${PREFIX}/lib/ruby/gems/2.0.0/gems/default/io-console-0.3/extconf.h
${PREFIX}/lib/ruby/gems/2.0.0/gems/default/io-console-0.3/extconf.rb
${PREFIX}/lib/ruby/gems/2.0.0/gems/default/io-console-0.3/lib/console/size.rb

If 1) is the expected output, I'll change tool/rbinstall.rb in the Ruby repository. RubyGems side implementation is already done.

If 2) is the expected output, I'll change RubyGems. There is a concern for this candidate. The listed files doesn't exist because Ruby's make install doesn't install source. (It's normal make install behavior.)

If there are no the expected output, could you tell me the expected output?

@drbrain
RubyGems member

1) is sufficient. We can treat built-in gems with C extensions as pre-compiled C extensions.

@kou

Thanks for your answer.
I'll apply the following patch to the Ruby repository when this pull request is merged and the changes are included in the Ruby repository:

Index: tool/rbinstall.rb
===================================================================
--- tool/rbinstall.rb   (revision 37766)
+++ tool/rbinstall.rb   (working copy)
@@ -562,24 +562,95 @@
       src.sub!(/\A#.*/, '')
       eval(src, nil, path)
     end
+
+    def to_ruby
+        <<-GEMSPEC
+Gem::Specification.new do |s|
+  s.name = #{name.dump}
+  s.version = #{version.dump}
+  s.summary = #{summary.dump}
+  s.description = #{description.dump}
+  s.homepage = #{homepage.dump}
+  s.authors = #{authors.inspect}
+  s.email = #{email.inspect}
+  s.files = #{files.inspect}
+end
+        GEMSPEC
+    end
   end
 end

 module RbInstall
   module Specs
+    class FileCollector
+      def initialize(base_dir)
+        @base_dir = base_dir
+      end
+
+      def collect
+        ruby_libraries + built_libraries
+      end
+
+      private
+      def type
+        /\/(ext|lib)?\/.*?\z/ =~ @base_dir
+        $1
+      end
+
+      def ruby_libraries
+        case type
+        when "ext"
+          prefix = "#{$extout}/common/"
+          base = "#{prefix}#{relative_base}"
+        when "lib"
+          base = @base_dir
+          prefix = base.sub(/lib\/.*?\z/, "") + "lib/"
+        end
+
+        Dir.glob("#{base}{.rb,/**/*.rb}").collect do |ruby_source|
+          remove_prefix(prefix, ruby_source)
+        end
+      end
+
+      def built_libraries
+        case type
+        when "ext"
+          prefix = "#{$extout}/#{CONFIG['arch']}/"
+          base = "#{prefix}#{relative_base}"
+          Dir.glob("#{base}{.so,/**/*.so}").collect do |built_library|
+            remove_prefix(prefix, built_library)
+          end
+        when "lib"
+          []
+        end
+      end
+
+      def relative_base
+        /\/#{Regexp.escape(type)}\/(.*?)\z/ =~ @base_dir
+        $1
+      end
+
+      def remove_prefix(prefix, string)
+        string.sub(/\A#{Regexp.escape(prefix)}/, "")
+      end
+    end
+
     class Reader < Struct.new(:src)
       def gemspec
         @gemspec ||= begin
-          Gem::Specification.load(src) || raise("invalid spec in #{src}")
+          spec = Gem::Specification.load(src) || raise("invalid spec in #{src}")
+          file_collector = FileCollector.new(File.dirname(src))
+          spec.files = file_collector.collect
+          spec
         end
       end

       def spec_source
-        File.read src
+        @gemspec.to_ruby
       end
     end

-    class Generator < Struct.new(:name, :src, :execs)
+    class Generator < Struct.new(:name, :base_dir, :src, :execs)
       def gemspec
         @gemspec ||= eval spec_source
       end
@@ -591,6 +662,7 @@
   s.version = #{version.dump}
   s.summary = "This #{name} is bundled with Ruby"
   s.executables = #{execs.inspect}
+  s.files = #{files.inspect}
 end
         GEMSPEC
       end
@@ -602,6 +674,11 @@
         } or return
         version.split(%r"=\s*", 2)[1].strip[/\A([\'\"])(.*?)\1/, 2]
       end
+
+      def files
+        file_collector = FileCollector.new(base_dir)
+        file_collector.collect
+      end
     end
   end
 end
@@ -615,6 +692,9 @@
   prepare "default gems", gem_dir, directories

   spec_dir = File.join(gem_dir, directories.grep(/^spec/)[0])
+  default_spec_dir = "#{spec_dir}/default"
+  makedirs(default_spec_dir)
+
   gems = {}
   File.foreach(File.join(srcdir, "defs/default_gems")) do |line|
     line.chomp!
@@ -624,11 +704,12 @@
     line.scan(/\G\s*([^\[\]\s]+|\[([^\[\]]*)\])/) do
       words << ($2 ? $2.split : $1)
     end
-    name, src, execs = *words
-    next unless name and src
+    name, base_dir, src, execs = *words
+    next unless name and base_dir and src

     src       = File.join(srcdir, src)
-    specgen   = RbInstall::Specs::Generator.new(name, src, execs || [])
+    base_dir  = File.join(srcdir, base_dir)
+    specgen   = RbInstall::Specs::Generator.new(name, base_dir, src, execs || [])
     gems[name] ||= specgen
   end

@@ -639,10 +720,12 @@

   gems.sort.each do |name, specgen|
     gemspec   = specgen.gemspec
+    base_dir  = specgen.src.sub(/\A#{Regexp.escape(srcdir)}\//, "")
     full_name = "#{gemspec.name}-#{gemspec.version}"

     puts "#{" "*30}#{gemspec.name} #{gemspec.version}"
-    open_for_install(File.join(spec_dir, "#{full_name}.gemspec"), $data_mode) do
+    gemspec_path = File.join(default_spec_dir, "#{full_name}.gemspec")
+    open_for_install(gemspec_path, $data_mode) do
       specgen.spec_source
     end

Index: defs/default_gems
===================================================================
--- defs/default_gems   (revision 37766)
+++ defs/default_gems   (working copy)
@@ -1,5 +1,5 @@
-# gem      versioning file         [executable files under bin]
-rake       lib/rake/version.rb     [rake]
-rdoc       lib/rdoc.rb         [rdoc ri]
-minitest   lib/minitest/unit.rb
-json       ext/json/lib/json/version.rb
+# gem      base directory      versioning file         [executable files under bin]
+rake       lib/rake        lib/rake/version.rb     [rake]
+rdoc       lib/rdoc        lib/rdoc.rb         [rdoc ri]
+minitest   lib/minitest        lib/minitest/unit.rb
+json       ext/json        ext/json/lib/json/version.rb

All of my works for default gem feature are done except applying the patch to the Ruby repository. :-)

@evanphx evanphx merged commit 0c54ab8 into rubygems:master Nov 27, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment