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

Fix #54 Add module-info.java #149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smehrbrodt
Copy link
Contributor

As suggested in #54, add a module-info to odfdom project, so that users who are using java module based project setup can benefit from this.

@smehrbrodt
Copy link
Contributor Author

There seems to be some issues with running tests now, anyone got an idea?

@svanteschubert
Copy link
Contributor

First I thought it has something to do with the new Jena version (I have just updated the Jena version yesterday)
The current compile error is:
Error: /home/runner/work/odftoolkit/odftoolkit/odfdom/src/test/java/org/odftoolkit/odfdom/pkg/GRDDLTest.java:[50,32] cannot find symbol
Error: symbol: class ModelTestBase

But the missing dependency is there (ModelTestBase is within the latest test sources:
https://repo1.maven.org/maven2/org/apache/jena/jena-core/4.3.2/jena-core-4.3.2-test-sources.jar )
in addition, your branch still points to the old version (and stand-alone the build were fine).

So I thought I just add a new module in module-info.java for the test as it is a jar for its own (see above), but

requires org.apache.jena.core.test;

didn't work out, seems this JAR is not a module.. hmm...
As this did not work, I looked at the Jena docu and found that the pom.xml Maven descriptions changed and jena-core no longer is being referenced explicitly:
https://jena.apache.org/download/maven.html
Unfortunately the new way:

<dependency>
  <groupId>org.apache.jena</groupId>
  <artifactId>apache-jena-libs</artifactId>
  <type>pom</type>
  <version>4.3.2</version>
</dependency>

does not compile either (with same errors)
The pom XML does not point to core nor to their test
https://repo1.maven.org/maven2/org/apache/jena/apache-jena-libs/4.3.2/apache-jena-libs-4.3.2.pom

like we do

<dependency>
    <groupId>org.apache.jena</groupId>
    <artifactId>jena-core</artifactId>
    <version>4.3.1</version>
</dependency>
<dependency>
    <groupId>org.apache.jena</groupId>
    <artifactId>jena-core</artifactId>
    <classifier>tests</classifier>
    <version>4.3.1</version>
</dependency>

In addition, during compilation I stumbled over a different warning, which I am uncertain how to resolve:

--- maven-compiler-plugin:3.8.1:compile (default-compile) @ odfdom-java ---


  • Required filename-based automodules detected: [xercesImpl-2.12.1.jar, serializer-2.7.2.jar, java-rdfa-1.0.0-BETA1.jar, commons-validator-1.7.jar]. Please don't publish this project to a public artifact repository! *

which lead me to this article:
https://stackoverflow.com/questions/46501047/what-does-required-filename-based-automodules-detected-warning-mean

Anyway, if anybody can assist as I likely will not be able to look at it before next Tuesday!
Thanks in advance

@svanteschubert svanteschubert added the help wanted Extra attention is needed label Jan 11, 2022
@svanteschubert
Copy link
Contributor

svanteschubert commented Jan 15, 2022

To me, it seems I (or we) need additional information to solve this riddle.
I just checked:

  1. In both JARs of Jena core there is no module-info.java (unzipped the JARs of the Maven repo https://repo1.maven.org/maven2/org/apache/jena/jena-core/4.3.2/)
  2. The way they create two JARs from one pom.xml identifier seems legitimate: Look for 'classifier' in https://maven.apache.org/pom.html

Therefore, I have asked them to add module-info.java themselves:
https://issues.apache.org/jira/browse/JENA-2253

Perhaps, when they face the same problem with the java-core-tests classes and are able to fix it at the root.

Otherwise, any help is welcome. Won't be able to spend time on this for the next weeks...

@afs
Copy link

afs commented Jan 20, 2022

Apache Jena produces modules using automatic module names.

What do you use the test artifacts for? The tests do not run without the data files which are not in the test-jar. The test-jars do contain some code used by other parts of Jena.

@@ -0,0 +1,95 @@
/*
* Copyright 2022 The Apache Software Foundation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a copy/paste error here:
The Copyright does not belong to ASF but is likely belonging to "The Document Foundation" or you have the copyright.
My thanks to Andy Seabourne, for pointing this out.

Copy link
Contributor

@svanteschubert svanteschubert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adopt the tiny copy/paste issue - and somehow this should build :-)

@svanteschubert
Copy link
Contributor

Andy Seaborne (of the Apache JENA project) was so kind to the following adjustments for JENA:
"The test artifacts for jena-base, jena-core and jena-arq now have different modules names to the main artifacts ".test" was added." see https://issues.apache.org/jira/browse/JENA-2253?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&focusedCommentId=17481275#comment-17481275

@svanteschubert
Copy link
Contributor

@smehrbrodt
It seems the Jena test class can still not be found in the current regression test when using module-info.java
https://github.com/tdf/odftoolkit/runs/4938595062?check_suite_focus=true#step:4:715
Does it build locally, Samuel?

I have tried yesterday with a local build of Jena (including Andys fixes from the PR two days ago) and now there is even a SNAPSHOT release available: https://repository.apache.org/content/repositories/snapshots/org/apache/jena/jena-core/4.4.0-SNAPSHOT/ (both times I got different problems - the usual Maven Surefire plugin crashing the JavaVM likely by System.exit() somewhere... little intransparent the problem...)

Any ideas?

@afs
Copy link

afs commented Jan 25, 2022

requires org.apache.jena.core; -- the test jar is org.apache.jena.core.test and it only needs to eb on the test time classpath,module aren't needed (I don't know how put modules on the model path for test only).

Earlier question: ModelTestBase : what do you use it for? Do you need it?

@svanteschubert
Copy link
Contributor

requires org.apache.jena.core; -- the test jar is org.apache.jena.core.test and it only needs to eb on the test time classpath,module aren't needed (I don't know how put modules on the model path for test only).

Earlier question: ModelTestBase : what do you use it for? Do you need it?

Hello Andy, thanks for the reply, yes I have overseen your earlier question, sorry.
We derive from the Jena test module class in our GRDDL test:
https://github.com/tdf/odftoolkit/blob/master/odfdom/src/test/java/org/odftoolkit/odfdom/pkg/GRDDLTest.java#L50

[ERROR] ./odftoolkit/odfdom/src/test/java/org/odftoolkit/odfdom/pkg/GRDDLTest.java:[42,38] package org.apache.jena.rdf.model.test does not exist
[ERROR] ./odftoolkit/odfdom/src/test/java/org/odftoolkit/odfdom/pkg/GRDDLTest.java:[50,32] cannot find symbol
symbol: class ModelTestBase

Just one question, as I got more specific error messages on Linux (the above VM crash is Windows - I had it before only on my machine). When I wanted to test your latest patch using the SNAPSHOT build, exchanging the Jena version to
4.4.0-SNAPSHOT
at two places:
https://github.com/tdf/odftoolkit/blob/master/pom.xml#L75
and
https://github.com/tdf/odftoolkit/blob/master/pom.xml#L81

I get:
"The following artifacts could not be resolved: org.apache.jena:jena-shaded-guava:jar:4.4.0-SNAPSHOT, org.apache.jena:jena-iri:jar:4.4.0-SNAPSHOT, org.apache.jena:jena-core:jar:tests:4.4.0-SNAPSHOT: Could not transfer artifact org.apache.jena:jena-shaded-guava:jar:4.4.0-20220125.140831-15 from/to apache.snapshots (https://repository.apache.org/snapshots): transfer failed for https://repository.apache.org/snapshots/org/apache/jena/jena-shaded-guava/4.4.0-SNAPSHOT/jena-shaded-guava-4.4.0-20220125.140831-15.jar"

Any idea or suggestion on how to continue?

@svanteschubert
Copy link
Contributor

@afs What is strange. Without module-info.java your Jena test classes are found, but with module-info.java the class can no longer be found. Strange, isn'it?

@mistmist
Copy link
Contributor

mistmist commented Jan 25, 2022

just tried to build this with 4.4.0-SNAPSHOT twice in pom.xml, i get this very well hidden error message about 2 modules defining the org.w3c.dom.html package (on the terminal it is reported as a JVM startup failure, have to look at a log file to see).

Corrupted STDOUT by directly writing to native stream in forked JVM 1. Stream 'java.lang.module.ResolutionException: Modules xercesImpl and jdk.xml.dom export package org.w3c.dom.html to module java.rdfa'.
java.lang.IllegalArgumentException: Stream stdin corrupted. Expected comma after third character in command 'java.lang.module.ResolutionException: Modules xercesImpl and jdk.xml.dom export package org.w3c.dom.html to module java.rdfa'.

@afs
Copy link

afs commented Jan 25, 2022

It can't see any use of ModelTestBase other than it inherits from junit.framework.TestCase and has a @Test so is that necessary?

Personally - I'd remove the extends and see what breaks - maybe easier to fix up and will give better stability.

@afs
Copy link

afs commented Jan 25, 2022

Strange, isn'it?

It is modulepath related. No modules - test code on the classpath.

Putting reusable test framework in test jars isn't great and may not play with modules but it has been that way for a very long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants