8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava

[bplb]

This proposed change would move the native objects required for NIO file interaction from the libnio native library to the libjava native library on Linux, macOS, and Windows.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (3 reviews required, with at least 1 Reviewer, 2 Authors)

Issue

• JDK-8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava (Sub-task - P3)

Reviewers

• Daniel Fuchs (@dfuch - Reviewer) 🔄 Re-review required (review applies to f57b6f13)
• Magnus Ihse Bursie (@magicus - Reviewer) 🔄 Re-review required (review applies to b54b1683)
• Daniel Jeliński (@djelinski - Reviewer) 🔄 Re-review required (review applies to b54b1683)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20317

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back bpb! 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]

@bplb This change is no longer ready for integration - check the PR body for details.

[openjdk]

@bplb The following labels will be automatically applied to this pull request:

• build
• core-libs
• net
• nio
• security

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

[bplb]

This change passes CI tiers 1-5 jobs on all platforms. With the change in place, one can remove libnio from the JDK and still be able to copy a file using FileChannel. Without the change, doing this will result in throwing a java.lang.UnsatisfiedLinkError.

[mlbridge]

Webrevs

• 13: Full (95ce13fb)
• 12: Full (da21fa69)
• 11: Full (3e1e15dc)
• 10: Full (2cb82267)
• 09: Full (8176e89c)
• 08: Full (403602f0)
• 07: Full - Incremental (b54b1683)
• 06: Full - Incremental (853d3491)
• 05: Full (4f47d5a6)
• 04: Full - Incremental (f57b6f13)
• 03: Full - Incremental (e28a4f57)
• 02: Full - Incremental (7e8a02e3)
• 01: Full - Incremental (48519737)
• 00: Full (744fa359)

[AlanBateman]

/label remove security

[openjdk]

@AlanBateman
The security label was successfully removed.

[AlanBateman]

I think this will require thinking about how to organize the native code in native/libjava as it hard to maintain if everything is in the same directory. We may have to create subdirectories, as we do in native/libnio.

Also think to work through some naming on IOUtil vs. NIOUtil as it won't be obvious to maintainers which class to use.

[bplb]

Yes, I was wondering about changing the organization of the native code files. It's not great to have them all in <platform>/native/libjava.

[bplb]

Perhaps <platform>/native/libjava/nio/ch and <platform>/native/libjava/nio/fs?

[bplb]

Also think to work through some naming on IOUtil vs. NIOUtil as it won't be obvious to maintainers which class to use.

Maybe NIOUtil should be NetUtil as its methods appear to be invoked only by classes involved in networking?

[bplb]

As part of 7e8a02e, the nio_util.h header files were modified. One unused symbolic constant was removed. Symbolic constants used in only one file were moved to that file. Function declarations that were only used where the function is defined were removed. Function declarations used in only one file other than the one where the function is defined were moved to an extern in that file. On Unix, one function declaration used in three files was moved to a new header file Net.h.

[dfuch]

Disclaimer: I have not tried to understand the proposed change in details. However I have spotted a small oddity.

[bplb]

Also think to work through some naming on IOUtil vs. NIOUtil as it won't be obvious to maintainers which class to use.

Maybe NIOUtil should be NetUtil as its methods appear to be invoked only by classes involved in networking?

Another option is NIONetUtil but the NIO prefix is redundant with the package name.

[dfuch]

The changes to the java/net code look OK to me now. Thanks Brian. I am approving these changes - but please also get a Reviewer for the NIO and build side of these.

[magicus]

Build changes still look good.

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bplb]

continue;

[bplb]

The merge commit 403602f built and passed jdk_core tests on Linux, macOS, and Windows.

[openjdk]

@bplb this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout libjava
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[magicus]

@AlanBateman I think what is holding this PR back is that you said you wanted to review it.

[AlanBateman]

@AlanBateman I think what is holding this PR back is that you said you wanted to review it.

I asked Brian to hold off integrating this as I need time to work through several issues. Note that this is just one part of overall solution, we don't know yet if the changes in this PR will help the startup issue that motivated it (in isolation, the changes in this PR don't make sense of course).

[magicus]

I see.

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bplb]

continue;

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bplb]

continue;

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@bplb This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[bplb]

/open

[openjdk]

@bplb This pull request is now open

[bridgekeeper]

@bplb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!