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

I would like to strictly name the module #451

Closed
GedMarc opened this issue Nov 26, 2018 · 50 comments
Closed

I would like to strictly name the module #451

GedMarc opened this issue Nov 26, 2018 · 50 comments

Comments

@GedMarc
Copy link

GedMarc commented Nov 26, 2018

Hi There,

I would like to update the pom and build to generate the module-info class file, and strictly name this module.

Currently as an automatic module, you cannot perform a lot of JPMS functions, such as requires transitive, or evade the maven central deploy warning for unnamed modules.

This is purely the raw code, could I get a zip of the build and send back a finished zip for perusal?

@johnjaylward
Copy link
Contributor

https://github.com/BGehrels/JSON-java holds the packaging for maven releases. Please peruse there and offer any merge requests at that location.

@johnjaylward
Copy link
Contributor

Also, strictly speaking, this project must support back to Java6 still until @stleary mentions otherwise. So any module-info changes must not cause an issue with java6 runtimes.

@GedMarc
Copy link
Author

GedMarc commented Nov 26, 2018

It will still support JDK 6 so long as

  • You build under JDK 8 with source and target versions set to 1.6

This will produce the 1.6 output with the module-info correctly placed,

Thanks for considering it!, Hope you pull it in!

@GedMarc
Copy link
Author

GedMarc commented Nov 26, 2018

BGehrels#3

@GedMarc
Copy link
Author

GedMarc commented Nov 26, 2018

You can view the build log and the output here (Guest user is enabled)

https://jwebmp.com/teamcity/viewLog.html?buildId=201982&tab=artifacts&buildTypeId=ThirdPartyBuild_BuildOrgJson

Thanks!

@stleary
Copy link
Owner

stleary commented Nov 26, 2018

Will existing developers have to change their build process to use the new build?

@GedMarc
Copy link
Author

GedMarc commented Nov 26, 2018

Not at all, this simply adds the module-info class file into the final packaged jar

I wanted to double check the class file version, it is indeed JDK 1.6 version 50

image

@xzel23
Copy link

xzel23 commented Nov 27, 2018

Hi, I have created a fork that adds modules, but I build it using gradle. I could contribute the module-info.java files. I would be happy to help to get module-info into trunk.

[UPDATE] Seems I did not yet push my changes to my fork. I will look into this.

@GedMarc
Copy link
Author

GedMarc commented Nov 27, 2018

I see you've moved the classes to a higher version of Java,

Thanks!
This strictly naming is definitely needed so you don't have to specify requires org.json in the consuming module but only the dependency, and to keep org.json out of the java.base root module (automatic named module placement)

To be honest, I do prefer my method, it keeps the libraries perfectly in tact without any code changes of any kind, or bumping of JDK's.
I would also like to still be able to use the official build repository, rather than otherwise.

If worst comes to worst, on all these projects that require the push forward into JPMS, I'll be placing them onto the com.jwebmp maven group, and utilizing all the updates from there :)

P.S. Gradle is the android native build tool. You shouldn't actually be using it for Java. While Maven Gradle Ivy and Grunt can all build Java, and they can all build android native, each was designed and built specifically with a purpose in mind. You should be using the build tool built for Java to enable advanced functions that are required post JDK 7 like annotation processor paths, annotation arguments, processing updates and manipulation, pretty much anything in the "advanced" category of Java building, and etc
So I wouldn't use this fork either based purely on the build tool choice. But that part is my opinion.

@xzel23
Copy link

xzel23 commented Nov 28, 2018

@GedMarc: Sorry, but I think I only understand half of what you try to say.

I think adding the module-info.class files will probably break things for some developpers. That's also the reason why I did not try to contribute back. I have to research because it's been some time, but I remember these issues:

While module-info.class files should be ignored when used with older Java versions, at least some tools choked on this, IIRC these were among others

  • SpotBugs
  • JUnit 4
  • Eclipse
  • the Android toolchain

Another solution would be Multi-release Jar files, but with more or less the same issues.

If you don't want to abandon Java 6 compatibility (a version that has been unsupported by Oracle for several years now), you should probably not force your users to use the newest versions of these softwares (I don't even know if all problems have been solved in the meantime).

So, at least some developpers will have to change their build process or dependencies. And I also doubt all those new versions will still be compatible with Java 6.

IMHO it would be great to have a truly modularized version in trunk, but I think you have to make a compromise. You cannot have it and still be compatible with JDK 6 and the tools used to maintain Java 6 based software.

Concerning your comment about Gradle: Maven is not the build tool for Java. If there's a tool that deserves that name, it should still be Ant. Just because Gradle has been adopted as Android's standard buid tool doesn't mean it's not suitable for Java. Following your rationale, you shouldn't even use Java for anything other than Android development, right? But I think this really doesn't belong here, and if you want to discuss build tools further, you can rather contact me directly. Gradle was simply the tool I chose because I could reach my goal faster.

@johnjaylward
Copy link
Contributor

@xzel23 The PR that @GedMarc provided only adds the module.info file in the JAR during the packaging process. It does not add it to this project. See his PR at #451 (comment)

@johnjaylward
Copy link
Contributor

Also, as a side note, please leave build tool discussions out of this project. They are not relevant.

@xzel23
Copy link

xzel23 commented Nov 29, 2018

@johnjaylward But there's a packaged Jar at mavencentral, linked from the main github page. Won't it add the module-info once this is merged? I think it should, but I also think this could break builds.

@xzel23
Copy link

xzel23 commented Nov 29, 2018

@GedMarc Maybe you could add to the PR description that with automatic modules, you cannot use JDK 9+ jlink tool to create standalone applications. That was the main reason to have a modularized version for me.

@GedMarc
Copy link
Author

GedMarc commented Nov 29, 2018

@xzel23 The idea behind this is that the original packaging/class files are not manipulated at all, and the way that the JVM works, the module-info class file will never be hit in any JVM less than 9 (the hyphen makes the class name invalid) so no incompatibility is ever brought in.

Moditect itself actually constructs the class file byte-for-byte and the file never goes through any "compilation", so when the class gets added into the final package, it can contain invalid references that the compiler would normally throw out.
THIS IS IMPORTANT! so important in fact, that in some projects that are JRE11, I still use moditect - as the build process may JarJar a dependency that needs to be exported, but would never ever compile as you can't compile empty packages ;)
(Take a look at the guice implementation that bundles asm and cglib, but cglib has to be exported. Even in JRE11, this compile would never be possible without Moditect)

This means that all builds using Moditect are unaffected, and are guaranteed to still execute properly, even with the module-info.class file in the JAR regardless of the project version (down to 1.5, 1.4 no compatible but that's a JRE 8 thing)

I did not know about JLink restrictions! We always learning lol, I'll need to run some tests, but from what I understand, if you open your base module, JLink will still work with filename/automaticmodule named modules, I haven't tried/tested this, but logically it should work.

I may actually use that as a further base for Hibernate and Guice implementations after I've verified that it is indeed the case. You mentioned JUnit 4, but you can't use that for JPMS, please use Jupiter (5) for proper TCK in JPMS, and don't use any junit 4 runners, keep it strictly jupiter. I have had great success going this route, and awful experiences in junit 4 :)

The goal as usual in Java, is not require any changes or dependencies code wise for JVM migration, and this solution does solve that properly.

@GedMarc
Copy link
Author

GedMarc commented Nov 29, 2018

I went multi-release Jars for my own personals, but that's because I actually want JRE11 byte-code, but it isn't necessary at all, you can keep your packages in tact with no changes other than plumping a module-info into the package post-build ;)

Quote : IMHO it would be great to have a truly modularized version in trunk, but I think you have to make a compromise. You cannot have it and still be compatible with JDK 6 and the tools used to maintain Java 6 based software.

^^ This is completely incorrect and none of the above is true in any regard.

@xzel23
Copy link

xzel23 commented Nov 29, 2018

^^ This is completely incorrect and none of the above is true in any regard.

Can you elaborate? Have you tested? All tools? Because I definitely had problems as I have written above. I agree that there should not be any problems, but for example some tools simply cannot handle the non-standard file name containing a dash. That bug has been fixed in the meantime, but the fixed version (3.1.x) is incompatible with Java 6. And I had problems with other tools as well. Maybe there are workarounds, but how can you say it's not true?

In my Jar, I only compiled the module-info with JDK 9, the remainder with JDK 8 compatibility (I know, yours is JDK 6). So the resulting Jar should be more or less the same as sneaking in a JDK-9 compiled module-info after the build.

I think this should be looked into before releasing a modularized Jar. Can you provide me with a binary Jar so that I can try it against the tools that I had problems with?

@johnjaylward
Copy link
Contributor

Are you able to just stick the module-info file into a version folder in the jar? Does it have to be root-level?

@johnjaylward
Copy link
Contributor

answered my own questions: https://openjdk.java.net/jeps/238#Modular-multi-release-JAR-files

looks like module-info need to exist both at the top level and in any version folders.

Would be nice if you can test it out with it missing at the root level and see if it works in a couple JVMs

@xzel23
Copy link

xzel23 commented Nov 29, 2018

@johnjaylward No, it doesn't have to exist at the top level. from your link:

A multi-release modular need not have a module descriptor at the located root.

I have also tested it, and it worked for me. However, IIRC there were problems with other tools. Stephen Colebourne posted a nice writeup on problems with JPMS on his blog some time ago.

I will try to compile a list with the relevant PRs here:

I don't do Android development at the moment, so I cannot check on that platform. But for Plain Java, I could re-check if it works with the "tweaked" Jars.

@johnjaylward
Copy link
Contributor

johnjaylward commented Nov 29, 2018

Ah, I've always like Stephens work. Joda was a go-to for me for years. And I agree with that blog 100%. My company is on Java8 and has almost no plans to move to Java11 because so far in testing with java 9&10 it has been a major undertaking with lots of incompatibilities. It will probably be another year or 2 before we fully make the switch.

As for the module-info at root, I must have been reading too quick, I could have sworn it said the exact opposite. Thanks for testing it out.

@GedMarc
Copy link
Author

GedMarc commented Nov 29, 2018

@xzel23 It took me a good year before I actually got happy with JPMS, and it still really irritates me that things like the Jlink and those little thing I still don't know or haven't come across..
But I can safely say i'm a big fan of it now, as irritating as it was to unlearn and relearn...
And no, I didn't use the versions mechanism for my multi-jar, there was actually so many problems with that especially related to class scanning and which class to pick when (Take a look at ClassGraph for JPMS class/path scanning). I physically switch the group to a suffix of "jre11".

So i'm using Jupiter 5.2, Selenium 4.1, Google Guice 4.2.3-SNAPSHOT, BTM 2.2, Hibernate 5.3, Hazelcast 3.10.4 and honestly still trying to get a proper JPMS named JMS up and running, RabbitMQ looks to be the most promising.
I've gone through grunt, gradle, ivy, JUnit 4, TestNG, EclipseLink, validations, activations (good lord removing all @nullables was not fun!!!!) and through so many across JRE 9 and JRE 10 builds it's almost sickening. JRE 10 transactions was so half baked it was near impossible to get running, jaxb also needed stuff. Ended up having to use @ArgumentFiles for JRE10.. xD

Running the base applications is simple, the automatic and unnamed modules are currently the biggest headache, but as an excersize the clean out the JDK library pool, it is definitely very effective. Downside mostly is the aged/dropped/finalized libraries (aopalliance, javax.inject, etc) have no organizers contactable anymore, and trying to update the maven repository is going to difficult.
What's also interesting here is that java.base module is also actually defined as your root module, which is why all unnamed and automatic modules have to be explicitly defined. Being placed in the base module puts of those modules in classpath mode, setting your app to open module actually opens the entire jdk library and there's no point in even going jpms from a low level point of view.

I think the biggest thing that I struggled with, which seems to be for you as well, is that JPMS is not backwards compatible, it just isn't, that's why there is a specific "classpath mode" to run the older programs. Trying to run builds with advanced functions becomes extremely difficult (try package lombok and the static metamodel together), and because everything is new, all the cool stuff you actually need the newer tools to use it (properly).
Automatic Module Naming turned out it was simply a way of "reserving" the module name for when it's fully implemented, but it is in no way actually JPMS (which I found out the hard way through deep module inspections)
I did a checkup and it looks like Gradle 5.0 will support java annotations properly, but that's still quite a way away but it's also correct because you shouldn't, e.g, be able to specify annotation handlers by simply including a dependency (its a nice luxury but shouldn't have ever been possible)

  • Module-Info breaking on android - JDK-ME for JPMS hasn't been released. so this is obvious.
  • SpotBugs - google uses the truth library for the same function, but that also is canned ;) It is not backwards compatible. Perhaps, look closer at the more latest tech stuff? I use SonarQube e.g., I know Jenkins also works with JPMS? If that's a possibility, things like Atlassian stuffs will not work yet, they are very far behind but there is a rather large client base to make sure still runs... ;)

Good lord that was long, I am so sorry for destroying the thread xD

@GedMarc
Copy link
Author

GedMarc commented Nov 29, 2018

@GedMarc
Copy link
Author

GedMarc commented Dec 1, 2018

whoop whoop, JDK 6 finally EOL!!!!

JDK 6 EOL = December 2018
JDK 7 EOL = July 2019
JDK 8 EOL = March 2022
JDK 9 EOL = March 2018
JDK 10 EOL = September 2018
JDK 11 EOL = September 2026

@GedMarc
Copy link
Author

GedMarc commented Dec 1, 2018

@johnjaylward 9 and 10 were JPMS Alpha and Beta, and yes I agree were absolutely horrid ;)

It looks like 6 and 7 are completely out next year, I'm not sure if you have 2 years to make the switch. JRE 8 looks to have an extended life span.

@stleary
Copy link
Owner

stleary commented Dec 1, 2018

Updated:
Given that there are no plans to stop supporting Java 6, and the current packaging will not be changed, is there any consensus about whether to proceed with offering a separate package that provides additional support for things like a module info class file, strict module naming, JPMS functions, etc?
Or is is better to not make any changes at this time?

@xzel23
Copy link

xzel23 commented Dec 2, 2018

Updated:
@stleary I think we should proceed, and offer a separate package.

@GedMarc
Copy link
Author

GedMarc commented Dec 2, 2018

I don't think we should drop jdk 1.6 support until the jvm no longer supports running it,
I say yes please pull it in, i have a week and then it's either this library or the one i'm dev'ing on ;)

@stleary
Copy link
Owner

stleary commented Dec 2, 2018

Sorry, was not clear. Question has been updated, feel free to edit your answers or provide new ones.

@GedMarc
Copy link
Author

GedMarc commented Dec 2, 2018

@stleary
The changes don't affect JDK 6 and no one will feel it except for those using jpms, i don't think it's needed to offer a separate package, simply a new release with the changes?

@johnjaylward
Copy link
Contributor

@stleary the PR earlier in this thread has links to a JAR file which was built using the changes @GedMarc made to allow for the module-info within a Java6 JAR. I think we should get a few people to test it on java6-8 JVMs and then make a decision. If the changes @GedMarc made work as-is across older JVMs, then I think we should plan on using it as a measure to support newer JVM features while maintaining our Java6 byte code.

I'm not a fan of using multiple packages unless we actually add language/code features for the high versions of supported JVMs (like streams, lambda, Optional, etc).

@stleary
Copy link
Owner

stleary commented Dec 3, 2018

@GedMarc feel free to create a pull request to update the readme to offer users the option of downloading your alternate jar. It can appear at the top of the readme.

@johnjaylward
Copy link
Contributor

@stleary the PR is against @BGehrels repository which would update our future releases to support this. I don't think updating the readme is the way to go.

@stleary
Copy link
Owner

stleary commented Dec 3, 2018

Missed that, sorry. The changes should be made to a fork of @BGehrels repo, not the original. Or people can just get the pull request branch. No changes to how JSON-Java releases are packaged should be made at this time. If you don't want to update the readme, I guess people can learn about it from this issue.

@xzel23
Copy link

xzel23 commented Dec 3, 2018

I'm not a fan of using multiple packages unless we actually add language/code features for the high versions of supported JVMs (like streams, lambda, Optional, etc).

@johnjaylward: Being able to create a redistributable runtime that only contains what's needed for your application to run is a new feature that is enabled by providing strictly named modules. As an example, I have created a markdown-viewer application that is ready to run with 78 MB in size (application + jlink-generated JVM) as opposed to more than 250 MB (JVM alone) when not using jlink to create a stripped down version of the JVM. jlink does not work with automatic modules.

@johnjaylward
Copy link
Contributor

johnjaylward commented Dec 3, 2018

Creating a second package (whatever we call it) may give a false impression that newer JVMs are implicitly supported. My main issue is that going forward, JVMs (9+) are not guaranteed to be backwards compatible. Granted we are not using features that are likely to be deprecated/removed and cause compatibility issues, but it's still a valid concern that library maintainers need to take into consideration.

Using the approach from @GedMarc seems logical and a small footprint, but as I've stated multiple times, the resulting JAR needs to be tested against multiple JVMs. In JVMs 9+, it would also need to be tested on both the module path and the classpath (as noted in Stephen's blog linked earlier in this thread).

It sounds like @stleary does not want to take on that maintenance task at this time. If that's the case and @GedMarc's PR will not be rolled into the main release branch, then going with the README change and linking to the PR branch is probably the best bet at the moment.

@GedMarc
Copy link
Author

GedMarc commented Dec 3, 2018

Hmmm,
This was mostly a "I love your library let's keep it relevant", for my purposes I don't need anything under JRE8 and can go the route of custom-built libraries, also I tackle the build and deploy strategy a little differently for multiple repo destinations so anything I do to it outside the scope of keeping the original package in tact will completely destroy any commit and pull request I do here on.

I think I'm going to rather go to my fork and go the lint and rework for jre8, technically JDK6 died off 2 days ago and with 7 falling away in 6 months.

Sadly I really do need a named module so I can't use this one anymore.

@GedMarc GedMarc closed this as completed Dec 3, 2018
@BGehrels
Copy link
Contributor

BGehrels commented Dec 4, 2018

Hmm, it's sad that @GedMarc closed this issue, because i consider the discussion highly valuable.

I'm also highly irritated by the way motivated contibutor is bounced without even a simple "Thank you for the work you've invested and your efforts to keep our library up to date. We sadly can't merge it but highly appreciate your contribution and input to this discussion".

The decision to stick to Java 6 was taken 3 years ago in #156 in order to support Android versions <4.4. While back then, ~50% of the android devices still used <4.4, it's just 3,5 % nowadays - and given the short live of mobile devices nowadays i guess it will be much less in 6 months from now. So maybe it's time to drop this requirement. Spring, for example, dropped support for java 6 and 7 with version 5 in september this year. I wouldn't go that far since Java 8 is not fully supported in Android < 7, which is nowadays still used by ~50 % of the devices. Maybe @snodnipper - who advocated for Java 6 support back then - can contribute a bit of expertise here?

While i understand the argument that manually testing against n different jdks/jres is a huge burden, i guess that's what CI systems like https://travis-ci.org/ are made for. Travis also integrates nicely with github and can run a test, build and deploy on a wide range of different environments.

Those benefits will be much easier to leverage if we have the pom, the tests and the code in one place. This raises the question: Which value do we get from splitting the unit tests and the code in two distinct repositories? Which value do we get from maintaing the maven central build in yet another seperate repository with a seperate maintainer?

So maybe, instead of draining contributors of their motivation, we should see java 11 as a chance to renew the project and put it on stable, more maintainable feet?

@johnjaylward
Copy link
Contributor

johnjaylward commented Dec 4, 2018

I agree. I think it's a little premature to close this issue yet. I think we can find a good resolution that will work.

I know we have some restrictions on public API changes based on the hands-off from Douglas, but I think merging the test cases back into mainline and possibly Implementing some build tool integration are a great place to go forward.

We should highly consider these things going forward. There are multiple times even recently that contributors have had trouble finding tests, or in this case the distribution POM.

Consolidating the projects should simplify a lot, both new contributors and even old contributors like myself.

@GedMarc
Copy link
Author

GedMarc commented Dec 4, 2018

I closed the PR and issue cause I need something working for the weekend :)
With the changes that I'm going to make, (I don't want to branch off and config CI again), the PR would never be able to be pulled in. Also going off on my branch instead, I've bumped up my version to min JRE8.

I think it's pretty simple and straight forward, there's nothing funny going on, still JDK6 build nothing changed there at all, it's purely just a moditect + module-info in the build (the PR was on the right github repo I think)

I've already punched through a large portion of the library so I can't re-open the PR, the group, build method, StringBuffer->StringBuilder, Lint'ing and etc have all already been changed. There's like 160 more lint's to get my branch to an acceptable level for JRE8 (for me) so not going to revert it.
https://github.com/GedMarc/JSON-java

@johnjaylward The build and pom structure is a bit weird 👍

@GedMarc
Copy link
Author

GedMarc commented Aug 6, 2019

Would you like me to provide a PR?

Thanks for reopening

@stleary
Copy link
Owner

stleary commented Aug 6, 2019

@GedMarc, Yes, please add a PR.

@GedMarc
Copy link
Author

GedMarc commented Aug 20, 2019

Update please -

Why is this not pulled in, Why is there no feedback

@johnjaylward
Copy link
Contributor

@stleary I've commented on the linked PR, I'm no subject expert on modules, but the changes look like they shouldn't cause any issues with backwards compatibility to me.

@johnjaylward
Copy link
Contributor

@stleary If you could also provide guidance on if we should do a new package release or not to get the named module support in (as opposed to the auto module naming we have currently), that would be great. I think a new release would be the best course myself just so we can get community feedback if there are any issues.

@stleary
Copy link
Owner

stleary commented Aug 20, 2019

@johnjaylward, agreed, I also commented, and thanks for doing the analysis.

@jordanamr
Copy link

whoop whoop, JDK 6 finally EOL!!!!

JDK 6 EOL = December 2018
JDK 7 EOL = July 2019
JDK 8 EOL = March 2022
JDK 9 EOL = March 2018
JDK 10 EOL = September 2018
JDK 11 EOL = September 2026

Whoop whoop, we are in 2020, what's going on with this issue?

@GedMarc
Copy link
Author

GedMarc commented Jan 4, 2020

See end comments here with maven coordinates
BGehrels#4

https://search.maven.org/artifact/com.guicedee.services/json

Although the version is changing (with the encapsulating guicedee project) it is fully in-sync with source

@stleary
Copy link
Owner

stleary commented Jan 4, 2020

Comment added to PR 4

@stleary
Copy link
Owner

stleary commented Jan 5, 2020

Please see comments in BGehrels#4. Looks like no further work is planned for this issue.

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

No branches or pull requests

6 participants