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

Extract rackup command, Rack::Server, Rack::Handler and related code into a separate gem. #1937

Merged
merged 4 commits into from
Aug 4, 2022

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Aug 1, 2022

We discussed the options here and the general consensus seems to be to extract it. #1928

This PR removes all the rackup specific code into https://github.com/rack/rackup

I've released a v0.1.0 gem for testing, and confirmed it works with this branch.

The following classes were moved:

  • Rack::Server -> Rackup::Server
  • Rack::Handler -> Rackup::Handler. Some minimal function was retained for compatibility.
  • Rack::Lobster -> Rackup::Lobster.

I've moved almost all the code verbatim with minimal changes.

There are several follow up actions but I suggest we make this as direct lift and shift as possible. Later on we can improve rackup independently of rack.

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

As stated previously, I don't think the benefits of extracting rackup outweigh the backwards compatibility breakage. However, since we've decided to move in that direction, in terms of implementation, this looks mostly fine.

Can you explain why Rack::Handler#{register,[]} were left? Is it because web servers use those and you don't want to break that? If so, shouldn't both issue deprecation warnings? Ultimately, setting of handlers is for the benefit of rackup, not rack itself, if I understand things correctly. So we would want the webservers to check Rack.release and:

  • For >= 3, if they want to support rackup, switch to Rackup::Handler (if only defined, or attempting to require first and defining only if successful?). If they don't want to support rackup, do nothing.
  • For < 3, continue using Rack::Handler.

@ioquatix ioquatix changed the title Extract rackup Extract rackup command, Rack::Server, Rack::Handler and related code into a separate gem. Aug 4, 2022
@ioquatix ioquatix merged commit 882cbb3 into main Aug 4, 2022
@ioquatix ioquatix deleted the extract-rackup branch August 4, 2022 04:08
dentarg added a commit to dentarg/rack that referenced this pull request Sep 7, 2022
Does not work since the rackup extraction (rack#1937).
@dentarg dentarg mentioned this pull request Sep 7, 2022
ioquatix pushed a commit that referenced this pull request Sep 9, 2022
Does not work since the rackup extraction (#1937).
ElMassimo pushed a commit to ElMassimo/jekyll-vite that referenced this pull request Sep 9, 2022
stbenjam added a commit to stbenjam/origin that referenced this pull request Sep 13, 2022
This test is failing in CI:

```
[sig-builds][Feature:Builds][Slow] s2i build with environment file in
sources Building from a template should create a image from
"test-env-build.json" template and run it in a pod
[apigroup:build.openshift.io][apigroup:image.openshift.io]
```

In rack/rack#1937, rackup was moved to a
different gem, which is what the s2i ruby config relies on:

https://github.com/sclorg/s2i-ruby-container/blob/master/2.2/s2i/bin/assemble#L52

This adds "rackup" gem.
stbenjam added a commit to stbenjam/origin that referenced this pull request Sep 13, 2022
This test is failing in CI:

```
[sig-builds][Feature:Builds][Slow] s2i build with environment file in
sources Building from a template should create a image from
"test-env-build.json" template and run it in a pod
[apigroup:build.openshift.io][apigroup:image.openshift.io]
```

In rack/rack#1937, rackup was moved to a
different gem, which is what the s2i ruby config relies on:

https://github.com/sclorg/s2i-ruby-container/blob/master/2.2/s2i/bin/assemble#L52

This adds "rackup" gem.
@ppibburr
Copy link

ppibburr commented Oct 10, 2022

Should have never took the lobster out

  1. Example code is everywhere with require 'rack/lobster
  2. Rack::Lobster is word play on Rock-Lobster, not Rockup-Lobster

@ioquatix
Copy link
Member Author

Can you share some of the example code that uses this?

blooper05 added a commit to blooper05/LGTuMblr-api that referenced this pull request Oct 13, 2022
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/origin that referenced this pull request Oct 19, 2022
This test is failing in CI:

```
[sig-builds][Feature:Builds][Slow] s2i build with environment file in
sources Building from a template should create a image from
"test-env-build.json" template and run it in a pod
[apigroup:build.openshift.io][apigroup:image.openshift.io]
```

In rack/rack#1937, rackup was moved to a
different gem, which is what the s2i ruby config relies on:

https://github.com/sclorg/s2i-ruby-container/blob/master/2.2/s2i/bin/assemble#L52

This adds "rackup" gem.
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/origin that referenced this pull request Oct 19, 2022
This test is failing in CI:

```
[sig-builds][Feature:Builds][Slow] s2i build with environment file in
sources Building from a template should create a image from
"test-env-build.json" template and run it in a pod
[apigroup:build.openshift.io][apigroup:image.openshift.io]
```

In rack/rack#1937, rackup was moved to a
different gem, which is what the s2i ruby config relies on:

https://github.com/sclorg/s2i-ruby-container/blob/master/2.2/s2i/bin/assemble#L52

This adds "rackup" gem.
@rubyFeedback
Copy link

rubyFeedback commented Sep 17, 2024

Recently I re-organized my web-related ruby code. Still going through ...

One component was rack + sinatra. So when I looked at rack, I found that in my old code for the lobster:

 Rack::Server.start(
    app: Rack::ShowExceptions.new(Rack::Lint.new(Rack::Lobster.new)), Port: PORT_TO_USE
 )

It took me a while to find out that Rack::Server is not part of rack anymore. Finally I found this here,
and I have to agree with jeremy that I don't think it was worth the change - but ok, pointless to debate
it after it was already changed. How did I find out that Rack::Server is not part of rack anymore though?

I read the changelog.md file. At the least the change was documented.

Still, I find this was not a good change. I believe that IF such things are changed, they should be
instantly documented on the main README as well. Not everyone can find things anymore -
Google search sucks now. Things I could find in the past, I can no longer find. And it is hard to
search for Rack::Server (I found it via github, but not via google search).

I also agree that the lobster is epic and should not be tampered with. Back when chris2 wrote
rack, I recall I tested the lobster. It was addictive: flip, flop, flip, flop. Now the lobster appears
to have been put in another box (Rack::Server). I don't object to clean up if you really want to,
but please consider that downstream people also have to find things again.

So my first suggestion here is:

  1. On https://github.com/rack/rack mention where these former internal gems can now be found.
    Right now as I type this, I still don't know where Rack::Server is now. I may eventually find it, but
    since this already took away several minutes, I would love to see the README mention this (or
    a separate document, linked in from the main README). You don't have to add a detailed description,
    but ideally the name, and the URL. Perhaps a table would be useful here; the main README has
    such a table too. Can be short and succinct, just so we can click on things and find the repository
    where the oldschool lobster is now hiding in.

  2. I'd also love to see some simple examples in the github repository, perhaps under examples/.
    This can be simple, but just so we can get up-to-date again quickly. Ideally these simple examples
    "just work". Right now I have to go through each example and find out what changed upstream,
    and there is barely any documentation for it. With simple examples this could reduce my time
    investment too. Would it also be perhaps possible to add a did-you-mean gem improvement?
    E. g. Rack::Server namespace not known but it could be registered where the repository is. But
    that's probably too complicated, I Just wanted to mention it here as well, in addition for a
    request for more simple examples that are guaranteed to work, and perhaps reside under
    examples/.

@rubyFeedback
Copy link

I should also point out again, just so that people understand this, that google search really sucks now. Finding examples and working code is so much harder than it used to be. Because of this I think it is SUPER important that ruby in general improves its documentation everywhere. Otherwise with projects that are abandoned yet changing code and nobody updating the documentation, we are operating in a black box made worse by Google having ruined its search engine (and the replacements also suck; I tried to switch to duckduckgo, and it consistently produces even more garbage results than Google. I have no idea what happened, but either way my conclusion is that documentation must be improved in general, everywhere - and particular in ruby-based projects. Some document very nicely, such as Jeremy's projects, but other projects kind of lack documentation. Rack is somewhere in the middle, though such breaking code changes also intensify the need to have better documentation overall. I keep local documentation too, but just as the example of Rack::Server shows, I may end up being dumbfounded for a while and then it takes some time to get up-to-date again. That change is not even that old, ~2 years ago, so you can see how quickly things decay quality-wise.)

@ioquatix
Copy link
Member Author

Thanks for your feedback.

I've added a link to rackup from the "See Also" section and mentioned the rename from Rack::Server to Rackup::Server. https://github.com/rack/rack?tab=readme-ov-file#see-also

This was also mentioned in the upgrade guide, and it has some examples, although not as extensive as you probably want.

Feel free to contribute an examples/ directory via a PR if you have specific examples in mind that you think would be useful.

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