RFR: 8338675: javac shouldn't silently change .jar files on the classpath

[Jan Lahoda]

On Wed, 19 Feb 2025 15:46:34 GMT, David Beaumont <duke at openjdk.org> wrote:

> Modifying `JavacFileManager` to skip creating sibling output class files for source files found in JARs.
> 
> This should match older (JDK 8) behavior whereby the JAR was not writable, and results in any newly generated class files being written to the current working directory (the output of class files into current directory isn't good, but it should match the old behavior).

The fix as such seems reasonable to me. The test is good, but I added some comments for possible improvements for the test.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 1:

> 1: /*

I would suggest to move the file into `test/langtools/tools/javac/file`. The filenames and directory names based on bug numbers are mostly historical, and are being frowned upon nowadays.

Overall, the test is not bad at all, but I'll leave some comments that I think would make fit better with the other tests.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 66:

> 64:  * current directory may not be usable.
> 65:  */
> 66: public class NoOverwriteJarClassFilesByDefault {

Overall, I think I would mildly prefer if the test used either `TestRunner`, or junit:
https://github.com/openjdk/jdk/blob/master/test/langtools/tools/javac/ImportModule.java
https://github.com/openjdk/jdk/blob/master/test/langtools/tools/javac/util/IteratorsTest.java

so that it is easier to add or have multiple testcases.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 68:

> 66: public class NoOverwriteJarClassFilesByDefault {
> 67: 
> 68:     private static final String OLD_LIB_SOURCE = """

Possibly a personal preference, but I think I would put the opening `"""` on the next line, so that the text block is visibly joint.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 95:

> 93:         ToolBox tb = new ToolBox();
> 94:         tb.createDirectories("lib");
> 95:         tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE);

Using `Path`s based on relative paths is not wrong, I think, but I think I would suggest to create same `Path base = Paths.get("<testcasename>");`, and in the rest of the test use `Path` derived from the `base` path. So that it is easier to e.g. change the location of the working directory.

Also note `ToolBox` has `writeJavaFile`, which infers the relative path automatically in almost all cases (not so useful in this case, of course, as this test needs to know the real path anyway).

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 100:

> 98:         // The class file generated he is in the lib/ directory, which we delete
> 99:         // after making the JAR (just to be sure).
> 100:         new JavacTask(tb).files(LIB_SOURCE_NAME).run();

I tend to use `.writeAll()` after `.run()`, so that if there's any output, it is written out. (Admittedly, the run method could be changed to do that automatically, but I don't think it does currently.)

Also, nit: might be nicer to format this using a "builder" style:

new JavacTask(tb)
    .file(LIB_SOURCE_NAME)
    .run()
    .writeAll();


Also note there's `ToolBox.findJavaFiles`, which finds Java sources in the given directories. No that useful here, but might be useful in some cases.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 105:

> 103:         // it proves it's getting it from the source file in the JAR.
> 104:         Instant olderTime = Instant.now();
> 105:         Instant newerTime = olderTime.plusSeconds(1);

I would slightly prefer to make the time distance longer - maybe 60s. Mostly because (historically?) IIRC some timestamp precision was 2s. It is not a factor here, but given the timestamp is only a number here, we can make the difference larger.

There could also be a second test that would not include the classfile at all - the test would work the same, and would be immune to changes in timestamp/ordering.

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java line 130:

> 128: 
> 129:         // Before running the test itself, get the CRC of the class file in the JAR.
> 130:         long originalLibCrc = getLibCrc();

I think I would suggest to compute some hash for the whole jar, not only for the classfile. There should be no changes to the jar at all.

-------------

PR Review: https://git.openjdk.org/jdk/pull/23699#pullrequestreview-2632199994
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1964978046
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1964981518
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1964985930
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1964991373
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1964995081
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965068517
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965096206