Add ruby_34 and ruby_35 as valid platform: #8430
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality. We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below. If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack. For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide |
Contributor
|
@Edouard-chin Yes, please, do consolidate this in a refactor. Don't mind if you want to do it here or as a separate PR! |
Contributor
Author
|
Thanks for the quick reply. Sounds good, I'll do it in this PR! |
Edouard-chin
force-pushed
the
ec-platform-34
branch
2 times, most recently
from
481cd39 to
66944e9
Compare
Edouard-chin
commented
Jan 23, 2025
Contributor
Author
|
Pushed two more commits to move everything related to the ruby version and the ruby platform inside the Thanks ! |
Edouard-chin
force-pushed
the
ec-platform-34
branch
2 times, most recently
from
a15b7a6 to
7b597a5
Compare
Contributor
Author
|
Had to make a small change to one test I added as it was failing on windows. |
Contributor
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
Looks really good, I only had a couple of minor comments.
- We keep 2 list of supported ruby versions and each time a new ruby version is released we need to maintain both list. Forgetting to update one would prevent users from adding gem for a specific plaftorm (i.e. 7cd19d8 and 5462322). Extracted the list from the Dependency class and moved it to the CurrentRuby class (which I believe was originally added for that reason).
- Similar change than 29a1be0, keep a single source of truth where we store the platform. The only change worth highlighing is the platform "maglev". It was not part of the supported platform of dependencies, so calling `gem 'foo', plaftorm: 'maglev'` would not work. However, it was supposed to according to 45ec86e. That's why it was possible to do `Bundler.current_ruby.maglev?` or `Bundler.current_ruby.maglev_30?`. I didn't change the current behaviour and maglev is not supported, though I kept the `*maglev` methods as I believe CurrentRuby is public API.
Edouard-chin
force-pushed
the
ec-platform-34
branch
from
7b597a5 to
29e219e
Compare
Contributor
Author
My comment doesn't appear in the thread but I have answered 😄 : 29e219e#r1932111420 |
deivid-rodriguez
approved these changes
Jan 28, 2025
Contributor
|
Thanks a lot @Edouard-chin! |
Edouard-chin
added a commit
to Edouard-chin/rubygems
that referenced
this pull request
Jan 29, 2025
- Follow up to ruby#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in 45ec86e). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now.
4 tasks
Edouard-chin
added a commit
to Edouard-chin/rubygems
that referenced
this pull request
Jan 29, 2025
- Follow up to ruby#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in 45ec86e). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now.
Edouard-chin
added a commit
to Edouard-chin/rubygems
that referenced
this pull request
Jan 29, 2025
- Follow up to ruby#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in 45ec86e). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now.
Edouard-chin
added a commit
to Edouard-chin/rubygems
that referenced
this pull request
Feb 1, 2025
- Follow up to ruby#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in 45ec86e). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now.
matzbot
pushed a commit
to ruby/ruby
that referenced
this pull request
Feb 5, 2025
… maglev methods: - Follow up to ruby/rubygems#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in ruby/rubygems@45ec86e2e528). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now. ruby/rubygems@66388babf8
deivid-rodriguez
added a commit
that referenced
this pull request
Feb 17, 2025
Add ruby_34 and ruby_35 as valid platform: (cherry picked from commit abd06da)
AthosHeart
pushed a commit
to AthosHeart/rubygems
that referenced
this pull request
Mar 10, 2025
- Follow up to ruby#8430 (comment). The maglev platform was not supported by Bundler, so calling `gem "foo", platforms: ["maglev"]` would raise an error. The helpers added in the `CurrentRuby` class were used at a time when maglev was supported (as explained in 45ec86e). Support of maglev was most likely dropped at some point and the helpers in the `CurrentRuby` class were not deprecated/removed. We decided to deprecate them now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the end-user or developer problem that led to this PR?
Adding
gem 'foo', platform: "ruby_34"doesn't work and raises an error saying that it's not a valid platformWhat is your fix for the problem, implemented in this PR?
ruby_34method missing when runningbundle install#8427Side note
We keep two constants for the list of ruby versions and also two constants for the plaforms, and we need to modify both whenever a new Ruby version gets released. I think this should be consolidated in a small refactor. I didn't do it as I wasn't sure you'd be interested in this. Happy too takes some time to it though.
https://github.com/rubygems/rubygems/blob/186a4f24789e6e7fd967b290ce93ed5886ef22d8/bundler/lib/bundler/dependency.rb#L12
https://github.com/rubygems/rubygems/blob/186a4f24789e6e7fd967b290ce93ed5886ef22d8/bundler/lib/bundler/current_ruby.rb#L12
Make sure the following tasks are checked