JENA-1609 Jena master does not build under JDK10 #474

lewismc · 2018-10-01T18:42:12Z

This PR addresses https://issues.apache.org/jira/browse/JENA-1609

ajs6f

I don't know very much about Java 10 builds, but I'm a bit confused about some of these changes-- maybe we could get some in-line comments on them?

ajs6f · 2018-10-01T18:44:30Z

.gitignore

@@ -14,7 +14,10 @@ buildNumber.properties
 .project
 .settings
 .recommenders
-.metadata
+.metadat


Is this correct? .metadata => .metadat?

ajs6f · 2018-10-01T18:45:03Z

jena-arq/pom.xml

@@ -216,6 +216,7 @@
            </group>
          </groups>
          <bottom>Licenced under the Apache License, Version 2.0</bottom>
+          <additionalOptions>-html5</additionalOptions>


Is this in some way needed for Java 10 builds?

If it is not included there is a WARNING message as follows

javadoc: warning - You have not specified the version of HTML to use. The default is currently HTML 4.01, but this will change to HTML5 in a future release. To suppress this warning, please specify the version of HTML used in your documentation comments and to be generated by this doclet, using the -html4 or -html5 options.

The -html5 additional option cleans up presence of WARNING's during Maven build.

This setting is in the parent POM. It is commented out because the build fails with Java8. It is not the only setting up for the javadoc plugin.

Is setting in the parent enough? If it does, it is better to have in one place because ...

Has this PR been run with Java8? The build exits with an error in jena-iri:javadoc for me.

ajs6f · 2018-10-01T18:46:33Z

...src/main/java/org/apache/jena/hadoop/rdf/mapreduce/filter/AbstractNodeTupleFilterMapper.java

- * {@link #accepts(TKey, T)}
- * 
- * 
+ * {@link #accepts(Object, AbstractNodeTupleWritable)}


This just looks wrong-- the method below clearly is accepts(TKey key, T tuple), and the concrete class isn't there at all.

I agree... it was strange when it turned out that this seemed to be the fix. Without the code augmentation, I was unable to generate the Javadocs for the project. It should be noted that Javadoc generation is activated by default during an install target.

ajs6f · 2018-10-01T18:47:07Z

jena-jdbc/jena-jdbc-core/pom.xml

+			<artifactId>log4j</artifactId>
+		</dependency>
+
+		<dependency>


Why are you pulling in the log4j API?

Because the org.apache.log4j.PropertyConfigurator class is required in jena/jena-jdbc/jena-jdbc-core/src/main/java/org/apache/jena/jdbc/JenaDriver.java

IMO The fix is not to use log4 directly but to use SLF4J and LogCtl because then logger can be swapped. But, for history reasons, this may not be practical. cc @rvesse

Yup. We may have to do something like this, but it's definitely "papering-over" the underlying fault.

This being said, what would you rather see? This issue is titled Jena master does not build under JDK10 so I wonder if getting Jena to build under JDK 10 is the immediate response. Someone can then go and work on the underlying logging issue. Right now I have not investigated it enough or else I would have provided the fix.

JenaDriver can optionally configure Log4j, this is because JDBC drivers can be used in contexts where the end user can't directly configure logging e.g. when dropped into other applications. In those contexts users can add logging=<log4j-config> in their JDBC connection string to have the driver configure logging for them.

However the default behaviour is to not configure anything and all the code internally uses SLF4J so in environments where users can control logging configuration they are free to do that. So in environments where the user fully controls things they can configure logging as appropriate.

Ok thanks @rvesse. The logic seems clear from what you wrote, it doesn't however take away from the fact that the codebase will not build under JDK10 (and possibly JDK9) without the presence of the log4j library containing org.apache.log4j.PropertyConfigurator as currently used within jena/jena-jdbc/jena-jdbc-core/src/main/java/org/apache/jena/jdbc/JenaDriver.java.
The issue here is to either refactor the logging or to add the dependency... I would think there is no other scenario under consideration.
Is that a fair statement?

lewismc · 2018-10-01T19:22:51Z

Thanks for comments @ajs6f

acoburn · 2018-10-01T19:27:50Z

It is possibly worth noting here that these changes also make it possible to build Jena on JDK 11.

lewismc · 2018-10-01T19:56:34Z

Thats good to know @acoburn thanks for chiming in.

afs · 2018-10-01T20:12:24Z

See https://builds.apache.org/view/H-L/view/Jena/job/Jena_JDK11/ which builds with -Pdev and has been running for a while. It does not build jena-jdbc or jena-elephas both of which need looking at.

lewismc · 2018-10-01T23:31:02Z

Thanks @afs I looked at the build and built locally using the following parameters

mvn clean verify -Pdev

... it results in SUCCESSFUL build.

rvesse · 2018-10-02T09:36:00Z

jena-jdbc/jena-jdbc-core/pom.xml

+			<artifactId>log4j</artifactId>
+		</dependency>
+
+		<dependency>


JenaDriver can optionally configure Log4j, this is because JDBC drivers can be used in contexts where the end user can't directly configure logging e.g. when dropped into other applications. In those contexts users can add logging=<log4j-config> in their JDBC connection string to have the driver configure logging for them.

However the default behaviour is to not configure anything and all the code internally uses SLF4J so in environments where users can control logging configuration they are free to do that. So in environments where the user fully controls things they can configure logging as appropriate.

rvesse · 2018-10-02T09:39:01Z

jena-jdbc/jena-jdbc-core/pom.xml

+
+		<!-- Intercept direct use of log4j2 -->
+		<dependency>
+			<groupId>org.apache.logging.log4j</groupId>


I think this is unnecessary, the only Log4j direct usage is to configure Log4j, the code itself always uses SLF4J for its logging

Agreed - I checked using the current master branch. Only log4j1 is used (log4j::log4j).

What I am baffled by is that fact it builds with java8 and not with java10 and the failure is a dependency missing. log4j1 is a "optional" dependency in the parent.

OK so we agree on this however the code doesn't build due to the missing dependency. I wasn't able to build without the additions as proposed.

I'm unclear here. I tried with JDK10 and had to add the log4j1 dependency but not the log4j2 dependencies.

In the code I can only find imports of org.apache.log4j, which is log4j1, not org.apache.logging.log4j which is log4j2. Something weird is going on anyway that the log4j1 dependency is necessary. @lewismc, are the group "org.apache.logging.log4j" dependencies necessary for you? If so, what's the build failure?

Elasticsearch does use log4j2 and Jena adds adapters there for that.

More:

Confirmed I have only have to add a single log4j1 dependency.
The error is otherwise from the maven-javadoc-plugin, not the compiler:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.0.0:jar (attach-javadocs) on project jena-jdbc-driver-bundle: MavenReportException: Error while generating Javadoc: [ERROR] Exit code: 1 - /home/afs/Jena/jena-jdbc/jena-jdbc-core/src/main/java/org/apache/jena/jdbc/JenaDriver.java:42: error: package org.apache.log4j does not exist [ERROR] import org.apache.log4j.PropertyConfigurator; [ERROR] ^ [ERROR] [ERROR] Command line was: /usr/lib/jvm/default-java/bin/javadoc @options @packages [ERROR] [ERROR] Refer to the generated Javadoc files in '/home/afs/Jena/jena-jdbc/jena-jdbc-driver-bundle/target/apidocs' dir. [ERROR]

Agreed. This is the behavior I get as well.
My thinking was that the dependency needed to be available at compile time as oppose to only runtime... but maybe this thinking is wrong.
The generated Javadoc look fine but the error prevents a stable build.

From what I understand, the offending class org.apache.log4j.PropertyConfigurator resides in log4j 2.0... the runtime dependencies are detailed as follows with the Maven Artifacts detailed here.

This code predates Log4j 2.x so shouldn't have any dependency upon it

Wrong PropertyConfigurator - there are two (!!), one in log4j1 and one in the log4j2 for compatibility code log4j-1.2-api/src/main/java/org/apache/log4j/PropertyConfigurator.java.

Jena is picking up the one from log4j1. So the log4j2 dependencies aren't need but the log4j1 is. Either will do to compile but log4j1 is intended.

afs · 2018-10-02T12:36:32Z

I'm trying to get a single build configuration for both java10 and java8.

I can't get Jena to build with java8 if <additionalOptions>-html5</additionalOptions> is set.

It is a problem in the full build because skipping javadoc isn't an option - the distributions include javadoc by dependency.

With java 10.0.2 (Ubuntu latest with the placeholder java11 - still waiting for real java11): jena-elephas-common fails because:

[ERROR] Failed to execute goal on project jena-elephas-common: Could not resolve dependencies for project org.apache.jena:jena-elephas-common:jar:3.10.0-SNAPSHOT: Could not find artifact jdk.tools:jdk.tools:jar:1.6 at specified path /usr/lib/jvm/java-11-openjdk-amd64/../lib/tools.jar -> [Help 1]

(it also used MRUnit which is a retired project)

A pure maven build will put the java11 runtime (no tools.jar) on the compile classpath.

@lewismc - am I right in concluding it worked for you in the IDE because the java8 runtime was on the build path?

rvesse · 2018-10-02T13:07:04Z

@afs For some projects where I have added JDK 9/10 support I have had to create separate profiles activated by JDK version for the Javadoc plugin because there are breaking changes between the plugin versions and supported flags for the underlying javadoc tool

Also if you have updated the Javadoc plugin version to a 3.x release then you need to add an <option> sub-element within <additionalOptions> rather than just specifying the options directly

lewismc · 2018-10-03T02:15:07Z

@afs how about we propose the change to

<additionalOptions>-html4</additionalOptions>

This would produce JDK WARNING's under several environment's however would at least enable successful build under this environments.

lewismc · 2018-10-03T02:15:39Z

@afs I have not tried this build under JDK11 yet, neither did I try to build under an IDE. I just build using my terminal.

afs · 2018-10-03T08:49:15Z

@lewismc

@afs how about we propose the change to
<additionalOptions>-html4</additionalOptions>
This would produce JDK WARNING's under several environment's however would at least enable successful build under this environments.

The problem is not html4/html5 (we can clean up the javadoc) but compatible with java8 javadoc tool. Java8 build causes:

Exit code: 1 - javadoc: error - invalid flag: -html4

I'd prefer that the build uses java8 for java8 libraries because cross-compiling does not catch changes in the runtime libraries.

afs · 2018-10-03T08:57:46Z

@rvesse - The current plugin in the build is 3.0.0. <option> didn't change the situation. Can you given an example or point to one? (the online plugin doc does not talk about <option>

Profiles: that would be great. Again, a sample to start from would help. While all the suggestions here are great, it's going to be slower if I have to carry out each one from scratch.

rvesse · 2018-10-03T09:11:19Z

@afs https://github.com/rvesse/airline/blob/master/pom.xml#L495-L497 which I discovered via JIRA https://issues.apache.org/jira/browse/MJAVADOC-475

Notice that same POM configures the Javadoc plugin differently for each supported JVM via profiles

lewismc · 2018-10-03T16:38:31Z

@afs

The problem is not html4/html5 (we can clean up the javadoc) but compatible with java8 javadoc tool. Java8 build causes:

Got it.

I'd prefer that the build uses java8 for java8 libraries because cross-compiling does not catch changes in the runtime libraries.

So do you want to abandon this PR and state that Jena should be used with JDK 1.8? If so I can close out this issue. Unless of course if you can propose an alternative.

rvesse · 2018-10-03T17:34:17Z

I think there is a simpler solution to this, I have Jena JDBC building locally under JDK10 and will push a new PR shortly

lewismc · 2018-10-03T17:46:50Z

@rvesse grand

afs · 2018-10-03T17:58:36Z

"should be used with JDK 1.8" has several facets.
There are different levels of Java11 usage: the project discussions back when it was Java9:

dev@ message and sent as part of users@ message.

Ultimately, it is a community decision.

My personal opinion is that Jena should be able to run on Java8 runtime. Research projects may upgrade quickly but enterprise deployments are another matter.

To ensure no accidents, I prefer the release build used Java8 because it protects against runtime library changes (we have one currently about tools.jar).

I'd like the build to work with Java11 - hence the commented line in the parent pom.xml and the Jenkins job, both of which came out of the project discussions from Feb. Currently, "-Pdev" works, the full release build does not.

I have ~/.mavenrc setup to make sure I gets builds right. I use JDK10 as the default java on my system (I am resisting installing Java11 manually and waiting for Ubuntu ... at least for now!), I make sure maven runs with Java8.

 # ~/.mavenrc
JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64

Java11 will change the ecosystem and we don't know how that plays out or what may come out of adoptjava and the Jan2019 deadline from Oracle. I think it will rapidly become clear.

rvesse · 2018-10-03T18:08:31Z

@afs Agreed, many folks are going to migrate Java 8 -> Java 11 because of the LTS release status and are skipping interim versions

ajs6f · 2018-10-03T18:22:33Z

@rvesse Yup, that's my site and every other one I know. That's not to say that this PR isn't worthwhile, just that I would certainly like to spend as much time as possible on 11, not 10.

afs · 2018-10-03T18:35:10Z

10 < 11. All these issues are in Java11 because Java 9 & 10 were the big changes and Java 11 the consolidation.

lewismc · 2018-10-03T18:46:49Z

Closing in favor of #476

afs · 2018-10-03T19:26:21Z

I've picked out the .gitignore changes (not for META-INF - that's needed) and added them.

JENA-1609 Jena master does not build under JDK10

18497ea

ajs6f requested changes Oct 1, 2018

View reviewed changes

JENA-1609 Jena master does not build under JDK10

1f86d71

JENA-1609 Jena master does not build under JDK10

116ed22

rvesse reviewed Oct 2, 2018

View reviewed changes

rvesse mentioned this pull request Oct 3, 2018

Fix JDK10 Build (JENA-1609) #476

Merged

lewismc closed this Oct 3, 2018

lewismc deleted the JENA-1609 branch October 3, 2018 18:47

JENA-1609 Jena master does not build under JDK10 #474

JENA-1609 Jena master does not build under JDK10 #474

Conversation

lewismc commented Oct 1, 2018

ajs6f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afs Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvesse Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewismc commented Oct 1, 2018

acoburn commented Oct 1, 2018

lewismc commented Oct 1, 2018

afs commented Oct 1, 2018 • edited

lewismc commented Oct 1, 2018

rvesse Oct 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afs Oct 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afs Oct 3, 2018 • edited

Choose a reason for hiding this comment

afs commented Oct 2, 2018

rvesse commented Oct 2, 2018

lewismc commented Oct 3, 2018

lewismc commented Oct 3, 2018 • edited

afs commented Oct 3, 2018

afs commented Oct 3, 2018

rvesse commented Oct 3, 2018 • edited

lewismc commented Oct 3, 2018

rvesse commented Oct 3, 2018

lewismc commented Oct 3, 2018

afs commented Oct 3, 2018

rvesse commented Oct 3, 2018 • edited

ajs6f commented Oct 3, 2018

afs commented Oct 3, 2018

lewismc commented Oct 3, 2018

afs commented Oct 3, 2018

afs Oct 1, 2018 •

edited

rvesse Oct 2, 2018 •

edited

afs commented Oct 1, 2018 •

edited

rvesse Oct 2, 2018 •

edited

afs Oct 3, 2018 •

edited

afs Oct 3, 2018 •

edited

lewismc commented Oct 3, 2018 •

edited

rvesse commented Oct 3, 2018 •

edited

rvesse commented Oct 3, 2018 •

edited