fix logic of register_default_spec

[jaggederest]

Register default spec currently only checks the first file to see if it should use the new spec type (i.e. lib/foo.rb) or the old one (i.e. foo.rb).

This patch fixes that misbehavior at the cost of (possibly) going through all files for each require path.

[drbrain]

I think this is correct since we can't depend on ordering in #files. @headius, can you check it?

[headius]

It was intentional. No code out there generates a default spec with both layouts. Do you have a counter-example?

[headius]

By counter-example I mean a case where scanning all the files is necessary. For large default gems it could be prohibitively slow at boot.

[drbrain]

If we apply #612 the test for this behavior breaks as bin/ comes first in the list. rdoc is a default gem that would have a bin/ first.

@jaggederest can you change the new_format check to look for bin/ in addition to require_paths so only the first item needs to be checked? I think this will allow us to check only the first item.

I do prefer that the new version doesn't use a loop side-effect to do the checking, it's easier to understand than the three-state new_format variable of the original.

[jaggederest]

So here's a question: Is there ever going to be a gem which uses the new format but does not contain the character '/' in the file list, or vice versa? Is there any other way of detecting the format type?

By which I mean we could replace

spec.require_paths.any? {|path| spec.files.any? {|f| f.start_with? path } }

with

spec.files.any? {|f| f.includes?('/') } || spec.require_paths.any? {|path| spec.files.first.start_with? path }

Which would iterate across the files once and the paths once, O(n) instead of the files x paths O(n^2) if we knew that only new-format gems include '/'

Alternately, if that's not true, we could do:

spec.files.any? {|file| file.include?('/') && spec.require_paths.any? {|path| file.start_with? path }}

Which reduces the files to the subset that could include a require path (e.g. not those that are, say ABOUT, or aardvark.yml, or something sorting before 'bin' located in the root directory)

Also to note it's only O(n^2) in the case that it's the old format - how many gems are the old format and also have many files and require paths? New format should bail out of the short circuit within a couple iterations.

[headius]

Ruby 2.0 default gems are in the old format, so it would be O(n^2) on all Ruby 2.0 installs for the gems they have as default.

I guess I'm still not seeing what the failure case is here for the simpler logic. Can you provide a real-world example of a default gem spec that would break?

[drbrain]

@headius right now it is fine, but with #612 applied this test fails: https://travis-ci.org/rubygems/rubygems/jobs/9715205#L106

@jaggederest old format gems look like this (YAML):

$ gem spec rdoc -v 4.0.0 files
---
- rdoc.rb
- rdoc/alias.rb
[…]
- rdoc/top_level.rb
- bin/rdoc
- bin/ri

$ gem spec io-console files
---
- io/console/size.rb
- extconf.rb

New format will have an extra lib/ on the items that currently don't (for the rdoc example).

There's no guarantee the first item in an old-format gem won't have a /.

[jaggederest]

Did some back of the envelope benchmarking and it looks like doing it the way I have it is O(n^2) but has a smaller constant time than the other similar methods I mentioned above.

Total number of files in all default gems at the moment is 257, total number of require paths per gem is one or two, so I think it's ok to do it the less computationally intensive way, if this only applies to default gems not everything ever.

@headius Does JRuby include other gems in the default set that I should be checking when I do my pseudo-benchmarking? Any with, say, dozens of include paths and thousands of files, for example? Otherwise I think this is an ok patch.

[headius]

Ok, getting back into this today finally.

JRuby and MRI only include a handful of default gems right now, but that could change in the future. However...am I right in thinking this scan could completely go away if we were able to just use the new format?

I have a compromise position...

Whatever patch resolves this is fine with me if there's a way for the per-impl defaults.rb file to specify that it only supports either new or old-style defaults. Current default will be to scan and determine which format to use. Future default would be to always assume new-style defaults, perhaps with specific MRI versions filtered out to use old style (only 2.0, hopefully).

This would allow implementations to opt out of the scanning altogether by specifying which format they use, and the complexity of the scan would be irrelevant.

[drbrain]

I like the compromise position. To make it easier on JRuby, CRuby, etc. we can set the default based on RUBY_ENGINE. If it is not "ruby" use the new style. When CRuby switches to the new style we can add a version check for "ruby"-only.

If someone wants to use the old style they can override the default.

@jaggederest do you want to implement it?

[jaggederest]

On it! :)

[headius]

Good call, @drbrain. Should work on JRuby with new format out of the box then.

[jaggederest]

I'd appreciate it if one of you fine fellows would test it on non-mri versions of ruby, I only have 2.0 installed on this machine. (and of course the mark one eyeball check is also appropriate 😃 )

[drbrain]

Can you move the default to a new method in lib/rubygems/defaults.rb?

Maybe def full_path_default_gems (false for RUBY_ENGINE == 'ruby')

[jaggederest]

Updated, should this new method have tests? It seems pretty trivial, but maybe there's a subtle behavior I'm missing?

[drbrain]

A test for the defaults would be nice!

[headius]

Change looks good to me at a glance. Thanks!