-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix -I require priority #3124
Fix -I require priority #3124
Conversation
This issue was found by the Debian ruby team when trying to package psych for ruby2.7, and this PR should close the related issue they opened to the psych project: ruby/psych#431. |
7bcc9c4
to
fb907f9
Compare
I need to figure out the Windows CI issue, but other than that I think it should be ready. |
If `require "a"` is run when two folders have been specified in the -I option including a "a.rb" file and a "a.so" file respectively, the ruby spec says that the ".rb" file should always be preferred. However, the logic we added in 6b81076d9 to make the -I option always beat default gems does not respect this spec, creating a difference from the original ruby-core's require. [the ruby spec says]: https://github.com/ruby/spec/blob/d80a6e2b221d4f17a8cadcac75ef950c59cba901/core/kernel/shared/require.rb#L234-L246
60a1c91
to
b394438
Compare
Windows CI is passing now too 👍. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks!
begin | ||
if File.symlink? safe_lp # for backward compatibility | ||
next | ||
Gem.suffixes.each do |s| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I confirmed Gem.suffixes
returned ["", ".rb", ".bundle"]
on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I had two doubts here:
- Is the "empty extension" actually needed?
- Should I explicitly loop first through ".rb", and then through the rest of the extensions, so that we don't depend on the order of
Gem.suffixes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of the issues are actually present in the current code, so I'll move this forward for now.
@bundlerbot r=hsbt |
3124: Fix -I require priority r=hsbt a=deivid-rodriguez # Description: If `require "a"` is run when two folders have been specified in the -I option including a "a.rb" file and a "a.so" file respectively, [the ruby spec says](https://github.com/ruby/spec/blob/d80a6e2b221d4f17a8cadcac75ef950c59cba901/core/kernel/shared/require.rb#L234-L246) that the ".rb" file should always be preferred. However, the logic we added in 6b81076d9 to make the -I option always beat default gems does not respect this spec, creating a difference from the original ruby-core's require. # Tasks: - [x] Describe the problem / feature - [x] Write tests - [x] Write code to solve the problem - [x] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded
|
Origin: rubygems/rubygems#3124 Author: deivid <deivid.rodriguez@riseup.net> Gbp-Pq: Name 0006-Fix-priority-order-of-paths-in-I-option.patch
Description:
If
require "a"
is run when two folders have been specified in the -I option including a "a.rb" file and a "a.so" file respectively, the ruby spec says that the ".rb" file should always be preferred. However, the logic we added in 6b81076d9 to make the -I option always beat default gems does not respectthis spec, creating a difference from the original ruby-core's require.
Tasks:
I will abide by the code of conduct.