pull request: compile with java 11

aanno commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

sideshowbarker commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/validator/validator/pull/744#issuecomment-455121060 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 https://github.com/relaxng/jing-trang/issues/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 commented 5 years ago

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.

aanno commented 4 years ago

Closed in favour of #255

relaxng / jing-trang

pull request: compile with java 11 #245