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

Restore rack 1.6 compatibility #3156

Merged
merged 5 commits into from May 16, 2023

Conversation

severin
Copy link
Contributor

@severin severin commented May 15, 2023

Description

The constant Rack::RELEASE was introduced "only" (I know, it's a long time ago) in rack version 2 (specifically: rack/rack@a2fe30a). Earlier versions provided the class method Rack.release which still exists.

#3061 has introduced usage of Rack::RELEASE and therefore broke compatibility with rack 1.6.X. Since this was released with a minor or patch version update of puma I feel it would be nice to fix and restore compatibility with rack 1.6.X.

So this PR changes usage of Rack::RELEASE to Rack.release. Since none of the occurrences seem to be in any code that is called per request I assume it's fine to introduce a potential tiny performance overhead of a method call vs. constant lookup.

Please let me know if this makes sense and let me know if I can/should change anything with this PR.

I didn't add any tests since I didn't know how to best approach this: rack is not a dependency of puma and there does not seem to be any infrastructure in place to run tests against different versions of rack (I now see that there's a env var that controls rack version for tests). Since rack 1.6.X is very outdated I would say in this case it's "good enough" if the change does not introduce any regressions but let me know if you have any ideas on how to better test this!

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.

The constant Rack::RELEASE was introduced only in rack version 2
(specifically: rack/rack@a2fe30a)
Earlier versions provided the class method Rack.release which still
exists.

puma#3061 has introduced usage of
Rack::RELEASE and therefore broke compatibility with rack 1.6.X.
@dentarg
Copy link
Member

dentarg commented May 15, 2023

(I now see that there's a env var that controls rack version for tests).

Yes, I think one rack1 job would make sense. Can you add it?

@dentarg
Copy link
Member

dentarg commented May 15, 2023

Shouldn't be too hard I hope, here's the relevant places

puma/Gemfile

Lines 14 to 15 in 898dc44

gem "rack", (ENV['PUMA_CI_RACK_2'] ? "~> 2.2" : ">= 2.2")
gem "rackup" unless ENV['PUMA_CI_RACK_2']

- name: Test with Rack 2
if: |
(matrix.rack-2 == ' rack2') &&
(needs.skip_duplicate_runs.outputs.should_skip != 'true')
shell: bash
run: echo 'PUMA_CI_RACK_2=true' >> $GITHUB_ENV

- name: Test with Rack 2
if: |
(matrix.rack-2 == ' rack2') &&
(needs.skip_duplicate_runs.outputs.should_skip != 'true')
shell: bash
run: echo 'PUMA_CI_RACK_2=true' >> $GITHUB_ENV

@MSP-Greg
Copy link
Member

@severin

I'm looking at this, may have additions shortly...

@severin
Copy link
Contributor Author

severin commented May 15, 2023

Yes, I think one rack1 job would make sense. Can you add it?

Yes, I can do that. Will work on it tomorrow morning (if @MSP-Greg hasn't done it by then 😅, looking forward to their additions).

@MSP-Greg
Copy link
Member

MSP-Greg commented May 15, 2023

Pushed commits, it's working correctly. Running one job with Ruby 2.4.

The CI in Rack 1.6.13 (Travis, no Actions) stopped at Ruby 2.3, along with ruby-head, which I suspect was Ruby 2.4.

So, although current CI passes with it, not sure about full compatibility...

@severin
Copy link
Contributor Author

severin commented May 15, 2023

Pushed commits, it's working correctly. Running one job with Ruby 2.4.

The CI in Rack 1.6.13 (Travis, no Actions) stopped at Ruby 2.3, along with ruby-head, which I suspect was Ruby 2.4.

So, although current CI passes with it, not sure about full compatibility...

Oh wow, so much more changes needed for rack 1 support 😅 My feeling was wrong and adding a test environment for it was worth it in the end 😬

For some more context: we are currently running puma version 6.0.2 in production (without issues) and we are using the rack 1.6 fork maintained by the Rails LTS group: https://github.com/rails-lts/rack/tree/lts-1-6-stable

I would be happy if this PR gets merged, even if it includes a big disclaimer that rack 1.6 support is only best effort.

@nateberkopec nateberkopec merged commit 3d156ed into puma:master May 16, 2023
64 checks passed
@nateberkopec
Copy link
Member

Thanks for the initial work @severin and thanks Greg for getting it over the line.

@latortuga
Copy link

latortuga commented May 22, 2023

Thank you for this fix, I just ran into it in a similar situation as @severin, running the Rails LTS fork of Rack 1.4. ❤️ Looking forward to it being released!

@dentarg dentarg added the bug label May 30, 2023
@dentarg dentarg changed the title Use Rack.release over Rack::RELEASE (rack 1..6.X compatibility) Restore rack 1.6 compatibility May 30, 2023
@dentarg dentarg mentioned this pull request May 30, 2023
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.

None yet

5 participants