Skip to content
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

8283620: System.out does not use the encoding/charset specified in the Javadoc #8270

Closed
wants to merge 3 commits into from

Conversation

naotoj
Copy link
Member

@naotoj naotoj commented Apr 15, 2022

Promoting the internal system properties for System.out and System.err so that users can override the encoding used for those streams to UTF-8, aligning to the Charset.defaultCharset(). A CSR has also been drafted.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8283620: System.out does not use the encoding/charset specified in the Javadoc
  • JDK-8284778: System.out does not use the encoding/charset specified in the Javadoc (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8270/head:pull/8270
$ git checkout pull/8270

Update a local copy of the PR:
$ git checkout pull/8270
$ git pull https://git.openjdk.java.net/jdk pull/8270/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8270

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8270.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 15, 2022

👋 Welcome back naoto! 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
Copy link

openjdk bot commented Apr 15, 2022

@naotoj The following label will be automatically applied to this pull request:

  • core-libs

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.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration labels Apr 15, 2022
@naotoj naotoj force-pushed the JDK-8283620 branch 2 times, most recently from 23c010a to 96b1f79 Compare April 21, 2022 21:53
@naotoj naotoj marked this pull request as ready for review April 21, 2022 23:17
@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 21, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 21, 2022

Webrevs

* <td>Character encoding name for {@link System#err System.err}.
* The property may be set on the command line to the value
* {@code UTF-8}. Setting the property to a value other than {@code UTF-8}
* leads to unspecified behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proposal to introduce two standard properties is good and is consistent with the recently introduced native.encoding. I'm not 100% sure that the sentence "The property may be set on the command line ..." is appropriate for the spec of standard properties. We got away with that for file.encoding in implNote but that isn't spec. I think we may have to replace this with something that says that the Java runtime can be started with the system property set to "UTF-8", starting it with the property set to another value clears to undefined behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Alan. Modified them as suggested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Alan has a typo: "clears" -> "leads"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Alan has a typo: "clears" -> "leads"

Apologies, there was a typo in my comment on the PR. I didn't mean for the word "clears" to go into the spec.

* <tr><th scope="row">{@systemProperty stdout.encoding}</th>
* <td>Character encoding name for {@link System#out System.out}.
* The Java runtime can be started with the system property set to {@code UTF-8},
* starting it with the property set to another value clears to undefined behavior.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leads to undefined behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this looks good, assuming the typo in the spec ("clears" -> "leads") is fixed.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 25, 2022
@openjdk
Copy link

openjdk bot commented Apr 25, 2022

@naotoj 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:

8283620: System.out does not use the encoding/charset specified in the Javadoc

Reviewed-by: alanb

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 88 new commits pushed to the master branch:

  • d435d69: 8285614: Fix typo in java.lang.Float
  • 3416bfa: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297
  • 80a7f7b: 8267690: Revisit (Doc)Tree search implemented by throwing an exception
  • 9b82708: 8284319: Test runtime/cds/appcds/TestParallelGCWithCDS.java fails in repo-loom
  • fb60594: 8285477: Add a PRECISION public static field to j.l.Float and j.l.Double
  • 1e79ded: 8284889: runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java#custom-cl-zgc timed out
  • 414918d: 8285389: EdDSA trimming zeros
  • 293bc5e: 8129778: Few awt test fail for Solaris 11 with RuntimeException
  • 36f2e52: 8225777: java/awt/Mixing/MixingOnDialog.java fails on Ubuntu
  • 32593df: 8279888: Local variable independently used by multiple loops can interfere with loop optimizations
  • ... and 78 more: https://git.openjdk.java.net/jdk/compare/13fb1eed52f1a9152242119969a9d4a0c0627513...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 25, 2022
@naotoj
Copy link
Member Author

naotoj commented Apr 26, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Apr 26, 2022

Going to push as commit 03bcf7b.
Since your change was applied there have been 99 commits pushed to the master branch:

  • 20a132d: 8284994: -Xdoclint:all returns warning for records, even when documented properly
  • a3b7881: 8284930: Re-examine FilterInputStream mark/reset
  • 97a0a29: 8283643: [AIX, testbug] MachCodeFramesInErrorFile test fails to find 'Native frames' text
  • 67755ed: 8284890: Support for Do not fragment IP socket options
  • a7b5157: 8282541: AArch64: Auto-vectorize Math.round API
  • 8de3c65: 8284951: Compile::flatten_alias_type asserts with "indeterminate pointers come only from unsafe ops"
  • 552e1b0: 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper
  • e333cd3: 8285611: Retrofit (Doc)Pretty with java.io.UncheckedIOException
  • 9478696: 8283441: C2: segmentation fault in ciMethodBlocks::make_block_at(int)
  • 00e9c96: 8285398: Cache the results of constraint checks
  • ... and 89 more: https://git.openjdk.java.net/jdk/compare/13fb1eed52f1a9152242119969a9d4a0c0627513...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Apr 26, 2022
@openjdk openjdk bot closed this Apr 26, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 26, 2022
@openjdk
Copy link

openjdk bot commented Apr 26, 2022

@naotoj Pushed as commit 03bcf7b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants