Skip to content

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

Merged
evanphx merged 12 commits intoruby:masterfrom
kou:default-gem
Nov 27, 2012
Merged

Support normal gem require without explicit 'gem "name"' for default gem#377
evanphx merged 12 commits intoruby:masterfrom
kou:default-gem

Conversation

@kou
Copy link
Copy Markdown
Member

@kou kou commented Sep 23, 2012

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
Copy link
Copy Markdown
Member Author

kou commented Sep 30, 2012

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

Comment thread lib/rubygems.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@drbrain
Copy link
Copy Markdown
Member

drbrain commented Oct 2, 2012

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

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

@kou
Copy link
Copy Markdown
Member Author

kou commented Oct 5, 2012

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

@evanphx
Copy link
Copy Markdown
Member

evanphx commented Oct 6, 2012

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
Copy link
Copy Markdown
Member

drbrain commented Oct 8, 2012

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

@kou
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member Author

kou commented Oct 13, 2012

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
Copy link
Copy Markdown
Member Author

kou commented Oct 23, 2012

@evanphx ping. Could you review my new changes?

@drbrain
Copy link
Copy Markdown
Member

drbrain commented Oct 23, 2012

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
Copy link
Copy Markdown
Contributor

voxik commented Oct 24, 2012

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
Copy link
Copy Markdown
Contributor

voxik commented Oct 24, 2012

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
Copy link
Copy Markdown
Contributor

voxik commented Oct 24, 2012

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.

Comment thread lib/rubygems/specification.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@evanphx
Copy link
Copy Markdown
Member

evanphx commented Oct 24, 2012

@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
Copy link
Copy Markdown
Member

drbrain commented Oct 24, 2012

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

@voxik
Copy link
Copy Markdown
Contributor

voxik commented Oct 25, 2012

@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
Copy link
Copy Markdown
Member

drbrain commented Oct 25, 2012

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
Copy link
Copy Markdown
Member

On Oct 24, 2012, at 22:15 , Eric Hodel notifications@github.com wrote:

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.

They agreed to gemify at the last kaigi. Matz was on board.

Personally, I'd totally be behind just gemifying the gems that ship with ruby. It would fix a number of issues and NOT actually affect any of the people protesting since they don't own any of the code in question (except the build process?... but I don't think this is a build/packaging issue)

@voxik
Copy link
Copy Markdown
Contributor

voxik commented Oct 25, 2012

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

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

@drbrain
Copy link
Copy Markdown
Member

drbrain commented Oct 25, 2012

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
Copy link
Copy Markdown
Member

zzak commented Oct 25, 2012

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

@voxik
Copy link
Copy Markdown
Contributor

voxik commented Oct 25, 2012

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

@kou
Copy link
Copy Markdown
Member Author

kou commented Oct 26, 2012

@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
Copy link
Copy Markdown
Member Author

kou commented Oct 26, 2012

@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
Copy link
Copy Markdown
Member Author

kou commented Nov 11, 2012

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.

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 added 10 commits November 14, 2012 20:34
unresoleved ->
unresolved

Pointed out by @drbrain. Thanks!!!
Suggested by Evan Phoenix.

I'll re-propose this change later. I don't work on it with default gem
changes.
The default gems should be placed at
#{GEM_HOME}/specifications/default/*.gemspec.
Is it right place? Should we use 'defaults' instead of 'default'?
Suggested by Evan Phoenix. Thanks!!!
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!!!
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!!!
Note: Gem::Specification#default_gem? is suitable name? Should it be
private?
It is merging miss...
@kou
Copy link
Copy Markdown
Member Author

kou commented Nov 14, 2012

I rebased on to the current master again.

@drbrain
Copy link
Copy Markdown
Member

drbrain commented Nov 14, 2012

gem contents io-console should be a good example

@kou
Copy link
Copy Markdown
Member Author

kou commented Nov 16, 2012

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

@kou
Copy link
Copy Markdown
Member Author

kou commented Nov 18, 2012

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
  1. 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
Copy link
Copy Markdown
Member

drbrain commented Nov 18, 2012

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

@kou
Copy link
Copy Markdown
Member Author

kou commented Nov 20, 2012

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 added a commit that referenced this pull request Nov 27, 2012
Support normal gem require without explicit 'gem "name"' for default gem
@evanphx evanphx merged commit 0c54ab8 into ruby:master Nov 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants