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

Allow ::Rack::Handler::Puma.run to work regardless of whether Rack/Rackup are loaded #3080

Merged
merged 4 commits into from Feb 16, 2023

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Feb 13, 2023

Description

PR #3061 fixed the use of the rackup bin file for use with either Rack 2 or Rack 3. It broke the use of ::Rack::Handler::Puma.run when used stand-alone, which is used to start Puma in some CI frameworks.

Fix the above issues, add a test for running the rackup bin file using IO.popen, which isolates it (and what it requires) from the main test process. Note that the test only runs on MRI Ruby, as non-MRI Rubies take time when using IO.popen.

The 'Tests' workflow uses the Rackup (includes the rackup bin) & Rack v3 gems, and the 'Rack_v2' workflow uses Rack v2 gem (includes the rackup bin).

See #2990 (comment)

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.

@MSP-Greg MSP-Greg changed the title Allow ::Rack::Handler::Puma to run regardless of whether Rack/Rackup are loaded Allow ::Rack::Handler::Puma.run to work regardless of whether Rack/Rackup are loaded Feb 13, 2023
@MSP-Greg MSP-Greg merged commit 18d60d4 into puma:master Feb 16, 2023
@MSP-Greg MSP-Greg deleted the 00-fix-rack branch February 16, 2023 04:29
@ghiculescu
Copy link
Contributor

ghiculescu commented Feb 25, 2023

Is it possible to get a 6.1.1 release with this fix? This is blocking our upgrade to 6.1.0 in an app that uses system tests with Capybara - it crashes here.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Feb 25, 2023

turbo-rails is using Capybara & Puma 6.1.0 in their CI. See:

https://github.com/hotwired/turbo-rails/actions/runs/4253429159/jobs/7398287552#step:3:131

Hence, is there more information you can give?

EDIT: turbo-rails is using Rack 2, so never mind if you're using Rack 3. Not sure if that would work...

@ghiculescu
Copy link
Contributor

It's from a private repo, but:

Using rack 3.0.4.1
Using rack-session 2.0.0
Using rack-test 2.0.2
Using rackup 2.0.0
Using capybara 3.38.0
Using puma 6.1.0

@ghiculescu
Copy link
Contributor

But to be clear, this PR fixes the issue. It just hasn't been released to rubygems yet.

nateberkopec pushed a commit that referenced this pull request Feb 28, 2023
…ckup are loaded (#3080)

* rack/handler/puma.rb - fix for use without Rack

* test/test_rack_handler.rb - add test for rackup bin file

* test_rack_server.rb - only load needed Rack files

* test_rack_handler.rb - remove Windows skip for test_bin
@nateberkopec
Copy link
Member

Done 👍

@ghiculescu
Copy link
Contributor

<3 you @nateberkopec

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

4 participants