Skip to content

Add Gem::Specification#required_engine_version#1468

Closed
djberg96 wants to merge 2 commits intoruby:masterfrom
djberg96:engine_version
Closed

Add Gem::Specification#required_engine_version#1468
djberg96 wants to merge 2 commits intoruby:masterfrom
djberg96:engine_version

Conversation

@djberg96
Copy link
Copy Markdown
Contributor

This is a followup to issue #1263.

This PR adds the Gem::Specification#required_engine_version member to deal with the limitations of the required_ruby_version, which was originally put in place when there was only one implementation. With multiple Ruby implementations now in the wild, it's necessary to not only specify a Ruby version, but an engine version as well in some cases.

I've also added a Gem.ruby_engine_version singleton method. There's already a Gem.ruby_engine singleton, so it makes sense to add it, and I use it internally in the installer code.

There is one test failure:
1) Failure:
TestGemSpecification#test_handles_private_null_type [test/rubygems/test_gem_specification.rb:1124]:
Expected: nil
  Actual: "developers@soundcloud.com"

Which is coming from this test:

def test_handles_private_null_type
  path = File.join DATA_PATH, "null-type.gemspec.rz"

  data = Marshal.load Gem.inflate(Gem.read_binary(path))

  assert_equal nil, data.rubyforge_project
end

Perhaps the null-type.gemspec.rz file needs to be regenerated? I'm not sure what the proper thing to do is.

spec.instance_variable_set :@platform, array[16].to_s
spec.instance_variable_set :@license, array[17]
spec.instance_variable_set :@metadata, array[18]
spec.instance_variable_set :@required_engine_version, array[8]
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.

This has to go at the end or it its incompatible with previous versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, dang, I was trying to keep the related stuff together. Ok.

@drbrain
Copy link
Copy Markdown
Member

drbrain commented Feb 5, 2016

You'll also need to update SPECIFICATION_VERSION and MARSHAL_FIELDS, but it may be better to put required_engine_version in the metadata so we don't have to mess with those.

Daniel Berger added 2 commits April 3, 2016 13:22
Added a Gem.ruby_engine_version singleton method and use it within the installer.

Reordered fields, updated CURRENT_SPECIFICATION_VERSION and MARSHAL_FIELDS, and updated specs.
@djberg96
Copy link
Copy Markdown
Contributor Author

djberg96 commented Apr 4, 2016

@drbrain Updated.

@djberg96
Copy link
Copy Markdown
Contributor Author

@copiousfreetime @segiddins @krainboltgreene Whaddya think?

@segiddins
Copy link
Copy Markdown
Contributor

I'm personally not ready to ship this yet -- there's a lot of stuff that would need to be put in place for this to be used across the ecosystem (updating the bundler dependency API, updating the new index format specification, updating bundler itself to take this into account) and I'm honestly not convinced that this is worth doing -- it seems like a bandaid on the current multiplatform situation, and given we plan on actually tacking that area head-on in the near future, this could well end up just being baggage

@krainboltgreene
Copy link
Copy Markdown

For what it's worth npm stopped enforcement of the engine metadata.

@djberg96
Copy link
Copy Markdown
Contributor Author

@segiddins You say that we plan on tackling this in the near future, but I've seen no mention of it, nor have I seen any sort of proposal anywhere for how to better deal with this. Meanwhile, there's at least a few folks out there waiting for some sort of solution.

@segiddins
Copy link
Copy Markdown
Contributor

@djberg96 if we merge this now, we'll have to support it basically forever. I know @indirect and the ruby together team intend to tackle platform support right after the bundler / rubygems merger is complete, which will be this fall.

@djberg96
Copy link
Copy Markdown
Contributor Author

@segiddins Alright, closing for now.

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