rbinstall - Use Gem::Package like object instead of monkey patching#2515
Merged
nobu merged 6 commits intoruby:masterfrom Feb 26, 2020
Merged
rbinstall - Use Gem::Package like object instead of monkey patching#2515nobu merged 6 commits intoruby:masterfrom
Gem::Package like object instead of monkey patching#2515nobu merged 6 commits intoruby:masterfrom
Conversation
Contributor
Author
|
BTW I wonder who is really using the |
Gem::Package like object instead of monkey patching
5b8f199 to
645ea14
Compare
1. This is similar to what RubyGems does and it is less magic [[1]]. 2. It avoids deprecated code paths in RubyGems [[2]]. [1]: https://github.com/rubygems/rubygems/blob/92892bbc3adba86a90756c385433835f6761b8da/lib/rubygems/installer.rb#L151 [2]: https://github.com/rubygems/rubygems/blob/92892bbc3adba86a90756c385433835f6761b8da/lib/rubygems/installer.rb#L187
rbinstall is using `$script_mode` and `$prog_mode`. However, the `$script_mode` fallbacks to `$prog_mode` if not provided. However, RubyGems do not distinguish between `$script_mode` and `$prog_mode`: https://github.com/rubygems/rubygems/blame/92892bbc3adba86a90756c385433835f6761b8da/lib/rubygems/installer.rb#L196 https://github.com/rubygems/rubygems/blame/92892bbc3adba86a90756c385433835f6761b8da/lib/rubygems/installer.rb#L515 https://github.com/rubygems/rubygems/blame/92892bbc3adba86a90756c385433835f6761b8da/lib/rubygems/installer.rb#L543 Comparing the usage of `$script_mode` and `$prog_mode`, it seems that the `$script_mode` should be used where RubyGems expects `$prog_mode`.
This just gets the `RbInstall::DirPackage` closer by functionality to `Gem::Package`.
The local `path` variable does not provide any additional value and was kept around just for clarity for easier review of the `extrac_files` method move.
.gemspec files specifies not just `bin`, but also other directories.
It is not necessary to strip the `destdir` prefix every iteration, when it can be done just once.
645ea14 to
9564268
Compare
deivid-rodriguez
approved these changes
Feb 24, 2020
Contributor
deivid-rodriguez
left a comment
There was a problem hiding this comment.
This looks good to me, and I think it's a good direction forward 👍.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Use
Gem::Packagelike object instead of monkey patching.I'll try to follow up with modifications, that will allow to use more of standard RubyGems infrastructure to install the StdLib gems. My concern are mainly the default gems, which is related to this ticket [3].