-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8300088: [IMPROVE] OPEN_MAX is no longer the max limit on macOS >= 10.6 for RLIMIT_NOFILE #17361
Conversation
👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into |
@gerard-ziemski The following label will be automatically applied to this pull request:
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. |
Webrevs
|
Hello Gerard,
A note about that - that bug has been fixed in upstream ksh last year ksh93/ksh#591 and appears to have made it in the 1.0.6 release https://github.com/ksh93/ksh/releases/tag/v1.0.6 which was released 6 months back. Of course that doesn't mean all relevant setups/systems will be using this new ksh version and I believe it will take time for that fix to make it into recent releases (In fact, I think Apple has its own fork of ksh). |
Thank you for the note - good to hear that it has been fixed in new ksh, however, the workaround will need to stick for a long while now I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised structure looks good. Just some minor suggestions. Thanks.
Thank you for more feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks
@gerard-ziemski 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:
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 183 new commits pushed to the
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 |
@gerard-ziemski Thank you very much for working on this. How does it work, will this change be backported to JDK 21 & 17 branches or will it be on main only? |
I believe that backport issues will need to be manually created for this purpose if you want them. |
@gerard-ziemski - if you think that this fix is appropriate to backport to JDK21u |
Just to be clear, Calvin works on the backports into the Oracle CPU releases |
Thank you Dan! I will check with Calvin... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nit typos. Otherwise, thumbs up.
Don't forget to check for a copyright year update.
What's the current value of OPEN_MAX
and INT_MAX
on macOS these days?
That you! |
Thank you Dan for the feedback! The current values on macOS Sonoma (14.2.1) appear to be:
|
Does this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thumbs up.
Unfortunately, that's the case currently. |
Can I get final OK after comments fixes please? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general rule for comments is that you either use sentences and so start with a capital and end with a period, or you just have free text. You should avoid starting with a capital but omitting the period; and vice-versa.
Sure thing, since I am in that area, I might as well try to clean it up a bit. Hopefully I got it right. Can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed again. For those comments that already started with a capital
letter, I probably would have chosen to add a period at the end, but that's
a personal preference thing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
Yeah, it was a judgement call on my part, but the logic I used was: if the line fits on a single line, then make it an in-line comment (no capital first letter, no period), otherwise capitalize and use period at the end. |
Thank you very much for the feedback! |
/integrate |
Going to push as commit 3d82363.
Your commit was automatically rebased without conflicts. |
@gerard-ziemski Pushed as commit 3d82363. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
On current macOS (> 10.6) we can use
RLIM_INFINITY
forsetrlimit(RLIMIT_NOFILE)
, even though the man page for setrlimit(2) claims otherwise.The only wrinkle here is that some terminals (ksh) will crash with
RLIM_INFINITY
, because that value overflowsint
type, which they use internally, causing crash, so we work around that by usingINT_MAX
instead ofRLIM_INFINITY
.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17361/head:pull/17361
$ git checkout pull/17361
Update a local copy of the PR:
$ git checkout pull/17361
$ git pull https://git.openjdk.org/jdk.git pull/17361/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17361
View PR using the GUI difftool:
$ git pr show -t 17361
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17361.diff
Webrev
Link to Webrev Comment