Skip to content
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

remove explicit require for auto-loaded constant #3751

Merged
merged 1 commit into from Jun 29, 2020

Conversation

kares
Copy link
Contributor

@kares kares commented Jun 22, 2020

I do not think RGs needs:
autoload :Specification, 'rubygems/specification'
as well as
require 'rubygems/specification'

maybe for compatibility we should only keep the require and remove the autoload instead?
the intention seems to have been to only have one, when the require was added but got lost: 0962032 (10f1094)

What was the end-user or developer problem that led to this PR?

This is revealing a rare JRuby 9.2 auto-loading bug: jruby/jruby#6293

I will abide by the code of conduct.

@welcome
Copy link

welcome bot commented Jun 22, 2020

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

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Nice catch! Thus duplication has survived long enough so that you can catch your bug, but now it's time to go! 😃

If CI is good, I think it's fine to leave the autoload and be agressive. If some edge case breaks we can fix it.

@kares
Copy link
Contributor Author

kares commented Jun 23, 2020

Thanks David, appreciate your feedback. Indeed it's been there for 8+ years.
Also made sense to me to just remove the require as it should act pretty much the same for consumers.
Maybe except if someone is checking $LOADED_FEATURES for rubygems/specification which sounds silly.

@kares
Copy link
Contributor Author

kares commented Jun 23, 2020

curious how risky this seems, would this end up in RGs 3.2 or could the change also lend in the 3.1 branch for next release?

@deivid-rodriguez
Copy link
Member

Only 3.2.

@deivid-rodriguez
Copy link
Member

I mean, do you need us to backport this to 3.1 for jruby?

@deivid-rodriguez
Copy link
Member

@kares This sounds like an equivalent issue? Gem::Platform is also autoloaded in lib/rubygems.rb, and explicitly required from rubygems/specification.

@kares
Copy link
Contributor Author

kares commented Jun 25, 2020

This sounds like an equivalent issue? Gem::Platform is also autoloaded in lib/rubygems.rb, and explicitly required from rubygems/specification.

Nice catch, thanks David. Seems like so, let me review all requires and update the PR.
Users likely use Gem::Specification API more so that might be the reason why we've only seen those, so far (from reports).

I mean, do you need us to backport this to 3.1 for jruby?

JRuby's plan was to update RGs to latest 3.1 and patch (for 9.2.12) only if next RGs accepts this PR.
Since users might upgrade RGs not having the patch in next RGs likely makes it not worthy to work-around by patching RGs JRuby ships. And as to why it's not being fixed for real, there's a JRuby load-service rewrite pending and we did not want to spent too much time poking around the current impl, as we might lend a new one in 9.3.

@deivid-rodriguez
Copy link
Member

I'm merging this patch and it will be part of rubygems 3.2, that's for sure.

I would also be fine with backporting it to 3.1. We could consider backporting the inverse patch to be 100% sure that nothing breaks, but I think it should be fine as it is. But backporting depends on @hsbt, which is the person who currently releases rubygems.

@simi
Copy link
Member

simi commented Jun 25, 2020

ℹ️ I can prepare backport patch if needed.

@deivid-rodriguez
Copy link
Member

After reading @kares message again, I understand that as long as we include this with both 3.2, and the next 3.1 patch, it should be fine, right? No matter when we actually release those.

the Gem module's auto-loads will handle loading these as needed,

this started as a redundancy found in *rubygems.rb* which had:
`autoload :Specification, 'rubygems/specification'` as well as
`require 'rubygems/specification'`
@kares
Copy link
Contributor Author

kares commented Jun 27, 2020

I understand that as long as we include this with both 3.2, and the next 3.1 patch, it should be fine, right? No matter when we actually release those.

Yes, that would have been the plan.

I did a review NOT to require autoload-ed dependencies and the changeset has grown.
This could be sold as a startup time improvement as well ... for all Rubies booting RGs.

Let me know what you guys think, does this seem breaking in any ways for the 3.1 backport?

@simi
Copy link
Member

simi commented Jun 27, 2020

I thought those requires are in place to support requiring only specific files if needed, but for example rubygems/specification is not loadable on its own. I do not see any regression in here.

🤔 Any idea why some files are required explicitly and some are required by autoload? Would it be possible to unify this actually?

@MSP-Greg
Copy link
Contributor

Any idea why some files are required explicitly and some are required by autoload?

Maybe required files should only be what's needed for RG's Kernel#reguire to work? Everything else could be 'load on demand'? Unfortunately, this can also affect CI. Having CI/tests require a minimal number of lib files is a good goal, but often impractical...

@deivid-rodriguez
Copy link
Member

Yeah, I can only imagine really weird use cases where this would be breaking. I'm fine with merging this as is, and even backporting it.

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks for the nice cleanup @kares!!

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.

None yet

4 participants