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

Uniform jruby home #2031

Closed
wants to merge 2 commits into from
Closed

Uniform jruby home #2031

wants to merge 2 commits into from

Conversation

mkristian
Copy link
Member

the jruby home detection is an interesting thing - but there is well defined way of just setting it: just use the one from the classloader.

this patch would make much more sense if the "detection" of the parent classloader of the runtime.jrubyClassLoader works reliable: #2023
and if the uri:classloader: protocol will use this parent classloader for the lookup: #2027

even without the other two PR it reduces the setting of jruby.home to two use-case:

  • jruby home is on the file system and bin/jruby sets it via the property
  • jruby home lives inside a jar file: uri:classloader:/META-INF/jruby.home

the first case is already well tested. the second is not tested at all in the sense that jruby-complete currently hardly runs any tests. so I added the test/jruby.index using the jruby-complete to run them:

mvn -Pjars-test

and allow failures there for travis since it does not pass. well even without the jruby patches applied it does not pass.

well, the whole things fails when tools like "jruby-rack" to detect jruby home itself and overwrite the setting coming from jruby itself.

@enebo @ratnikov

@mkristian
Copy link
Member Author

#2034 (comment) demonstrates that some features of jruby just disappears when the jruby.home is set to classpath:/META-INF/jruby.home and it is just not feasible to test against all kind of possible jruby.home but testing against TWO jruby.home should be there:

  • jruby.home on filesystem
  • jruby.home inside a jar file
    anything is "not tested" and warned when used.

@kares
Copy link
Member

kares commented Oct 14, 2014

hmm, but tools like jruby-rack did work with the various hacks from time to time (at least to some extent)
... could we somehow not freak out everyone by default with the warning ?

on jruby-rack if there's an error of some kind the trace includes "jruby.home" thus we can advise from there

@mkristian
Copy link
Member Author

On Tue, Oct 14, 2014 at 2:47 PM, Karol Bucek notifications@github.com
wrote:

... could we somehow not freak out everyone by default with the warning ?

I thought jruby-rack does change the $LOAD_PATH directly and not via
runtime.instance_config - but I was thinking about jruby-rack and I agree
that it should not produce warnings which confuses.

@headius
Copy link
Member

headius commented Nov 2, 2014

Does this only affect the complete jar? If so, I would be concerned about environments that explicitly set jruby home to something on the filesystem rather than using the one inside the jar. However I'm not sure if there are such environments.

If you are able to make @kares and @bbrowning happy with this change I think we'd be ok. @enebo may have other concerns.

@mkristian
Copy link
Member Author

this affects jruby in general, i.e. bin/jruby will set the -Djruby.home=... which points to the filesystem. this works as ever and this jruby.home from filesystem can be used with jruby-complete.jar or with the jars from jruby-jars.gem or with the modular jruby maven artifacts. when you use the jruby.home from within a jar then it uses should use something which just works for all environments, j2ee, osgi and whatever and using the same load-service semantic.

right now if I embed jruby-openssl-0.9.6.dev into a jar (since I needs some fancy cipher) this only works if jruby.home uses jar:file:jruby-stdlib.jar!/META-INF/jruby.home. with jruby.home pointing to classpath:/META-INF/jruby.home the default gems are no gems and then rubygems will load the openssl from jruby-stdlib.jar instead of the embedded jruby-openssl-0.9.6.dev gem. so my jar is restricted to environments where the current jruby.home detection finds jars !! examples where classpath:/... will not work are all those test cases with mvn -Pj2ee and mvn -Posgi - none of them will pass with jruby.home set to classpath:/...

@kares my finding in jruby-rack regarding jruby-home is which should not trigger the warning: https://github.com/jruby/jruby-rack/blob/master/src/main/ruby/jruby/rack/booter.rb#L129
if I missed something and jruby-rack indeed does trigger the warning then I remove the warning part.

this PR also adds some tests using this setup which does have failures even if the case with jruby.home from filesystem is green. having only two cases: filesystem + uri:classloader: makes feasible to run tests for both cases.

@enebo
Copy link
Member

enebo commented Nov 3, 2014

Even if this does only affect jruby-complete I am concerned it will affect someone who is using it. There is way too much qualification in the last comment for me to even understand what may or may not break. I guess if @kares can support this with jruby-rack and @bbrowning will not see ill effects then perhaps we do this on master? I feel like our breakage from jar-dependencis was enough to cool my desire to change behavior on 1.7.

@bbrowning
Copy link
Contributor

TorqueBox 3 only supports an exploded jruby home on the filesystem set by either $JRUBY_HOME or -Djruby.home. So I don't think this change will negatively impact our users.

At either rate, we expect to have TorqueBox 4 out by or before JRuby 9k so any change that just happens on master shouldn't be a problem for us.

@enebo
Copy link
Member

enebo commented Nov 3, 2014

My last comment was directed towards me erroneously thinking this was intended for jruby-1_7. On master my only reservations would be any comments @kares has on the subject. Looks like @bbrowning is not a consumer.

@kares
Copy link
Member

kares commented Nov 6, 2014

as @mkristian noted there's the embedded scenario with jruby-rack linked previously ... this has been way back before me being envolved with the project. I've seen JRuby.runtime.instance_config.jruby_home == tmpdir only on WebSphere but that case is already better handled in 1.7 (or at least was since 1.7.5 I guess).

assuming the home detection works better it shall never reach the point where we end up manually setting $LOAD_PATH entries such as classpath:/META-INF/jruby.home/lib/ruby/shared ... thus it seems there's no real concerns on my end, and since it's 9k than I'm even fine with leaving the error warnings as is.

@mkristian mkristian force-pushed the uniform-jruby-home branch 2 times, most recently from 86aab7c to af5f0a7 Compare November 7, 2014 12:48
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.

None yet

5 participants