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

david-beaumont · 2025-02-19T15:46:34Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8350492 to be approved
Commit message must refer to an issue

Warning

⚠️ Found leading lowercase letter in issue title for 8338675: javac shouldn't silently change .jar files on the classpath

Issues

JDK-8338675: javac shouldn't silently change .jar files on the classpath (Bug - P4)
JDK-8350492: javac shouldn't silently change .jar files on the classpath (CSR)

Reviewers

Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23699/head:pull/23699
$ git checkout pull/23699

Update a local copy of the PR:
$ git checkout pull/23699
$ git pull https://git.openjdk.org/jdk.git pull/23699/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23699

View PR using the GUI difftool:
$ git pr show -t 23699

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23699.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-19T15:47:09Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-19T15:47:44Z

@david-beaumont This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 673 new commits pushed to the master branch:

2a31f69: 8353331: Test ForkJoinPool20Test::testFixedDelaySequence is failing
1809138: 8352284: EXTRA_CFLAGS incorrectly applied to BUILD_LIBJVM src/hotspot C++ source files
cef5610: 8353272: One instance of STATIC_LIB_CFLAGS was missed in JDK-8345683
6801eb8: 8352709: Remove bad timing annotations from WhileOpTest.java
85a0baf: 8352719: Add an equals sign to the modules statement
f25f701: 8353226: JFR: emit old object samples must be transitive closure complete for segment
aff5aa7: 8350566: NMT: add size parameter to MemTracker::record_virtual_memory_tag
196334f: 8352046: Test testEcoFriendly() in jdk tools launcher ExecutionEnvironment.java for AIX and Linux/musl is brittle
ad48846: 8350386: Test TestCodeCacheFull.java fails with option -XX:-UseCodeCacheFlushing
8b4e190: 8353349: ProblemList runtime/cds/appcds/SignedJar.java
... and 663 more: https://git.openjdk.org/jdk/compare/7e307916ecbf1ae9795e42e5b5a8347daad4af8c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lahodaj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-02-19T15:48:19Z

@david-beaumont The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-02-20T17:12:29Z

Webrevs

jddarcy · 2025-02-21T05:50:36Z

/csr needed

openjdk · 2025-02-21T05:51:29Z

@jddarcy has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@david-beaumont please create a CSR request for issue JDK-8338675 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

lahodaj

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

lahodaj · 2025-02-21T07:19:24Z

test/langtools/tools/javac/T8338675/NoOverwriteJarClassFilesByDefault.java

+    public static void main(String[] args) throws IOException {
+        ToolBox tb = new ToolBox();
+        tb.createDirectories("lib");
+        tb.writeFile(LIB_SOURCE_NAME, OLD_LIB_SOURCE);


Using Paths 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

david-beaumont · 2025-02-28T15:02:51Z

@lahodaj Is there anything else you think needs to be done for this PR? I think the CSR will be finalized very soon.

lahodaj · 2025-02-28T17:42:46Z

I think it would be useful to have an answer to this CSR question:
https://bugs.openjdk.org/browse/JDK-8350492?focusedId=14755916&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14755916

I suspect best would be to have some tests for the Filer methods.

Please note you'll have to mark the CSR Finalized before it is considered again. Thanks!

jddarcy · 2025-03-06T17:05:25Z

src/jdk.compiler/share/classes/com/sun/tools/javac/file/JavacFileManager.java

-                if (sibling != null && sibling instanceof PathFileObject pathFileObject) {
+                // Use the sibling to determine the output location where possible, unless
+                // it is in a JAR/ZIP file (we don't attempt to write class files back into
+                // archives). See JDK-8338675.


Per usual JDK coding conventions, we don't put bug numbers into source code.

I wasn't aware of this. Thanks.

jddarcy · 2025-03-06T17:11:07Z

test/langtools/tools/javac/modules/AnnotationFilerTest.java

+    }
+
+    @SupportedAnnotationTypes("*")
+    static class TestAnnotationProcessor extends AbstractProcessor {


Please see the test library file

langtools/toosl/javac/lib/JavacTestingAbstractProcessor.java

and consider extending that class instead.

…it between tests).

lahodaj

Looks reasonable to me.

jddarcy · 2025-03-31T20:50:57Z

test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java

+ * written back into the JAR in which its source was found, possibly overwriting
+ * an existing class file entry. This would be very problematic.
+ * <p>
+ * This test ensures javac will not modify JAR files on the classpath, even if


Does something reasonable happen if the soucepath and classpath are both set to the same jar file?

Behaviour is same for either/both. Jar file is never modified, sources appear in the working directory.

jddarcy · 2025-03-31T20:59:13Z

test/langtools/tools/javac/processing/filer/TestNoOverwriteJarFiles.java

+ *
+ * <h2>Important</h2>
+ *
+ * This test creates files from Java compilation and annotation processing, and


I suppose some of the desired effect could be achieved using jtreg directives instead, but this approach seems fine too.

lahodaj

The new tests seems reasonable to me.

david-beaumont · 2025-04-01T12:42:51Z

/integrate

openjdk · 2025-04-01T12:43:32Z

@david-beaumont
Your change (at version 6fcea57) is now ready to be sponsored by a Committer.

lahodaj · 2025-04-03T05:35:24Z

/sponsor

openjdk · 2025-04-03T05:36:34Z

Going to push as commit bd74922.
Since your change was applied there have been 706 commits pushed to the master branch:

8d3d1d4: 8353058: [PPC64] Some IR framework tests are failing after JDK-8352595
b01026a: 8353325: Rewrite appcds/methodHandles test cases to use CDSAppTester
e2e1598: 8353584: [BACKOUT] DaCapo xalan performance with -XX:+UseObjectMonitorTable
814730e: 8352645: Add tool support to check order of includes
d435362: 8353479: jcmd with streaming output breaks intendation
130b0cd: 8353217: Build libsleef on macos-aarch64
209e72d: 8353234: Refactor XMLSecurityPropertyManager
cc870d4: 8352088: Call of com.sun.jdi.ThreadReference.threadGroups() can lock up target VM
d979bd8: 8344671: Few JFR streaming tests fail with application not alive error on MacOS 15
49cb7aa: 8339114: DaCapo xalan performance with -XX:+UseObjectMonitorTable
... and 696 more: https://git.openjdk.org/jdk/compare/7e307916ecbf1ae9795e42e5b5a8347daad4af8c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-03T05:36:48Z

@lahodaj @david-beaumont Pushed as commit bd74922.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

david-beaumont added 3 commits February 18, 2025 13:47

Work in progress.

550bd4a

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

06b6ee5

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

0131a99

openjdk bot added the compiler compiler-dev@openjdk.org label Feb 19, 2025

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

5d47fdf

david-beaumont marked this pull request as ready for review February 20, 2025 17:08

openjdk bot added the rfr Pull request is ready for review label Feb 20, 2025

openjdk bot added the csr Pull request needs approved CSR before integration label Feb 21, 2025

lahodaj reviewed Feb 21, 2025

View reviewed changes

david-beaumont added 2 commits February 21, 2025 12:20

Changes based on PR feedback.

39d24c9

Changes based on PR feedback.

8b91348

Adding a test for the annotation processing Filer.

2fbb486

jddarcy reviewed Mar 6, 2025

View reviewed changes

david-beaumont added 4 commits March 11, 2025 15:38

Updating from feedback.

ab27880

Rewriting test and merging annotation tests based on feedback.

2905c22

Adding sanity check that the test directory is empty (since we clean …

0a3613b

…it between tests).

Remove unused import.

1341c7c

lahodaj approved these changes Mar 26, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Mar 31, 2025

jddarcy reviewed Mar 31, 2025

View reviewed changes

Add final test for sourcepath entry (same behaviour).

6fcea57

openjdk bot removed the ready Pull request is ready to be integrated label Apr 1, 2025

lahodaj approved these changes Apr 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 1, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 1, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 3, 2025

openjdk bot closed this Apr 3, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 3, 2025

david-beaumont deleted the JDK-8338675 branch June 28, 2025 16:44

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

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

Uh oh!

Conversation

david-beaumont commented Feb 19, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warning

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 19, 2025

Uh oh!

openjdk bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 19, 2025

Uh oh!

mlbridge bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jddarcy commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-beaumont commented Feb 28, 2025

Uh oh!

lahodaj commented Feb 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

david-beaumont commented Apr 1, 2025

Uh oh!

openjdk bot commented Apr 1, 2025

Uh oh!

lahodaj commented Apr 3, 2025

Uh oh!

openjdk bot commented Apr 3, 2025

Uh oh!

openjdk bot commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

david-beaumont commented Feb 19, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 19, 2025 •

edited

Loading

mlbridge bot commented Feb 20, 2025 •

edited

Loading