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

[David Beaumont]

On Fri, 21 Feb 2025 07:06:38 GMT, Jan Lahoda <jlahoda 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).
>
> 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.

Ahh cool, I was just copying prior precedent.

> 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.

I've seen so many different styles of test runner/setup that I'm just copying what's nearby for now. Is there a public document explaining current best practice around this?

> 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.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965203857
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965206095
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965207209