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

Print RUBY_DESCRIPTION when Puma starts #3407

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

JacobEvelyn
Copy link
Contributor

@JacobEvelyn JacobEvelyn commented Jun 5, 2024

This commit changes the runner to print RUBY_DESCRIPTION instead of a manually-generated string for the Ruby version. Other popular gems like Sidekiq similarly print RUBY_DESCRIPTION. One specific motivation for this change is that RUBY_DESCRIPTION indicates whether YJIT is enabled in MRI.

This commit also deprecates the Puma::Runner#ruby_engine method, which will be removed in v7.

Description

My team was testing YJIT and was frustrated by the fact that Puma does not indicate whether YJIT is running, while other gems like Sidekiq do.

I welcome any suggestions for this PR.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg
Copy link
Member

dentarg commented Jun 5, 2024

Mind showing some before and after examples?

@MSP-Greg
Copy link
Member

MSP-Greg commented Jun 5, 2024

Current:

[172745] Puma starting in cluster mode...
[172745] * Puma version: 6.4.2 (ruby 3.4.0-p-1) ("The Eagle of Durango")

PR:

[172898] Puma starting in cluster mode...
[172898] * Puma version: 6.4.2 ("The Eagle of Durango") (ruby 3.4.0dev (2024-06-05T20:27:00Z master 0b31986909) [x86_64-linux])

After the PR, the line is 127 characters wide. Splitting it to another line might be an idea, but doing so could (maybe) mess up code that is parsing it...

@dentarg
Copy link
Member

dentarg commented Jun 5, 2024

Ideas/alternatives:
Should Ruby has its own line instead?
Should we only add YJIT yes/no?

@ioquatix
Copy link
Contributor

ioquatix commented Jun 6, 2024

RUBY_DESCRIPTION is the correct choice, and it should probably be on its own line. I recently adopted it in Falcon too.

lib/puma/runner.rb Outdated Show resolved Hide resolved
Comment on lines 93 to 94
# @!attribute [r] ruby_engine
def ruby_engine
Copy link
Member

@dentarg dentarg Jun 9, 2024

Choose a reason for hiding this comment

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

This method is public, so we should deprecate it, not remove it.

$ ruby -rpuma -rpuma/runner -e 'p Puma::Runner.new(Puma::Launcher.new(Puma::Configuration.new)).ruby_engine'
"ruby 3.2.4-p170"

Unless this change should happen in Puma v7, but even if so, it is nice to deprecate things we intend to remove.

@JacobEvelyn JacobEvelyn force-pushed the print-ruby-description branch from 52a7c31 to e5c94d3 Compare June 10, 2024 19:26
This commit changes the runner to print `RUBY_DESCRIPTION` instead of
a manually-generated string for the Ruby version. Other popular gems
like Sidekiq similarly print `RUBY_DESCRIPTION`. One specific motivation
for this change is that `RUBY_DESCRIPTION` indicates whether YJIT is
enabled in MRI.

This commit also deprecates the `Puma::Runner#ruby_engine` method,
which will be removed in v7.
@JacobEvelyn JacobEvelyn force-pushed the print-ruby-description branch from e5c94d3 to 0234fce Compare June 10, 2024 19:35
def ruby_engine
warn "Puma::Runner#ruby_engine is deprecated; use RUBY_DESCRIPTION instead. It will be removed in puma v7."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ ruby -I lib -rpuma -rpuma/runner -e 'p Puma::Runner.new(Puma::Launcher.new(Puma::Configuration.new)).ruby_engine'
Puma::Runner#ruby_engine is deprecated; use RUBY_DESCRIPTION instead. It will be removed in puma v7.
"ruby 3.3.2-p78"

@JacobEvelyn JacobEvelyn marked this pull request as ready for review June 11, 2024 19:12
@JacobEvelyn
Copy link
Contributor Author

@dentarg let me know what I should do to move this forward. Thanks!

@dentarg
Copy link
Member

dentarg commented Jun 17, 2024

I think this makes sense. Is there any existing tests for this part of Puma that should be extended?

@dentarg
Copy link
Member

dentarg commented Jun 17, 2024

I recall some change to the Puma logging in the past and I remember @nateberkopec being conservative about it. It would be great if he signed off on this.

If I have time I can try to look for that past discussion.

@JacobEvelyn
Copy link
Contributor Author

JacobEvelyn commented Jun 17, 2024

Is there any existing tests for this part of Puma that should be extended?

I didn't see any, but let me know if you think I should try to add one.

@JacobEvelyn
Copy link
Contributor Author

I recall some change to the Puma logging in the past and I remember @nateberkopec being conservative about it. It would be great if he signed off on this.

@nateberkopec any thoughts on this?

@JacobEvelyn
Copy link
Contributor Author

This change feels fairly uncontroversial to me; what can we do to move it forward?

@dentarg
Copy link
Member

dentarg commented Oct 24, 2024

I recall some change to the Puma logging in the past and I remember @nateberkopec being conservative about it. It would be great if he signed off on this.

If I have time I can try to look for that past discussion.

Maybe I misremembering, I can only find this comment #2817 (comment) and it is probably questionable to call this logging 'behavior'.

We have changed the logging before in a minor release, ee3341d67, so this should be fine. I'll merge it if you rebase @JacobEvelyn

@dentarg
Copy link
Member

dentarg commented Oct 24, 2024

I'll merge it if you rebase @JacobEvelyn

I clicked the Update branch button so no need

@dentarg dentarg self-assigned this Nov 4, 2024
@dentarg dentarg merged commit 4c55a1a into puma:master Nov 5, 2024
77 of 78 checks passed
@JacobEvelyn JacobEvelyn deleted the print-ruby-description branch November 5, 2024 14:23
@nateberkopec
Copy link
Member

Yeah thanks for checking w/me but in this case I think we're good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants