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

Remove rackup server #1607

Closed
wants to merge 3 commits into from
Closed

Conversation

xjunior
Copy link

@xjunior xjunior commented Feb 25, 2025

Description

Rackup::Server was removed on 1.0.1. There's no much explanation on their changelog, and the git blame doesn't give up much information either. Fact is that this class doesn't exist.

The first commit simply adds this dependency, which breaks the specs. The second fixes it.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@xjunior xjunior marked this pull request as ready for review February 25, 2025 02:11
@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

Can you provide an explanation as to why this change is necessary? What is broken that this fixes?

@xjunior
Copy link
Author

xjunior commented Feb 25, 2025

@lsegal Rackup::Server was removed on rackup 1.0.1. So requiring rackup works, but the constant doesn't exist and this breaks. This started happening for us on Rails 7.1, as it adds a direct dependency to rackup >~ 1.0

1 similar comment
@xjunior
Copy link
Author

xjunior commented Feb 25, 2025

@lsegal Rackup::Server was removed on rackup 1.0.1. So requiring rackup works, but the constant doesn't exist and this breaks. This started happening for us on Rails 7.1, as it adds a direct dependency to rackup >~ 1.0

@xjunior
Copy link
Author

xjunior commented Feb 25, 2025

@lsegal if you go back to the first commit of this branch and run the specs you'll see them fail

@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

This looks like a third-party issue with a version of rackup 1.x, but I'm not sure which. With rackup 2.x it works fine:

❯ ruby -rrackup -e 'p Rackup::VERSION, Rackup::Server'
"2.2.1"
Rackup::Server

@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

This is a downstream issue with rackup, not something YARD should change. If a minor version of rackup broke the API, you should file an issue with that project. You could alternatively lock to 1.0.0. Given that this bug only exists in one patchlevel release of the gem, I don't see this as something that YARD should be responsible for working around.

@xjunior
Copy link
Author

xjunior commented Feb 25, 2025

@lsegal it's really confusing, honestly. If you look into rackup the change from 1.0.0 to 1.0.1 is Fix rackup.rb invalid requires.. But in fact they removed everything from the project. Since there's no proper history on this change, I'm not sure which one is broken, but it's a fact that YARD is affected by this in Rails 7.1.

@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

Seems like a bug that should be reported to the rackup project as they broke compatibility across versions.

@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

FYI you should be capable of providing gem 'rackup', '= 1.0.0' in your Rails Gemfile to lock the version and workaround this issue, so I'm not entirely sure that "affected by this in Rails 7.1" is necessarily accurate. It certainly might be an issue out of the box, but that shouldn't really be a blocker.

@xjunior
Copy link
Author

xjunior commented Feb 25, 2025

@lsegal what concerns me is the update message "remove invalid require". There's no much info about what they meant with that, and whether that's actually accurate, as what they did in fact was wipe up the library. Assuming wiping was wrong, this lib would have to explicitly require rackup/server instead.

Regardless, I'm making this contribution as a supporter and user of both libraries, but in our case we're just removing the Yard::Server we have hosted for our internal APIs as this is not our primary way of using them. That unblocks us.

@lsegal thank you for your service and quick replies!

@lsegal
Copy link
Owner

lsegal commented Feb 25, 2025

what concerns me is the update message "remove invalid require".

It must be a mistake, as rackup 2.x retains this constant, but I will leave it up to them to respond properly.

@xjunior xjunior closed this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants