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

Add upward compatibility to Java 9 Modularity #323

Closed
wants to merge 1 commit into from

Conversation

R077A6r1an
Copy link
Contributor

I've noticed that Jython uses 1.8 source and target compatibility, but when Jython is used as a Maven/Gradle dependency with Java >= 9 it becomes an unnamed module. Adding the Automatic-Module-Name attribute in the jar's manifest resolves this issue, so named modules can require on a save way on it, without need to move Jython to Java 9. The Java 8 build will not be affected by this change, and for java module users it increase stability instead of the module name derived from the jar file name.

Now have a look if the module name org.python.jython fits, or if you prefer another name instead. In case of keep compatibility to the module name derived from the jar file name, the module name should be changed to jython, as this is the name that Java derives from the jar file published on Maven Central.

@Stewori
Copy link
Contributor

Stewori commented Apr 2, 2024

In fact, the jar-derived name might likely be jython.slim, given that that's the preferred artifact to be used in maven dependency management. (jython.standalone and even worse jython.installer might pop up as well).
That said, we should consider whether it is desirable to have the variants of Jython distributions (normal, standalone, installer and slim) called the same on module level. I agree that the module name org.python.jython is preferrable over the status quo. OTH, wouldn't that be a backwards-breaking change? Is there any example of a project using "requires jython" in its module.java?

I think the proper solution (perhaps for Jython 3) would be a proper module structure of several modules, e.g.

  • jython.core: the bare minimum Jython needs to run (batteries not included)
  • jython.corelib (requires jython.core) some (i.e. the dependency-lightweight) batteries but not all - most common libraries that are workable without requiring the majority of the vast Jython dependency list
  • jython.lib the remaining batteries; equivalent to current jython-slim (because dependencies are not shaded)
  • jython.standalone requires the above and includes shaded dependencies, perhaps shaded dependencies could go into a separate module jython.shaded. Or, perhaps preferably, we discard the whole shading idea as we have entered the gradle age.

@R077A6r1an
Copy link
Contributor Author

Yeah, that's a good idea. But for the current Jython 2, we can set at least explicit the from jar name derived module name. Means for example setting for Jython Slim the jython.slim name, for Jython Standalone jython.standalone and so on. That would now break nothing, and is for Java 9 modularity safe.

@jeff5
Copy link
Member

jeff5 commented Apr 6, 2024

I authorised the regression tests but they don't involve the affected parts of the build. One would need to go through the build for release to test it.

@Stewori
Copy link
Contributor

Stewori commented Apr 6, 2024

I'm still puzzled whether the different Jython distribution flavors (normal, slim, standalone, installer) should or shouldn't have the same module name. This PR suggests they should. That changes status quo since until now with jar-derived module names they did not. Let's say an external module has a clause requires org.python.jython. That would be satisfied by any of the distributions. Maybe that is actually what we want, I'm not sure about the details in how they differ right now.

@Stewori
Copy link
Contributor

Stewori commented Apr 6, 2024

One would need to go through the build for release to test it.

No worries, I am rather confident this change wouldn't break anything within Jython. It can only possibly affect external usage of Jython. And that's not Jython's responsibility to test.

@jeff5
Copy link
Member

jeff5 commented Apr 6, 2024

Bottom line in https://stackoverflow.com/questions/43192741/how-should-i-name-my-java-9-module appears to be to make it the same as a leading API package from the JAR, and that should be based on the reverse domain name.

When I specify a dependency in Gradle it is group:name:version. Our group is org.python, but the name is only the base-name of the JAR. I do not think the Java module name has to co-ordinate with the JAR name, except to aid humans.

Edit: It appears there can only be one module in each JAR, but no obstacle in the automatic module scheme to having the same packages in differently named modules.

You ask a sensible question about the same Java module in multiple JARs (as this PR proposes). I think there would only be a problem if two JARs with the same module were dependencies in the same project. I don't know for sure.

I know you're thinking ahead to a Jython 3, but I think that question would be relevant now, should anyone cite both jython-slim and jython-standalone, say.

@R077A6r1an
Copy link
Contributor Author

Ok, I've made a little test, building with ant jython-slim and jython-standalone, jython and python-installer. The issue I've found is a module resolving error by having both on module path:

Error occurred during initialization of boot layer
java.lang.module.FindException: Two versions of module org.python.jython found in ../lib (jython-dev.jar and jython-standalone.jar)

Cmdline was:

java --upgrade-module-path ../lib -jar module-test.jar

In my module path, I had the following jar's:

jython-dev.jar
jython-installer.jar
jython-standalone.jar
jython.jar

Means that for prevent this error, the automatic module name must be set for each jar different. Else if depending for example on jython-slim and jython-standalone, all get broken.

@R077A6r1an
Copy link
Contributor Author

Now, my suggestion would be, using for example as base module name org.jython, and then append for the different jars then the distribution name. Means setting the module name for jython-slim to org.jython.slim, for jython normal org.jython, for jython-installer org.jython.installer, and for jython-standalone org.jython.standalone.

@jeff5
Copy link
Member

jeff5 commented Apr 7, 2024

I'm still educating myself about modules (not my first attempt). However ...

I would have predicted the module clash from adding two of our JARs to the module path. I can't think of any reason to do that deliberately. I suppose it might occur accidentally in a complex build where two dependencies used Jython different ways.

As I understand it, the dependency graph is between modules, and a JAR is only a way of presenting a module. It would be bad (I think) if the same module had different content depending on which JARs were present, so I think we need different names for the modules, even though they contain many of the same packages.

I much prefer a module name (or names) starting org.python, and then refining. The PSF also own the jython.org domain name, but org.python is our publishing group on Sonatype and something of a sacred trust. I was content with org.python.jython although org.python.jython2 is possible, to underline the contingent nature of the choice.

The installer doesn't have to be a module, I suggest: it is a packaged app.

@Stewori
Copy link
Contributor

Stewori commented Apr 7, 2024

It would be bad (I think) if the same module had different content depending on which JARs were present, so I think we need different names for the modules, even though they contain many of the same packages.

I think I have seen errors when multiple modules are on the module path that contain colliding package names. So, unless we would change package names as well (just kidding), it might anyway not be possible to have e.g. jython.jar and jython-standalone.jar on the module path concurrrently. That is, even if module names differ. However, that was only a compile-time observation and perhaps I overlooked some workaround or magic javac parameters.

I see that it is impossible to fulfill all requirements - one module per jar, unique package names and having the concurring Jython distribution flavors. It's a shame that jigsaw does not offer a proper solution for this. (Perhaps one would model module similarities and differences as provided and used services.)

My suggestion is the following: we should check if colliding package names are feasible at all. If yes, I'm fine with having different module names. Otherwise I would suggest to have a common module name because multiple ones would not have a benefit and would just pointlessly break the promise that package names are unique across modules. Users can still choose/specify their exact Jython dependency on pom-level.

@Stewori
Copy link
Contributor

Stewori commented Apr 7, 2024

Perhaps it would be good to introduce a new additional Jython artifact jython-lib that contains the Lib folder (and only that folder) like it can currently be found in jython-standalone.jar. That would allow users to augment a dependency on jython-slim by jython-lib. That combination can then replace a dependency on jython-standalone in a more modular way. Then we could discourage/"deprecate" the idea of using jython-standalone as a maven dependency at all.
Introducing jython-lib should be feasible already for the Jython 2.7 branch as adding a module would not break anything.

Note that some of Jython's maven dependencies, e.g. netty, would actually apply to jython-lib rather than to jython-slim then. See https://apidia.net/java/Jython, where I listed dependencies that are referenced from Python code separately.

@R077A6r1an
Copy link
Contributor Author

Now regarding to this, having a look at https://stackoverflow.com/questions/53245628/jdk9-automatic-modules-and-split-packages-dependencies, split packages are neither allowed in automatic modules. But looking at the solution, at least in maven it would give two possibilities to solve for example in a complex build an dependency on say jython-slim and jython-standalone:

  1. Exclude one of the jython dependency and using that one with all required feature.

  2. In case that both are required, it would be able to merge both jar files with the maven assembly plugin with the right know-how in pom configuration.

So in this case, I think as like @Stewori said, it might be better to have a common module name. Otherwise, the idea to modularize the Lib folder with the additional artifact jython-lib would be in this case a better alternative, not only relating to this PR. Only thing on it is that it requires more work.

@Stewori
Copy link
Contributor

Stewori commented Apr 8, 2024

If split packages are tolerated in explicit, i.e. non-automatic modules, perhaps we could add a proper module-info.java and be done with it.
I'm suggesting to add jython-lib in any case, independently from this issue. I think users of jython-slim currently lack a good way to include the Jython Lib folder if needed. There is no jython-slim equivalent of jython-standalone, i.e. that has the Lib folder but refrains from dependency shading.

@jeff5
Copy link
Member

jeff5 commented Apr 8, 2024

I've built JARs from this PR branch and tried them out as I would a release,. They work as expected, i.e. they work with the same failures as when I tested 2.7.4b1.

I didn't find any problem mixing the JARs with the same module name. It ran ok, and the JVM seems to accept the first definition it finds on the module path:

PS 323> java --module-path "inst\jython.jar;kit\jython-standalone.jar" --list-modules
...
org.python.jython file:.../gh-iss/323/inst/jython.jar automatic

PS 323> java --module-path "kit\jython-standalone.jar;inst\jython.jar" --list-modules
...
org.python.jython file:.../gh-iss/323/kit/jython-standalone.jar automatic

There is no reason to do that if you control everything, but it is imaginable that transitive dependencies might bring that about.

I think we know that if different modules try to supply the same package, we will have a problem. So if we give each jar its own module name, then we also have to break up the contents into non-overlapping collections as @Stewori suggests. But then a project previously satisfied by jython-slim or jython-standalone has to assemble two or more JARs to get what it had before.

I wouldn't want to inflict that change on users of Jython 2. (Jython 3, of course, is allowed to be quite different.) So for Jython 2 I think what we have here is fine.

To get into 2.7.4, we would have to have a second beta as I tagged b1 last week.

@Stewori
Copy link
Contributor

Stewori commented Apr 8, 2024

But then a project previously satisfied by jython-slim or jython-standalone has to assemble two or more JARs to get what it had before.

Adding jython-lib would not change existing jython-slim or jython-standalone. Repeating myself: it would just allow to have the Lib folder without having to use shaded dependencies. I'm not saying this has to be part of 2.7.4.

@jeff5
Copy link
Member

jeff5 commented Apr 9, 2024

Adding jython-lib would not change existing jython-slim or jython-standalone. Repeating myself: it would just allow to have the Lib folder without having to use shaded dependencies.

I don't understand this idea yet. (Fortunately, that is not in the way of this PR.) jython-slim (i.e. the JAR without shaded dependencies) contains Lib already.

I can use Gradle to build an application with org.python.util.jython as the main class. It gives me a script in ./bin that wraps essentially java -classpath ./lib/* org.python.util.jython, with all the JARs it thinks I need in ./lib.

PS 323a> jar --list --file .\lib\jython-slim-2.7.4a1-SNAPSHOT.jar | Select-String -Pattern "Lib/csv"

Lib/csv$py.class
Lib/csv.py

PS 323a> .\bin\demo-jython-slim-gradle
Jython 2.7.4a1-SNAPSHOT (heads/pr-323-modules:7cf85f648, Apr 8 2024, 16:33:37)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java11.0.22
Type "help", "copyright", "credits" or "license" for more information.
>>> import csv
>>> csv.__file__
'C:\\Users\\Jeff\\Documents\\Jython\\gh-iss\\323a\\lib\\jython-slim-2.7.4a1-SNAPSHOT.jar\\Lib\\csv$py.class'

@Stewori
Copy link
Contributor

Stewori commented Apr 9, 2024

Okay, sorry Jeff. I must have checked the wrong file or somehow overlooked it. I was in the believe jython-slim was without Lib, i.e. an unshaded version of plain jython rather than jython-standalone. In that case, moving to modularity is not so simple, it would require also something like jython-core being what I thought jython-slim would already be. If we wanted to move to modularity, creating new artifacts core and lib would be the way rather than changing existing ones. But that's not so compelling any more.

@jeff5
Copy link
Member

jeff5 commented Apr 9, 2024

Confusion resolved, thanks. I'm not at all sure it is done right (including Lib, I mean) but it works, so I'm leaving it alone! Giving a bit of thought this afternoon to the right modules to structure API for 3 while the tutorial is fresh in my mind. If you have thoughts @Stewori, e-mail directly or via jython-dev.

@jeff5
Copy link
Member

jeff5 commented Apr 9, 2024

@R077A6r1an : thanks for working on this and encouraging us to understand modules better. We'd like to include this in a 2.7.4b2 release in a few weeks. (You just missed the b1.) For this I need to give you a few tasks.

  • Go here and sign the Python contributor agreement. (We are PSF owned, but for historical reasons, not as far as GitHub automation is concerned.)
  • Come back here and tell us you've signed it.
  • Add yourself to the end of the people in ACKNOWLEDGEMENTS. A real name is desirable (and is in your commit anyway). The date is the date of the previous step.
  • Push that change to your GitHub repo.

After that, I'll:

  • Merge the current tip onto this PR,
  • Test again, and
  • Squash merge onto our master branch.

Point of technique: those last steps I do will make a mess of your fork because your master and ours will be out of sync (irrecoverably?). It is better to create a branch on your repo and PR from there to our master so your master can continue to follow ours. It's the same process as here. No need to start over this time.

@R077A6r1an
Copy link
Contributor Author

Okay, I will prepare all. Of what I know, my master branch would not be irreparable broken, but to be sure, I will create a new branch and PR from there. I will fix my current master later.

@jeff5
Copy link
Member

jeff5 commented Apr 9, 2024

... to be sure, I will create a new branch and PR from there. I will fix my current master later.

Happier if you leave it as it is for this PR (as long as I can push to it).

Once we've merged to our master you could try renaming your master to a chosen name and sync again. Or just delete the repo and clone again when you have another urge to contribute or just want to track us.

@R077A6r1an
Copy link
Contributor Author

I've made a new PR from my updated branch at #325.

@jeff5
Copy link
Member

jeff5 commented Apr 14, 2024

Ok, I'll pull that one and test.

@jeff5 jeff5 closed this Apr 14, 2024
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

3 participants