Skip to content

lib/bundled_gems.rb: more reliable caller detection #11322

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

Closed
wants to merge 1 commit into from

Conversation

casperisfine
Copy link
Contributor

The 2 skipped frames went out of sync and now it should be 3.

Rather than just update the offset, we can implement a way that is adaptative as long as all require decorators are also called require.

Also we should compute the corresponding uplevel otherwise the warning will still point decorators.

Using the script reported at: #10351 (comment)

I also improved the message slightly:

$ ruby /tmp/bundled.rb 
/Users/byroot/.gem/ruby/head/gems/childprocess-5.0.0/lib/childprocess.rb:7: warning: /opt/rubies/head/lib/ruby/3.4.0+0/logger.rb was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.
Also please contact the author of childprocess-5.0.0 to request adding logger into its gemspec.

I'd really like to add a test for this, but I really don't see how I could...

@mattbrictson @hsbt

The `2` skipped frames went out of sync and now it should be `3`.

Rather than just update the offset, we can implement a way that
is adaptative as long as all require decorators are also called require.

Also we should compute the corresponding `uplevel` otherwise the
warning will still point decorators.
@byroot byroot requested a review from hsbt August 7, 2024 09:06
Copy link
Contributor

@mattbrictson mattbrictson left a comment

Choose a reason for hiding this comment

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

I confirmed on my machine that this PR fixes the problem I reported in #10351, but I am not sure if the solution is working entirely as intended; see comment below.

@@ -168,7 +194,7 @@ def self.build_message(gem)
end
end
if caller_gem
msg += " Also contact author of #{caller_gem} to add #{gem} into its gemspec."
msg += "\nAlso please contact the author of #{caller_gem} to request adding #{gem} into its gemspec."
Copy link
Contributor

@mattbrictson mattbrictson Aug 7, 2024

Choose a reason for hiding this comment

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

For some reason, when using this branch I am not seeing the "please contact the author" message when I run the reproduction script from #10351 (comment).

$ ruby -v
ruby 3.4.0dev (2024-08-07T09:05:43Z improve-bundled-gems e4e2ab05f0) [x86_64-darwin23]
$ ruby bundled.rb
/Users/mbrictson/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/childprocess-5.0.0/lib/childprocess.rb:7: warning: /Users/mbrictson/.rbenv/versions/ruby-dev/lib/ruby/3.4.0+0/logger.rb was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.
$

@hsbt
Copy link
Member

hsbt commented Aug 8, 2024

@casperisfine Thanks.

I merged this at 7594a29 and 7e0910a.

I update you patch with Ruby 3.3 support because I want to backport bundled_gems.rb to ruby_3_3. Please let me know, my change is wrong.

@hsbt hsbt closed this Aug 8, 2024
@casperisfine
Copy link
Contributor Author

Thank you @hsbt

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.

4 participants