8245956: JavaCompiler still uses File API instead of Path API in a specific case #1553

lgxbslgx · 2020-12-02T05:10:12Z

Hi all,

JavacFileManager.getClassLoader uses getLocation which causes exception in some situations.
This patch uses getLocationAsPaths instead of getLocation to solve it.
Thanks you for taking the time to review.

Best Regards.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8245956: JavaCompiler still uses File API instead of Path API in a specific case

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1553/head:pull/1553
$ git checkout pull/1553

…pecific case

bridgekeeper · 2020-12-02T05:11:13Z

👋 Welcome back lgxbslgx! 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 · 2020-12-02T05:11:59Z

@lgxbslgx 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 · 2020-12-02T05:15:10Z

Webrevs

01: Full - Incremental (f4d82b4)
00: Full (d9e8700)

jonathan-gibbons

Nice.

Some minor suggestions inline for your consideration.

jonathan-gibbons · 2020-12-07T19:38:40Z

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

@@ -741,13 +741,13 @@ public void close() throws IOException {
    @Override @DefinedBy(Api.COMPILER)
    public ClassLoader getClassLoader(Location location) {
        checkNotModuleOrientedLocation(location);
-        Iterable<? extends File> path = getLocation(location);
-        if (path == null)
+        Collection<? extends Path> paths = getLocationAsPaths(location);


Minor suggestion: the use of the simple name path or paths can be confusing in contexts like this, when the name may refer to an instance of java.nio.file.Path or a collection of such items.

You did good by renaming the variable from path to paths. A more explicit name would be to use searchPath as the name for a series of file system objects.

jonathan-gibbons · 2020-12-07T19:39:33Z

test/langtools/tools/javac/T8245956/T8245956.java

+/*
+ * @test
+ * @bug 8245956
+ * @summary JavaCompiler still uses files API instead of Path API in a specific case


I suggest changing files API to File API.
I have made the corresponding change in the JBS issue.

jonathan-gibbons · 2020-12-07T19:43:12Z

test/langtools/tools/javac/T8245956/T8245956.java

+                List<Path> classPath = new ArrayList<>();
+                classPath.add(fsPath);
+                fileManager.setLocationFromPaths(StandardLocation.CLASS_PATH, classPath);
+                fileManager.getClassLoader(StandardLocation.CLASS_PATH);  // Should not generate any exceptions.


Minor suggestion: add something like a print statement after the getClassLoader call to verify that the getClassLoader call terminated normally.

openjdk · 2020-12-08T00:49:53Z

@lgxbslgx 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:

8245956: JavaCompiler still uses File API instead of Path API in a specific case

Reviewed-by: jjg

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 199 new commits pushed to the master branch:

fd5f6e2: 8257986: [JVMCI] ProblemList 2 reprofile JVMCI tests
30de320: 6882207: Convert javap to use diamond operator internally
d33a689: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
e6b4c4d: 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways
b977a7b: 8257989: Error in gtest os_page_size_for_region_unaligned after 8257588
5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
bd22aa5: 8229862: NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:570)
cf62b0a: 8257518: LogCompilation: java.lang.InternalError with JFR turned on
6c69eca: 8257973: UTIL_LOOKUP_PROGS should only find executable files
... and 189 more: https://git.openjdk.java.net/jdk/compare/c0719605e79f7e91f528dd197dcb953f4ed99169...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 (@jonathan-gibbons) 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).

… getClassLoader.

lgxbslgx · 2020-12-08T01:40:49Z

/integrate

openjdk · 2020-12-08T01:41:28Z

@lgxbslgx
Your change (at version f4d82b4) is now ready to be sponsored by a Committer.

lgxbslgx · 2020-12-08T13:04:00Z

@jonathan-gibbons I revise the code according to your suggestions. Thank you for taking the time to review.

jonathan-gibbons · 2020-12-09T21:08:57Z

/sponsor

openjdk · 2020-12-09T21:09:39Z

@jonathan-gibbons @lgxbslgx Since your change was applied there have been 200 commits pushed to the master branch:

baf4c1a: 8257917: [JVMCI] crash when materializing boxed values under -Xcomp
fd5f6e2: 8257986: [JVMCI] ProblemList 2 reprofile JVMCI tests
30de320: 6882207: Convert javap to use diamond operator internally
d33a689: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
e6b4c4d: 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways
b977a7b: 8257989: Error in gtest os_page_size_for_region_unaligned after 8257588
5f03341: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
6dd06ad: 8254996: make jdk.net.UnixDomainPrincipal a record class
bd22aa5: 8229862: NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:570)
cf62b0a: 8257518: LogCompilation: java.lang.InternalError with JFR turned on
... and 190 more: https://git.openjdk.java.net/jdk/compare/c0719605e79f7e91f528dd197dcb953f4ed99169...master

Your commit was automatically rebased without conflicts.

Pushed as commit 46c9a86.

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

8245956: JavaCompiler still uses files API instead of Path API in a s…

d9e8700

…pecific case

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2020

openjdk bot added the compiler compiler-dev@openjdk.org label Dec 2, 2020

jonathan-gibbons approved these changes Dec 7, 2020

View reviewed changes

lgxbslgx changed the title ~~8245956: JavaCompiler still uses files API instead of Path API in a specific case~~ 8245956: JavaCompiler still uses file API instead of Path API in a specific case Dec 8, 2020

openjdk bot added the ready Pull request is ready to be integrated label Dec 8, 2020

lgxbslgx changed the title ~~8245956: JavaCompiler still uses file API instead of Path API in a specific case~~ 8245956: JavaCompiler still uses File API instead of Path API in a specific case Dec 8, 2020

Rename variable. Revise comment. Add print statement after the method…

f4d82b4

… getClassLoader.

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2020

jonathan-gibbons approved these changes Dec 9, 2020

View reviewed changes

openjdk bot closed this Dec 9, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 9, 2020

lgxbslgx deleted the JDK-8245956 branch December 13, 2020 14:08

8245956: JavaCompiler still uses File API instead of Path API in a specific case #1553

8245956: JavaCompiler still uses File API instead of Path API in a specific case #1553

lgxbslgx commented Dec 2, 2020 •

edited by openjdk bot

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020 •

edited

jonathan-gibbons left a comment

jonathan-gibbons Dec 7, 2020

jonathan-gibbons Dec 7, 2020

jonathan-gibbons Dec 7, 2020

openjdk bot commented Dec 8, 2020 •

edited

lgxbslgx commented Dec 8, 2020

openjdk bot commented Dec 8, 2020

lgxbslgx commented Dec 8, 2020

jonathan-gibbons commented Dec 9, 2020

openjdk bot commented Dec 9, 2020

8245956: JavaCompiler still uses File API instead of Path API in a specific case #1553

8245956: JavaCompiler still uses File API instead of Path API in a specific case #1553

Conversation

lgxbslgx commented Dec 2, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020 • edited

Webrevs

jonathan-gibbons left a comment

Choose a reason for hiding this comment

jonathan-gibbons Dec 7, 2020

Choose a reason for hiding this comment

jonathan-gibbons Dec 7, 2020

Choose a reason for hiding this comment

jonathan-gibbons Dec 7, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 8, 2020 • edited

lgxbslgx commented Dec 8, 2020

openjdk bot commented Dec 8, 2020

lgxbslgx commented Dec 8, 2020

jonathan-gibbons commented Dec 9, 2020

openjdk bot commented Dec 9, 2020

lgxbslgx commented Dec 2, 2020 •

edited by openjdk bot

mlbridge bot commented Dec 2, 2020 •

edited

openjdk bot commented Dec 8, 2020 •

edited