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.

Sign up for GitHub

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

Jump to bottom

Prevent loading with rack 3 #3166

Merged
merged 7 commits into from Jun 8, 2023
Merged

Prevent loading with rack 3 #3166

merged 7 commits into from Jun 8, 2023

Conversation

JoeDupuis
Copy link

@JoeDupuis JoeDupuis commented May 28, 2023 •
edited

Description

See #3164

Puma 5 is not compatible with Rack 3. App generated with the default Rails 6.1 and 7.0 app template will hit this error when they run bundle update.

This check ensures affected users will get a meaningful error message pointing them toward a fix.

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.

@JoeDupuis JoeDupuis mentioned this pull request May 28, 2023
Closed
@JoeDupuis JoeDupuis changed the title Prevent with rack 3 Prevent loading with rack 3 May 28, 2023
@JoeDupuis JoeDupuis mentioned this pull request May 28, 2023
Closed
7 tasks
@JoeDupuis JoeDupuis force-pushed the jd/prevent-loading-on-rack-3 branch 2 times, most recently from 377c1ac to f4d867d Compare May 28, 2023 21:45
@JoeDupuis JoeDupuis marked this pull request as draft May 28, 2023 21:46
@JoeDupuis JoeDupuis force-pushed the jd/prevent-loading-on-rack-3 branch 2 times, most recently from 1e96087 to f84bdb2 Compare May 28, 2023 22:15
@MSP-Greg
Copy link
Member

@JoeDupuis

Monday (US) I'll add some commits (some are backports) to Puma 5 to get the CI passing. This can then be rebased.

It will probably require a special test to start a sub-process with Puma loaded with Rack 3, similar to the other tests that check 'restart' functionality with changed dependencies.

@JoeDupuis JoeDupuis force-pushed the jd/prevent-loading-on-rack-3 branch from f84bdb2 to d9fafbf Compare May 29, 2023 05:34
@JoeDupuis
Copy link
Author

It will probably require a special test to start a sub-process with Puma loaded with Rack 3, similar to the other tests that check 'restart' functionality with changed dependencies.

I wrote a test for it. I tried to stay as close as possible in style to other examples I could find, like the restart ones you suggested.

I tested the validity of the test by commenting out the fix.

I tried to reuse some existing helpers, but I had to do some small modifications.
Perhaps we don't want that? As this introduce diff between the helpers of the 5-*-stable branch and the main branch. I can re-write the test without helpers if that's preferred.

@dentarg
Copy link
Member

dentarg commented May 29, 2023

I tried to reuse some existing helpers, but I had to do some small modifications.
Perhaps we don't want that? As this introduce diff between the helpers of the 5-*-stable branch and the main branch

I think that is fine, the test suite is constantly evolving in main

@MSP-Greg
Copy link
Member

@JoeDupuis

I just pushed #3167, which fixes the v5 CI. There will be conflicts with this PR. Also, I've found that rebasing someone else's PR can get messy, so if you can do so after #3167 is merged?

Since some people may be pinned to 5.6, maybe this PR should be on 5-6-stable? I don't think there will be a 5.7, as that implies new features, which is unlikely. @nateberkopec ?

Thanks for the PR.

@JoeDupuis
Copy link
Author

JoeDupuis commented May 29, 2023 •
edited

Also, I've found that rebasing someone else's PR can get messy, so if you can do so after #3167 is merged?

Sure! Happy to do it!

maybe this PR should be on 5-6-stable?

Yeah, the PR is targeting 5-7, but they are both on the same commit, I was expecting that we would merge it to one and do a fast forward merge on the other (keeping them on the same commit again).

@MSP-Greg
Copy link
Member

I merged the PR and cancelled the two frozen CI runs, apparently CI freezes when the OS platform is unknown. Kind of odd. Previous CI had macos-10.15, which is no longer available...

@dentarg
Copy link
Member

dentarg commented May 29, 2023

Since some people may be pinned to 5.6, maybe this PR should be on 5-6-stable? I don't think there will be a 5.7, as that implies new features, which is unlikely. @nateberkopec ?

There's #3055

@MSP-Greg
Copy link
Member

@dentarg

Thanks, Missed that. Regardless, if people are pinned to 5.6, it would probably be a good idea to have this for 5.6.6?

@MSP-Greg
Copy link
Member

I just pushed #3167 to 5-7-stable. CI running now.

@JoeDupuis JoeDupuis force-pushed the jd/prevent-loading-on-rack-3 branch from 8299aba to 5167b71 Compare May 30, 2023 03:14
@MSP-Greg
Copy link
Member

@JoeDupuis

I've got patch for this at MSP-Greg@ede389a

I can push it, or you can add it. Thanks for your work on this.

@JoeDupuis
Copy link
Author

Merged! 🙌

@JoeDupuis
Copy link
Author

Reading the patch and thinking I really forgot to rename the class ?! 🤦‍♂️ 😂

@MSP-Greg
Copy link
Member

I really forgot to rename the class

No problem.

A few years ago ruby/ruby had the same issue. There were 'pure Ruby' tests files, and more files that checked the Ruby C API. There was duplication of some test class names, which resulted in squirrelly errors that were dependent on which test file was loaded last.

The failure is an intermittent failure unrelated to this PR.

@dentarg @nateberkopec

Where should this go (5-6-stable or 5-7-stable), or both?

@dentarg
Copy link
Member

dentarg commented May 30, 2023

Where should this go (5-6-stable or 5-7-stable), or both?

I'll let @nateberkopec be the judge of that because he's the one doing releases, I have no preference

@MSP-Greg
Copy link
Member

MSP-Greg commented May 30, 2023 •
edited

I was multi-tasking this morning and just recalled some items. See this additional patch for changes to puma.rb & rack/version_restriction.rb.

Several reasons for the changes, first being when I saw require 'rack/version_restriction', I wondered if it was a Rack file. Using require_relative makes it clear it is not. Also, added some comments.

Anyone have any thoughts about the changes in the patch?

@dentarg
Copy link
Member

dentarg commented May 30, 2023

Anyone have any thoughts about the changes in the patch?

LGTM

@JoeDupuis JoeDupuis marked this pull request as ready for review May 31, 2023 04:53
@JoeDupuis
Copy link
Author

Merged your patch

@nateberkopec
Copy link
Member

Let's open this against 5-6-stable, I don't want this to block on #3055.

@JoeDupuis JoeDupuis changed the base branch from 5-7-stable to 5-6-stable June 1, 2023 04:46
@JoeDupuis
Copy link
Author

Changed the base, but we should also push this in 5.7 before it is published.
For the fix to be effective it needs to be present on the latest 5.*

@JoeDupuis JoeDupuis force-pushed the jd/prevent-loading-on-rack-3 branch from de11d4d to 9e9eb73 Compare June 1, 2023 05:15
@MSP-Greg
Copy link
Member

MSP-Greg commented Jun 1, 2023

ruby head builds had an ABI change that nio4r didn't like. It's been reverted, and the head builds have been updated in GitHub Actions.

So, I reran the failing heads builds here.

@JoeDupuis
Copy link
Author

Looks like it passed the tests, ready to merge?

@nateberkopec nateberkopec merged commit 6dac5d9 into puma:5-6-stable Jun 8, 2023
54 checks passed
@nateberkopec
Copy link
Member

I don't think we have anything else waiting for a backport to 5-6-stable?

JoeDupuis added a commit to JoeDupuis/puma that referenced this pull request Jun 10, 2023
@JoeDupuis @MSP-Greg
Prevent loading with rack 3 (puma#3166)
aec9425 
* Prevent booting with rack 3

* Move with_unbundled_env to integration helpers

* Version restriction test environment

* Allow skipping the wait period with the cli_server helper

* Test for the rack version restriction

* Fixup & Rename test file

* Minor puma.rb and rack/version_restriction.rb changes

---------

Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>
@JoeDupuis JoeDupuis mentioned this pull request Jun 10, 2023
Closed
@JoeDupuis
Copy link
Author

I don't think we have anything else waiting for a backport to 5-6-stable?

Do you mean 5-7? I just opened the PR if that's the case.

@JoeDupuis
Copy link
Author

Came back to check on the PR after watching your talk at RubyConf AU. Felt like I reminder 😆

@nateberkopec
Copy link
Member

Sorry I was talking to myself/maintainers to see if I could release 5-6-stable as the next version

@JoeDupuis
Copy link
Author

Oops 😆 Got it

@MSP-Greg MSP-Greg mentioned this pull request Jun 21, 2023
Merged
jch added a commit to jch/rails that referenced this pull request Oct 28, 2023
@jch
Default gemfile generator to puma >= 6.0
1958cd5 
Puma 5.x is not compatible with Rack 3 cookies. The current default
new Gemfile uses puma ">= 5.0". Puma ~> 5.6 will error
on start when used with Rack 3, but earlier versions will not.

puma/puma#3164: Support Rack 3 cookies
puma/puma#3166: Prevent loading with rack 3
phusion/passenger#2503: Related passenger ex
@jch jch mentioned this pull request Oct 28, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoeDupuis @MSP-Greg @dentarg @nateberkopec