-
Notifications
You must be signed in to change notification settings - Fork 137
1.2: Back-port JRuby 10 and Rails 8.0 compatibility #360
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
Conversation
…or supported JRubies This has been set to 2.1 for all supported JRubies for a long time, and the setting did nothing within JRuby. This cherry-pick avoids changing the API by bringing the class over from JRuby, allowing compatibility with JRuby 10. (cherry picked from commit abc7e0b) # Conflicts: # README.md # src/main/java/org/jruby/rack/DefaultRackConfig.java # src/main/java/org/jruby/rack/embed/Config.java # src/spec/ruby/jruby/rack/integration_spec.rb # src/spec/ruby/rack/application_spec.rb # src/spec/ruby/rack/config_spec.rb
(cherry picked from commit d794a22) # Conflicts: # .github/workflows/maven.yml # CHANGELOG.md # README.md # pom.xml # src/main/ruby/jruby/rack/version.rb
(cherry picked from commit 854c086) # Conflicts: # .github/workflows/maven.yml # gemfiles/rails50.gemfile.lock # gemfiles/rails52.gemfile.lock # gemfiles/rails60.gemfile.lock # gemfiles/rails61.gemfile.lock # gemfiles/rails70.gemfile.lock # gemfiles/rails71.gemfile.lock # gemfiles/rails72.gemfile.lock # gemfiles/rails80.gemfile.lock
(cherry picked from commit c18d0f7)
(cherry picked from commit 7e9bb85)
Must have been a cherry-pick go a bit wild here somewhere.
| * @deprecated since JRuby 9.2 with no replacement; for removal with jruby-rack 1.3.0 | ||
| */ | ||
| @Deprecated | ||
| public enum CompatVersion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is a good idea, with proper modularization shouldn't even work (as the package is "owned" by a different .jar).
would personally just leave 1.2.x as is and not try to hack around to get it to work with latest JRuby...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this did occur to me. However is jruby-rack even possible to use within a modularised environment? It has no metadata. So is this an an academic concern or still real?
I'm yet to actually see such modularisation used in the wild in real applications, so I may be naive here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know an example of a Java web server that works in JPMS module mode? To my knowledge they all still use classpath mode (or OSGi which is a whole other can of worms jruby-rack doesn't support)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly do not know, just smells like a bad idea.
There has been embedded use-cases with different class-path layouts than a typical .war.
Understand where this is coming from given the enum is self contained.
We'll still have the org.jruby.CompatVersion (with <10) class around twice and can not guarantee which one will get loaded, shouldn't matter. 🤷
If you get someone else e.g. @headius to bless this approach. 🙏
Personally would rather not go down this path, esp. given 1.3.0 without hacks is good to go but if there's smarter folks saying will do just fine... then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, almost certainly a bad idea in the general case.
My thinking was just to make life easier for folks to upgrade and avoid issues like jruby/warbler#573 where someone uses JRuby 10 with Warbler and it sometimes works enough to confuse folks, except when there is a start-up error and that error is swallowed by this CompatVersion problem. Warbler doesn't define a maximum jruby version, but does define a < 1.3 jruby-rack version so there is a bit more friction there.
I was thinking that might allow us to keep the delta for 1.3 even smaller for folks - but I hear you..
Alternatively to this what we could possibly do on 1.2.x is disable the jruby-rack config capture (on init failure) for JRuby 10. The actual failure/issue right now only seems to come from the instance_methods call (JRuby MethodGatherer invocation) here:
jruby-rack/src/main/ruby/jruby/rack/capture.rb
Lines 85 to 99 in 2bdcb42
| module JRubyRackConfig | |
| def capture | |
| super | |
| servlet_context = JRuby::Rack.context | |
| methods = servlet_context.config.class.instance_methods(false) + | |
| org.jruby.rack.DefaultRackConfig.instance_methods(false) | |
| methods = methods.uniq.reject do |m| | |
| m =~ /^(get|is|set)/ || m =~ /[A-Z]|create|quiet|([!?=]$)/ | |
| end | |
| output.puts("\n--- JRuby-Rack Config", | |
| *(methods.sort.map do |m| | |
| "#{m} = #{servlet_context.config.send(m)}" rescue "#{m} = <error: #{$?}>" | |
| end)) | |
| end | |
| end |
So if we removed that, and if the user does not retrieve the CompatVersion in custom config, I don't think jruby/jruby-rack will cause the CompatVersion class to be loaded, despite the imports below because the getCompatVersion() method is never invoked.
| import org.jruby.CompatVersion; |
Would that be better to you - to just lose the rack config dump capability on JRuby 10 for 1.2.x? (unless you can think of an even smarter way to stop the .instance_methods(false) call from inspecting everything. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am reasonably confident that JRuby 10 works fine without the config dump being needed because the Warbler integration tests validate startup and a basic call to a Rails API work fine and are passing in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was just to make life easier for folks to upgrade and avoid issues like jruby/warbler#573 where someone uses JRuby 10 with Warbler and it sometimes works enough to confuse folks, except when there is a start-up error and that error is swallowed by this CompatVersion problem. Warbler doesn't define a maximum jruby version, but does define a < 1.3 jruby-rack version so there is a bit more friction there.
That still sounds like a work-around for smt that could be fixed pushing patch jruby-rack/warbler versions.
On the jruby-rack (1.2.x) gem side by requiring to have the required_ruby_version < 3.2 then at runtime Bundler/RubyGems should validate.
potentially same could be done on the Warbler side, althoughlikely less reasonable - what kind of version you're using to pack and at actual runtime might be different (as you mentioned on the Warbler ticket that jruby-jars requirement should get an upper limit).
Alternatively to this what we could possibly do on 1.2.x is disable the jruby-rack config capture (on init failure) for JRuby 10.
Not sure I fully follow but sounds like things are getting complicated... 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but we can't change the requirements for already pushed versions, so in practice that still that kinda leaves people ending up accidentally using incompatible combinations (ignoring yanking gems).
But yeah, ok. I'll leave this and refocus on Rack 3 and 1.3.
Would you be able to help push a release of this branch as it currently stands in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backports #273, #299, #314 to the
1.2.xbranch by housing the JRuby-10-removedCompatVersionclass (used byRackConfigAPI), and avoiding needing to change the jruby-rack API.Generally 1.2.x was already working with JRuby 10 if the Config was not used in certain ways causing the
CompatVersionclass to be loaded; and if there were not failures during initialization that cause reflection on theRackConfigto be able to dump to logging.This will make it easier for folks to move to JRuby 10 within Warbler without having to move to jruby-rack
1.3.Since
CompatVersionhas been a no-op on JRuby itself for a very long time, removing the actual implementation does no harm.jruby.compat.versionjruby-rack init properly will just be ignored, and the default values from earlier returned.