-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8041676: remove the java.compiler system property #13475
Conversation
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
Webrevs
|
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.
Code changes look good. Thanks for cleaning this up.
warning("The java.compiler system property is obsolete and no longer supported," | ||
" use -Xint if you want to run the application in interpreted-only mode."); | ||
} else { | ||
warning("The java.compiler system property is obsolete and no longer supported."); |
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.
This looks okay although the warning for the empty value and "NONE" case will probably wrap as it's very long. Today, -Djava.compiler=foo is ignored, no warning, seems good to emit a warning about that too.
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.
Hello Alan,
This looks okay although the warning for the empty value and "NONE" case will probably wrap as it's very long.
Agreed, it's a bit long. I can't think of a simpler message though. If anyone has a suggestion, I'll update accordingly.
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 drop the detailed explanation, they can check the help for -Xint.
"The java.compiler system property is obsolete and no longer supported, use -Xint".
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.
Thank you Roger, updated the PR with this suggested change.
@jaikiran 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 68 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 |
Thank you everyone for the reviews. The CSR has been approved, I'll go ahead and integrate this. |
/integrate |
Going to push as commit b8f0a66.
Your commit was automatically rebased without conflicts. |
Can I please get a review of this change which removes the
java.compiler
system property? This addresses https://bugs.openjdk.org/browse/JDK-8041676.A CSR has been filed for this change and is available at https://bugs.openjdk.org/browse/JDK-8305998. The CSR has more details about this proposed change, but to summarize, the
java.compiler
system property wasn't used in any practical way. With the recent removal ofjava.lang.Compiler
interface from the JDK, the presence of this system property wasn't of any practical importance.The commit in this PR, removes its reference from the
System.getProperties()
documentation and also removes the specific implementation from hotspot where it treated the valuesNONE
and empty string in the absence of-Xdebug
in a specific manner and considered it to be an instruction to run the application in interpreter-only mode.-Xint
option has been around for this purpose, so removal of thisjava.compiler
system property support from hotspot implementation wouldn't remove any usable feature for the applications.Additionally, the hotspot implementation now logs a warning message when it detects the presence of
java.compiler
system property whenjava
is launched. This warning message will be removed after a few releases.The current JDK source has reference to the
java.compiler
system property in numerous NetBeans project build files. The usage of this system property in such files isn't necessary. However, this PR doesn't intend to cleanup those references, since it isn't clear which of those NetBeans projects are still relevant. I think we can use a separate PR to do that cleanup.tier1, tier2, tier3 testing has been done with these changes and those tests have passed.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13475/head:pull/13475
$ git checkout pull/13475
Update a local copy of the PR:
$ git checkout pull/13475
$ git pull https://git.openjdk.org/jdk.git pull/13475/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13475
View PR using the GUI difftool:
$ git pr show -t 13475
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13475.diff
Webrev
Link to Webrev Comment