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

pull request: compile with java 11 #245

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@aanno
Copy link

aanno commented Dec 27, 2018

When I try to compile relaxng/jing-trang branch master with java 11, I get the following error:

BUILD FAILED
/home/tpasch/scm/github/jing-trang/build.xml:43: java.lang.ClassNotFoundException: com.icl.saxon.TransformerFactoryImpl
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:315)

My pull request fixes the problem by:

  • removing the (build) dependency on the (old) com.icl.saxon: The ant build now only uses the modern saxon9 jar (net.sf.saxon)

In addition I tackled the following:

  • build is now for java8 (as java7 is long gone)
  • added a gradle build file (that only imports the ant build): This way, the project becomes easier to use in/with other gradle builds (as composite build or sub project)
    • But for this to work, I had to rename some of the ant tasks a bit:
      • init -> ant-init
      • jar -> ant-jar
      • clean -> ant-clean
      • javadoc -> ant-javadoc

I would be very glad if you could consider integration into the master branch.

Kind regards,

aanno

aanno added some commits Dec 27, 2018

@aanno

This comment has been minimized.

Copy link
Author

aanno commented Jan 16, 2019

I updated my gradle build file (build.gradle.kts) a bit.

I would like to repeat is that gradle stuff is completely optional, i.e. I would remove it from the PR if there are concerns.

@ndw

This comment has been minimized.

Copy link
Contributor

ndw commented Jan 16, 2019

This seems fine to me, @sideshowbarker do you have concerns?

@sideshowbarker

This comment has been minimized.

Copy link
Member

sideshowbarker commented Jan 16, 2019

This seems fine to me, @sideshowbarker do you have concerns?

Yes, I do have concerns.

For one, this breaks the build, because it renames the current ant build targets — specifically, the part of the patch that adds the gradle stuff.

So at a minimum, I think the gradle stuff needs to be removed and split out into a different PR. (Even if it weren’t breaking the build, it’s not so great to have single PR that’s doing multiple disparate things).

But even outside of the gradle stuff, it’s not clear to me that the “compile with java 11” changes here are necessary — or even that they don’t potentially regress anything.

The thing is, we’re already compiling with Java 11 fine in our Travis CI environment, without this patch. And I also compile with Java 11 locally in my (MacOS) environment without this patch.

The change in this patch essentially just completely removes the saxon.jar file from the build and from the repo. It’s not clear to me that’s something we really want to do — I don’t know if something actually could be depending on the saxon.jar file which would regress if it isn’t there any longer.

But certainly I can say that it’s not necessary to remove the saxon.jar file in order to build successfully on Java 11 — because as I noted above, we’re already building under Java 11 with saxon.jar in place.

@aanno

This comment has been minimized.

Copy link
Author

aanno commented Jan 17, 2019

Well, thank you for feedback.

For me it's rather strange that you never encountered a ClassNotFoundException on builds - because it happens to me every time. As an informed guess, I would throw suspect on a classpath issue.

What I actually did is to remove the (very old) saxon.jar and use the (already present) saxon9.jar as replacement. For me it looks like that saxon.jar was only used for building the project.

I now thinks that is not true: there is the OldSaxonSchemaReaderFactory that uses the old saxon (package ns com.icl.saxon) while NewSaxonSchemaReaderFactory uses saxon9 (package ns net.sf.saxon).

So it boils down to (a) if there is still any need to support the aged saxon and (b) find a convincing argument why having both saxon on classpath could lead to ClassNotFoundException with java 11.

I completly agree that the PR needs separation of the gradle stuff.

@sideshowbarker

This comment has been minimized.

Copy link
Member

sideshowbarker commented Jan 18, 2019

For me it's rather strange that you never encountered a ClassNotFoundException on builds - because it happens to me every time.

What OS distro are on testing on? Yesterday at validator/validator#744 (comment) I got a report of a ClassNotFoundException that seems to happen only on Fedora with the openjdk11 package.

Do you have multiple OS distros you’re able to test on? And if so can you reproduce the ClassNotFoundException on those?

We currently have data to indicate the ClassNotFoundException problem reported here isn’t reproducible in the default Java environment on MacOS with openjdk11, nor on Ubuntu with openjdk11, oraclejdk11, openjdk10, oraclejdk9, or openjdk-ea (=12) — because those are what our Travis CI tests.

As an informed guess, I would throw suspect on a classpath issue.

Yes I can vaguely understand that it must be related somehow to the Modules feature introduced in Java 9 — and thus maybe the underlying issue reported at #246 — but I personally don’t have any experience so far in trying to troubleshoot and fix any problems related to that.

And regardless, I would expect that whatever difference were caused by the Modules feature, we’d consistently run into those problems no matter what OS distro we ran the build on.

So the only wild guess I can personally come up with at this point is that maybe in some environments (e.g., Fedora) the OS distro packagers/maintainers have the openjdk11 package configured (and maybe the openjdk10 and openjdk9 packages) to flip on some non-default Java environment setting related to Modules that breaks backward compatibility.

I now thinks that is not true: there is the OldSaxonSchemaReaderFactory that uses the old saxon (package ns com.icl.saxon) while NewSaxonSchemaReaderFactory uses saxon9 (package ns net.sf.saxon).

Right

@aanno

This comment has been minimized.

Copy link
Author

aanno commented Jan 20, 2019

Well, my main system is indeed a Fedora 29:

$ lsb_release -a
LSB Version:    :core-4.1-amd64:core-4.1-noarch:cxx-4.1-amd64:cxx-4.1-noarch:desktop-4.1-amd64:desktop-4.1-noarch:languages-4.1-amd64:languages-4.1-noarch:printing-4.1-amd64:printing-4.1-noarch
Distributor ID: Fedora
Description:    Fedora release 29 (Twenty Nine)
Release:        29
Codename:       TwentyNine

On this system, I tested 3 Java 11: (1) the Fedora RPM package, (2) the Oracle TGZ, (3) the OpenJDK TGZ. Result: always ClassNotFoundException.

Then I switched to a Ubuntu 18.10:

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 18.10
Release:        18.10
Codename:       cosmic

On this system, I tested the Ubuntu DEB Java 11 JDK:

$ $JAVA_HOME/bin/java -version
openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment (build 11.0.1+13-Ubuntu-2ubuntu1)
OpenJDK 64-Bit Server VM (build 11.0.1+13-Ubuntu-2ubuntu1, mixed mode, sharing)

Result: ClassNotFoundException again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment