jdk
jdk copied to clipboard
8342807: Update links in java.base to use https://
Please review this cleanup PR which updates a total of 12 links to external documentation or references in java.base to use https instead of plain text http.
Links in java.security and share/data/tzdata are excluded from this PR.
This is a documentaton-only cleanup. No tests are added or updated. noreg-cleanup added in the JBS. All updated links have been verified to resolve and to show the expected content.
There are two files here with non-Oracle copyright headers, not quite sure how to update those? (java_md_aix.h and xss-common-qsort.h).
Changes beyond the obvious are as follows:
linux/native/libsimdsort/xss-common-qsort.h:
The current link uses a non-default port number. Changed to using the default port number. Reported upstream as intel/x86-simd-sort#170
share/man/keytool.1:
www.oracle.com redirects this from /technetwork/java/javase/javasecarootcertsprogram-1876540.html to /java/technologies/javase/carootcertsprogram.html. Using the new URL here. Should this be updated in Oracle-internal sources as well?
unix/classes/sun/net/PortConfig.java
The current link no longer resolves. As a replacement, I suggest https://www.ibm.com/support/pages/node/886227 which answers a support question regarding ephemeral ports.
unix/native/libjava/*,
Links redirect from "www.opengroup.org" to "pubs.opengroup.org". Using this new host in the URLs
unix/native/libjava/ProcessImpl_md.c
A link to pasc.org now requires authentication. I changed this to be a Wayback link. Alternantives would be to delete this link, or to track down a revised version of the spec the linked Interpretation Request refers to.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8342807: Update links in java.base to use https:// (Enhancement - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21633/head:pull/21633
$ git checkout pull/21633
Update a local copy of the PR:
$ git checkout pull/21633
$ git pull https://git.openjdk.org/jdk.git pull/21633/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21633
View PR using the GUI difftool:
$ git pr show -t 21633
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21633.diff
Webrev
:wave: Welcome back eirbjo! 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.
@eirbjo 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:
8342807: Update links in java.base to use https://
Reviewed-by: rriggs, ihse, jkern
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 112 new commits pushed to the master branch:
- 175e58b2e321b779276a9a98a5e72cedb9638d0c: 8332980: [IR Framework] Add option to measure IR rule processing time
- b8c68c0e8c9aee43378fe16349c083cb868447f4: 8348207: Linux PPC64 PCH build broken after JDK-8347909
- 70eec9001a550888f35476f9e2cf3c62d41442dd: 8338303: Linux ppc64le with toolchain clang - detection failure in early JVM startup
- a1fd5f4e88f52125eef4feea91a60641981177c1: 8348554: Enhance Linux kernel version checks
- 002679ac9fe4de8150b7dd4c9aeb44eeef1257d6: 8347065: Add missing @spec tags for Java Security Standard Algorithm Names
- 99002e4f9d421d08d912187a1f01809d85820427: 8318098: Update jfr tests to replace keyword jfr with vm.flagless
- 5431668cb92a8ef2ccfe1059db1cde0e5d98adce: 8348212: Need to add warn() step to JavacTaskImpl after JDK-8344148
- 1d2eb2fbaea700fc77b644b5eb5a8a7c40ede108: 8299504: Resolve
usesandprovidesat run time if the service is optional and missing - f446cefee0715da6532b68f65a5a15775e20945d: 8343962: [REDO] Move getChars to DecimalDigits
- 7c0985fc32ec5419f7b409248385c5ca80f1093f: 8348420: Shenandoah: Check is_reserved before using ReservedSpace instances
- ... and 102 more: https://git.openjdk.org/jdk/compare/6cc1c0abdbf8cd3d01722951cf34ebcb667f1380...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.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
@eirbjo The following labels will be automatically applied to this pull request:
core-libsnet
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.
Webrevs
- 04: Full - Incremental (d1ec70bb)
- 03: Full - Incremental (7174f839)
- 02: Full (0f5bf950)
- 01: Full - Incremental (f2580d87)
- 00: Full (0c453f51)
I think the interesting links are the ones being changed in unix/native/libjava/ProcessImpl_md.c and unix/classes/sun/net/PortConfig.java since the current ones no longer resolve. Since those will potentially require new links (with different content), I believe the change will require a CSR then.
FWIW: I don't believe a change to a link in a comment in an internal class requires a CSR. For PortConfig.java - it would be good to have someone involved in the AIX port comment on the proposed changes. Removing the obsolete link altogether is also a possibility.
For PortConfig.java - it would be good to have someone involved in the AIX port comment on the proposed changes. Removing the obsolete link altogether is also a possibility.
The context for the link in PortConfig.java is:
// The ephemeral port is OS version dependent on AIX:
// http://publib.boulder.ibm.com/infocenter/aix/v7r1/topic/com.ibm.aix.rsct315.admin/bl503_ephport.htm
// However, on AIX 5.3 / 6.1 / 7.1 we always see the
// settings below by using:
// /usr/sbin/no -a | fgrep ephemeral
defaultLower = 32768;
defaultUpper = 65535;
So while the FAQ link I changed this to confirms the port numbers here, it probably makes the comment "The ephemeral port is OS version dependent on AIX" somewhat stale. Seems the original linked document somehow contradicted the observed settings? So if we use my suggested FAQ link, perhaps it is better to remove the surrounding comment altogether, as it seems stale.
For ProcessImpl_md.c, the obsolete link was to a request for clarification to what I think may be an early version of some POSIX standard. If I'm right, the current version is https://www.iso.org/standard/50516.html, which is not publicly available (requires paid access).
But yes, would be nice if some AIX people could have a look at this.
@eirbjo 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!
@eirbjo 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 links-use-https
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
@eirbjo 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.
@eirbjo This seemed like a valuable and worthwhile contribution, and I think it is a bit sad to see it closed due to inactivity. Was it the lack of volunteering reviewers that made you let it go?
/open
@eirbjo This pull request is now open
@eirbjo This seemed like a valuable and worthwhile contribution, and I think it is a bit sad to see it closed due to inactivity. Was it the lack of volunteering reviewers that made you let it go?
Thanks for looking at this Magnus, I agree cleaning this up would be good.
Although most of the link updates are trivial and safe, I think we just got stuck on the two links which were not completely clear how to update:
PortConfig.java:
This file includes a link to some legacy IBM documentation on ephemral ports on AIX which is no longer accessible. I found an IBM FAQ page which mentions ephemral port ranges and suggest this as the replacement. @dfuch suggested:"it would be good to have someone involved in the AIX port comment on the proposed changes", however this never happened. I'm not sure who to ask.
We can make progress here by getting someone with AIX experience to review, or we can simply remove the link. I personally think the replacement link I found is "good enough", although I've never actually seen the current link contents.
ProcessImpl_md.c:
This file includes a link to www.pasc.org for an unofficial interpretation of an early POSIX standard. This document now requires authentication. The PR currently links to a Wayback page, but maybe we should should rather remove this link altogether since it was always considered "unofficial" and the interpretation in question has long been baked into the standard.
So based on the above, my "call to action" for this PR in the current state is:
a) Find out who we can invite with AIX experience to look at the updates to PortConfig.java
b) If that fails, I'll remove the link and ask for a review of that
c) I would like feedback on the proposal to remove the link to the www.pasc.org "unofficial" interpretation document in ProcessImpl_mc.c
Thanks!
@JoKern65 Can you have a look at the AIX link?
For the changes in ProcessImpl_md.c, I think we'll want to have the input of @RogerRiggs.
I cannot find a better page for ephemeral ports on AIX. For me the new link is OK. The problem is, that such IBM links do not work very long. In one or a view years this link again might point to nirvana.
The problem is, that such IBM links do not work very long. In one or a view years this link again might point to nirvana.
Should we remove it instead then?
The problem is, that such IBM links do not work very long. In one or a view years this link again might point to nirvana.
Should we remove it instead then?
I would be OK with just dropping the link.
In ProcessImpl_md.c, we have adopted the Posix APIs and semantics for process handling. I suggest removing the "unofficial" link, it does not include useful information at this point and is fragile.
The current behavior always setting SIGCHLD to SIG_DFL is/has been reliable and does not depend on the behavior of the parent process. (Good) @tstuefe May also be interested in reviewing this change.
The previous sentence mentions Solaris and that should be removed at some point but out of scope for this PR.
In ProcessImpl_md.c, we have adopted the Posix APIs and semantics for process handling. I suggest removing the "unofficial" link, it does not include useful information at this point and is fragile.
The current behavior always setting SIGCHLD to SIG_DFL is/has been reliable and does not depend on the behavior of the parent process. (Good) @tstuefe May also be interested in reviewing this change.
https://pubs.opengroup.org/onlinepubs/9799919799/ seems to indicate that the "default action is to ignore the signal", which is unclear to me. I think it means that the signal will not cause invocation of a user handler or abort the process, but does this also mean that wait and waited won't work?
In any case, I agree with @RogerRiggs that keeping this behavior is fine.
The previous sentence mentions Solaris and that should be removed at some point but out of scope for this PR.
@tstuefe And you also agree that the link should be removed? (Which is what this PR is about)
The problem is, that such IBM links do not work very long. In one or a view years this link again might point to nirvana.
Should we remove it instead then?
I would be OK with just dropping the link.
I would keep the link, because currently it is good, but be aware to fix it again in one of the next jdk versions. Maybe you can copy the essentials of the IBM page regarding ephemeral ports as comment to PortConfig.java with the link as citation mark.
@tstuefe And you also agree that the link should be removed? (Which is what this PR is about)
The 20+ years old link to the PASC error report I would probably remove; it is likely outdated now. If one wanted to invest the time, one could test the current behavior on our supported Unices. As long as that is not done, we can live with setting SIG_DFL manually.
I would replace it with a link to https://pubs.opengroup.org/onlinepubs/007904875/functions/wait.html and point to the section that says:
[XSI] [Option Start] If the calling process has SA_NOCLDWAIT set or has SIGCHLD set to SIG_IGN,
and the process has no unwaited-for children that were transformed into zombie processes,
the calling thread shall block until all of the children of the process containing
the calling thread terminate, and wait() and waitpid() shall fail and
set errno to [ECHILD]. [Option End]
Again, that is XSI though; I am not sure which of our Unices comply to that. Linux does only partially, so one would have to test that behavior.
Another question @RogerRiggs , I was surprised to see that we set the SIGCHLD default behavior only at Java_java_lang_ProcessImpl_init. Should we also set it in jspawnhelper before the exec? But I guess the assumption is that no java process will fork before Java_java_lang_ProcessImpl_init, so maybe its okay.
@tstuefe And you also agree that the link should be removed? (Which is what this PR is about)
The 20+ years old link to the PASC error report I would probably remove; it is likely outdated now. If one wanted to invest the time, one could test the current behavior on our supported Unices. As long as that is not done, we can live with setting SIG_DFL manually.
Thanks for looking into this Thomas, much appreciated!
I think the improvements you suggest here are perhaps streching the scope of this particular PR, which was mostly to update links to use HTTPS. I must also admit that I have a very limited understanding of these low-level Unix APIs, so I'm reluctant to include material changes in this PR.
I have filed https://bugs.openjdk.org/browse/JDK-8348183 to track a review/update of the comment in ProcessImpl_md.c and suggest that we simply remove the "unofficial" link for this PR.
Would this be ok with you?
The current implementation reaps exit status of subprocesses using wait() in a dedicated OS thread. From time to time, we conjecture that using a signal handler to capture the exit status might be preferred. However, with Hotspot handling signals and other native code might also want to handle signals, there is less risk using the current mechanism to get the exit status. So for the purposes of reaping status, setting the handler to SIG_DFL to ignore the signals is fine as is. I agree doing (in this PR) anything more than fix the link is out of scope.
I agree doing (in this PR) anything more than fix the link is out of scope.
Good. My understanding is that we are converging on a consensus to:
- Change the broken IBM AIX emprempral port link to the suggested IBM FAQ one in
PortConfig.java - Remove the "unofficial" pasc.org link from
ProcessImpl_md.c - Handle any other changes rooted in the discussion about signals and child processes under https://bugs.openjdk.org/browse/JDK-8348183
If reviewers agree to this, I would welcome a final review of the changes in this PR based on the above.
@tstuefe And you also agree that the link should be removed? (Which is what this PR is about)
The 20+ years old link to the PASC error report I would probably remove; it is likely outdated now. If one wanted to invest the time, one could test the current behavior on our supported Unices. As long as that is not done, we can live with setting SIG_DFL manually.
Thanks for looking into this Thomas, much appreciated!
I think the improvements you suggest here are perhaps streching the scope of this particular PR, which was mostly to update links to use HTTPS. I must also admit that I have a very limited understanding of these low-level Unix APIs, so I'm reluctant to include material changes in this PR.
I have filed https://bugs.openjdk.org/browse/JDK-8348183 to track a review/update of the comment in ProcessImpl_md.c and suggest that we simply remove the "unofficial" link for this PR.
Would this be ok with you?
Yes thank you!
/integrate
Going to push as commit ffeb9b5aff6b91297b4bbedb7b33670dc17309ed.
Since your change was applied there have been 113 commits pushed to the master branch:
- afcc2b03afc77f730300e1d92471466d56ed75fb: 8348562: ZGC: segmentation fault due to missing node type check in barrier elision analysis
- 175e58b2e321b779276a9a98a5e72cedb9638d0c: 8332980: [IR Framework] Add option to measure IR rule processing time
- b8c68c0e8c9aee43378fe16349c083cb868447f4: 8348207: Linux PPC64 PCH build broken after JDK-8347909
- 70eec9001a550888f35476f9e2cf3c62d41442dd: 8338303: Linux ppc64le with toolchain clang - detection failure in early JVM startup
- a1fd5f4e88f52125eef4feea91a60641981177c1: 8348554: Enhance Linux kernel version checks
- 002679ac9fe4de8150b7dd4c9aeb44eeef1257d6: 8347065: Add missing @spec tags for Java Security Standard Algorithm Names
- 99002e4f9d421d08d912187a1f01809d85820427: 8318098: Update jfr tests to replace keyword jfr with vm.flagless
- 5431668cb92a8ef2ccfe1059db1cde0e5d98adce: 8348212: Need to add warn() step to JavacTaskImpl after JDK-8344148
- 1d2eb2fbaea700fc77b644b5eb5a8a7c40ede108: 8299504: Resolve
usesandprovidesat run time if the service is optional and missing - f446cefee0715da6532b68f65a5a15775e20945d: 8343962: [REDO] Move getChars to DecimalDigits
- ... and 103 more: https://git.openjdk.org/jdk/compare/6cc1c0abdbf8cd3d01722951cf34ebcb667f1380...master
Your commit was automatically rebased without conflicts.