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

[David Beaumont]

On Fri, 21 Feb 2025 07:19:24 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 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).

I was under the impression that JTreg already moves into a test-specific scratch directory which is guaranteed empty at test start, so it's fine to write test files from the "current" directory.
Since I'm still learning best-practice around this, if you have docs or other links giving best practice for test file naming, I'd be very grateful.

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

IntelliJ formatting did this. If there was a recommended IJ formatter setup that would would "just work (tm)" I'd love to know about it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965212556
PR Review Comment: https://git.openjdk.org/jdk/pull/23699#discussion_r1965215620