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

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

[x] Change must not contain extraneous whitespace
[x] 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

Jul 24 '24 19:07 bplb

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

Jul 24 '24 19:07 bridgekeeper[bot]

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

Jul 24 '24 19:07 openjdk[bot]

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

Jul 24 '24 19:07 openjdk[bot]

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.

Jul 24 '24 19:07 bplb

Webrevs

Jul 24 '24 19:07 mlbridge[bot]

/label remove security

Jul 25 '24 05:07 AlanBateman

@AlanBateman The security label was successfully removed.

Jul 25 '24 05:07 openjdk[bot]

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.

Jul 25 '24 10:07 AlanBateman

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.

Jul 25 '24 15:07 bplb

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

Jul 25 '24 16:07 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?

Jul 26 '24 21:07 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.

Aug 07 '24 15:08 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.

Aug 09 '24 21:08 bplb

/reviewers 2

Aug 12 '24 15:08 dfuch

@dfuch The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Aug 12 '24 15:08 openjdk[bot]

I plan to review this, please do not integrate this change until I get time to make sure that the placement and naming is workable in these areas.

Aug 12 '24 15:08 AlanBateman

please do not integrate this change until I get time

Sure, of course I will not.

Thanks to @magicus and @dfuch for helping to make it better.

Aug 12 '24 17:08 bplb

/reviewers 3

Aug 12 '24 20:08 bplb

@bplb The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

Aug 12 '24 20:08 openjdk[bot]

@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!

Sep 09 '24 21:09 bridgekeeper[bot]

continue;

Sep 09 '24 21:09 bplb

Thanks for making the changes. LGTM, assuming that tests still pass.

The tests passed the JDK tiers 1-3 tests on Linux, macOS, and Windows. In any case, I will run another round of tests before integrating.

Sep 12 '24 15:09 bplb

The tests passed the JDK tiers 1-3 tests on Linux, macOS, and Windows. In any case, I will run another round of tests before integrating.

Can you hold off integrating, I plan to go over the changes soon.

Sep 12 '24 15:09 AlanBateman

Can you hold off integrating, I plan to go over the changes soon.

I don't plan to integrate until you have reviewed it. I set the minimum reviewer count to 3 to avoid doing so accidentally.

Sep 12 '24 15:09 bplb

@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!

Nov 05 '24 16:11 bridgekeeper[bot]

continue;

Nov 05 '24 17:11 bplb

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

Nov 20 '24 02:11 bplb

@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

Nov 27 '24 21:11 openjdk[bot]

@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!

Dec 18 '24 07:12 bridgekeeper[bot]

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

Jan 13 '25 12:01 magicus