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

RFC: Do not add CLASSPATH to the module path #5728

Merged
merged 1 commit into from May 14, 2019

Conversation

@kschoelhorn
Copy link
Contributor

commented May 10, 2019

960efcb and 7c4bf9c changed the starter so that the $CLASSPATH is added to the module path when running on java9+.

Treating the CLASSPATH as modules will create automatic modules for all entries that are not actually modules. This can lead to several problems, e.g. the automatic module name is determined by the name of the jar or directory, which fails when one component is not a valid java identifier. Another issue is that several automatic modules might export the same package, which is not allowed and leads to an error.

While the first problems is relatively easy to fix, the second one requires large changes and might not even possible if you are using third-party libraries (e.g. we are using UNO, which has problems with this).

To fix these issues, we can use the module path just for jruby libraries and pass the CLASSPATH using -classpath, as both can be combined.

Note that I haven't done extensive testing, because I first wanted to hear what you think about this approach.

Unresolved questions:

  • Are there any unintended side effects?
  • Should $CP also go into the module path?
  • What about the other starters?
RFC: Do not add CLASSPATH to the module path
Treating the CLASSPATH as modules will create automatic modules for all
entries that are not actually modules. This can lead to several problems,
e.g. the automatic module name is determined by the name of the jar or
directory, which fails when one component is not a valid java identifier.
Another issue is that several automatic modules might export the same
package, which is not allowed and leads to an error.

To fix these issues, we can use the module path just for jruby libraries
and pass the CLASSPATH using -classpath, as both can be combined.

TODO:
 - Should $CP also go into the module path?
 - What about the other starters?
@headius

This comment has been minimized.

Copy link
Member

commented May 14, 2019

@kschoelhorn Good catch...I did not think about other libraries being on classpath since most Rubyists will require jars at runtime (which then load through URLClassLoader).

I will review. We will need an equivalent change for https://github.com/jruby/jruby-launcher.

@iloveeclipse

This comment has been minimized.

Copy link

commented May 14, 2019

Regarding open questions: please note, classpath and modulepath are for different purpose and entries from one should not go to the another one. So as a user one should be able to specify both at same time without a fear that jruby does a happy mix from them.

@headius

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Regarding your additional questions:

  • We should not be depending on any CLASSPATH jars loading as modules, since that implies they should be loading with -classpath. In other words I think all effects of your change are intentional and appropriate.
  • No, CP should be treated the same as CLASSPATH, as it is in your patch.
  • Only thing is patching jruby-launcher. Shouldn't be hard...want to do a second PR for that?

@headius headius added this to the JRuby 9.2.8.0 milestone May 14, 2019

@headius headius merged commit cd33502 into jruby:master May 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
jruby.jruby Build #20190510.1 succeeded
Details
@klemens

This comment has been minimized.

Copy link

commented May 14, 2019

Thanks for merging this so fast! I will try to create a PR for jruby-launcher tomorrow.

(This is my private account)

@kschoelhorn kschoelhorn deleted the kschoelhorn:do_not_treat_classpath_as_modules branch May 15, 2019

kschoelhorn added a commit to kschoelhorn/jruby-launcher that referenced this pull request May 15, 2019

Do not add CLASSPATH to the module path
Treating the CLASSPATH as modules will create automatic modules for all
entries that are not actually modules. This can lead to several problems,
e.g. the automatic module name is determined by the name of the jar or
directory, which fails when one component is not a valid java identifier.
Another issue is that several automatic modules might export the same
package, which is not allowed and leads to an error.

To fix these issues, we use the module path just for jruby libraries and
pass the CLASSPATH using -classpath, as both can be combined.

This is identical to the fix for jruby.bash in jruby/jruby (20e4278) [1].

[1]: jruby/jruby#5728
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.