RFR: JDK-8151914 java/util/jar/JarFile/MultiReleaseJar* tests do not declare module dependences

[Alexandre (Shura) Iline]

Hi,

Can you please take a look on:
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
?

Thank you

Shura

[Mandy Chung]

Post by Alexandre (Shura) Iline
Hi,
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
?

Looks okay to me.

Mandy

[Steve Drach]

Looks fine to me, although I am not an official reviewer. Thanks for doing this.

Post by Alexandre (Shura) Iline
Hi,
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
?
Thank you
Shura

[Chris Hegarty]

Post by Steve Drach
Looks fine to me,

+1.

-Chris.

Post by Steve Drach
although I am not an official reviewer. Thanks for doing this.

Post by Alexandre (Shura) Iline
Hi,
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/
?
Thank you
Shura

[Chris Hegarty]

Post by Alexandre (Shura) Iline
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/

Taking another look over this before sponsoring….

The test library dependency seems to be on java.compiler, and not jdk.compiler,
right ? Also, I see no reason for the addition of jdk.zipfs.

-Chris.

[Alan Bateman]

Post by Chris Hegarty

Post by Alexandre (Shura) Iline
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/

Taking another look over this before sponsoring….
The test library dependency seems to be on java.compiler, and not jdk.compiler,
right ? Also, I see no reason for the addition of jdk.zipfs.

java.compiler is just the API module, the runtime image needs to have
jdk.compiler linked in to be useful. So I think @modules jdk.compiler is
right here as otherwise the test will be selected when testing a "JRE".
java. javac needs the zipfs provider in order to compiler against
libraries that are packaged as JAR/zip files so I assume this is why it
listed here.

-Alan.

[Chris Hegarty]

Post by Chris Hegarty

Post by Alexandre (Shura) Iline
http://cr.openjdk.java.net/~shurailine/8151914/webrev.01/

Taking another look over this before sponsoring….
The test library dependency seems to be on java.compiler, and not jdk.compiler,
right ? Also, I see no reason for the addition of jdk.zipfs.

java.compiler is just the API module, the runtime image needs to have jdk.compiler linked in to be useful.

So the test depends on both the compiler API and impl, which is implicit
with the 'requires public java.compiler’ in the jdk.compiler module. That’s
fine, and makes sense.
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.

This is all to fragile and may cause problems with future refactoring. I
think we should add the same set of @moduels to all these tests, rather
than an individual set determined by intimate knowledge of the inner
workings of the test.

@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool

with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

-Chris

[Alan Bateman]

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

or we could move the tests into their own MultiRelease sub-directory and
create a TEST.properties with a module key. That would allow these tests
to drop @modules, except the test that uses the HTTP server.

-Alan

[Chris Hegarty]

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.

-Chris.

[Alexandre (Shura) Iline]

This makes sense - I will move the tests into a subduer, put the dependencies into a TEST.properties file.

jdk.zipfs has the code needed for access jars - the tests are failing without that dependency.

Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.

[Alexandre (Shura) Iline]

Chris, could you please take another look:
http://cr.openjdk.java.net/~shurailine/8151914/webrev.02/

What I have discovered is that jdk.zipfs was used to access jars on the classpath, which were JTreg jars: jtreg.jar, testing.jar, etc. Cleaning the class path through the environment removed dependency on the zipfs.

Whether the java.tools API behavior is correct is a separate matter. I am planning to create a standalone test case and take it with javac ppl.

Thank you.

Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.

[Alexandre (Shura) Iline]

Post by Alexandre (Shura) Iline
Whether the java.tools API behavior is correct is a separate matter. I am planning to create a standalone test case and take it with javac ppl.

I take this ^^^^^^ back, as the error was there all along: "java.nio.file.ProviderNotFoundException: no provider found for .jar files”

The fix is valid, then, is and waiting for a review.

Shura

Post by Alexandre (Shura) Iline
Thank you.
Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.

[Jonathan Gibbons]

There is potentially a separate discussion here, as to whether javac
should "force" the provision of a jar-fs provider.

Strictly speaking, javac does not inherently require it. You can use
javac just fine, with files in the system image, and source and class
files in the default file system. But I suspect most folk would be
suprised if they ca across an image for which javac could not read jar
files.

-- Jon

Post by Alexandre (Shura) Iline

Post by Alexandre (Shura) Iline
Whether the java.tools API behavior is correct is a separate matter. I am planning to create a standalone test case and take it with javac ppl.

I take this ^^^^^^ back, as the error was there all along: "java.nio.file.ProviderNotFoundException: no provider found for .jar files”
The fix is valid, then, is and waiting for a review.
Shura

Post by Alexandre (Shura) Iline
Thank you.
Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.

[Alexandre (Shura) Iline]

There is potentially a separate discussion here, as to whether javac should "force" the provision of a jar-fs provider.
Strictly speaking, javac does not inherently require it. You can use javac just fine, with files in the system image, and source and class files in the default file system. But I suspect most folk would be suprised if they ca across an image for which javac could not read jar files.

What would definitely help is a error message which specifies the jar name. At least in my case that would, as I would not miss the message.

Shura

-- Jon

Post by Alexandre (Shura) Iline

Post by Alexandre (Shura) Iline
Whether the java.tools API behavior is correct is a separate matter. I am planning to create a standalone test case and take it with javac ppl.

I take this ^^^^^^ back, as the error was there all along: "java.nio.file.ProviderNotFoundException: no provider found for .jar files”
The fix is valid, then, is and waiting for a review.
Shura

Post by Alexandre (Shura) Iline
Thank you.
Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.

[Chris Hegarty]

Post by Alexandre (Shura) Iline
http://cr.openjdk.java.net/~shurailine/8151914/webrev.02/

This looks ok to me Shura, maybe just 'mrjar' for the directory
name?

Since there are file moves can you please prepare the changeset,
and I will push it for you.

-Chris.

Post by Alexandre (Shura) Iline
What I have discovered is that jdk.zipfs was used to access jars on the classpath, which were JTreg jars: jtreg.jar, testing.jar, etc. Cleaning the class path through the environment removed dependency on the zipfs.
Whether the java.tools API behavior is correct is a separate matter. I am planning to create a standalone test case and take it with javac ppl.
Thank you.
Shura

Post by Chris Hegarty

Post by Chris Hegarty
The tests cause compilation of test library classes, but only some tests
actually use the methods that provoke compilation. Similar to above, tests
that don’t actually compile anything could depend on just java.compiler.
This is all to fragile and may cause problems with future refactoring. I
than an individual set determined by intimate knowledge of the inner
workings of the test.
@modules java.compiler
jdk.compiler
jdk.zipfs
jdk.jartool
with the addition of jdk.httpserver for MultiReleaseJarHttpProperties.

I think that would be better.
-Chris.