8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows #16235

yukikimmura · 2023-10-18T07:34:19Z

I would like to fix this issue
because the test dose not work as intended.
Could someone please review it?

Thanks,
Kimura Yukihiro

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows (Bug - P4)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer)
Kevin Walls (@kevinjwalls - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16235/head:pull/16235
$ git checkout pull/16235

Update a local copy of the PR:
$ git checkout pull/16235
$ git pull https://git.openjdk.org/jdk.git pull/16235/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16235

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16235.diff

Webrev

Link to Webrev Comment

…thTest.sh fails on Japanese Windows

bridgekeeper · 2023-10-18T07:35:13Z

👋 Welcome back kyukihiro! 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 · 2023-10-18T07:37:11Z

@yukikimmura The following labels will be automatically applied to this pull request:

core-libs
serviceability

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.

mlbridge · 2023-10-18T08:15:24Z

Webrevs

01: Full - Incremental (03a3d01b)
00: Full (25f09bf7)

AlanBateman · 2023-10-18T08:46:17Z

/label remove core-libs

openjdk · 2023-10-18T08:47:01Z

@AlanBateman
The core-libs label was successfully removed.

sspitsyn · 2023-10-19T04:43:06Z

test/jdk/java/lang/instrument/BootClassPath/Setup.java

@@ -45,6 +45,10 @@ public static void main(String[] args) throws Exception {
        }
        String workDir = args[0];
        String premainClass = args[1];
+        boolean isCygwin = false;
+        if (args.length==3 && args[2].equals("CYGWIN")) {


Nit: Could you, please, add spaces around ==?

Thank you for your advice.
I fixed it as you mentioned.

Thanks,
Kimura Yukihiro

sspitsyn

Hi Kimura,

The fix looks okay to me. Thank you for taking care about it.
I'm not aware much about all these encoding names and their correctness.
How did you test all these languages?

Thanks,
Serguei

…thTest.sh fails on Japanese Windows

yukikimmura · 2023-10-19T07:56:35Z

Hi Serguei,

Thank you for your comment.

I tested these encoding name using LANG, LC_ALL and LC_CTYPE environment variables on Linux.
For instance, when I tested "ETC_TW", I set LANG and LC_CTYPE to zh_TW.euctw which I got with "locale -a" command.
I took these encoding names from
GetJavaProperties() and ParseLocale() in java.base/unix/native/libjava/java_props_md.c.

And I tested only English and Japanese Windows because I could not make all environments.
I think these names are correct because I took them from
GetJavaProperties() and getEncodingInternal() in java.base/windows/native/libjava/java_props_md.c.

Thanks,
Kimura Yukihiro

sspitsyn

Thank you for the update and reply!
Looks good.
Thanks,
Serguei

yukikimmura · 2023-10-23T00:00:47Z

Hi Serguei,

Thank you for your quick reply.
Could you please sponsor the fix?

Thanks,
Kimura Yukihiro

sspitsyn · 2023-10-28T00:08:21Z

/sponsor

openjdk · 2023-10-28T00:09:05Z

@sspitsyn The change author (@yukikimmura) must issue an integrate command before the integration can be sponsored.

sspitsyn · 2023-10-28T00:09:58Z

Hi Serguei,

Thank you for your quick reply. Could you please sponsor the fix?

Thanks, Kimura Yukihiro

I was hoping to have one more reviewer of your fix.
But it seems there is nobody else to review it.
Sponsored now.

yukikimmura · 2023-10-30T09:28:03Z

This project has not yet received a notification from the bot that I can type the integrate command.
Should I adde a noreg-self label on JBS, or should I find one more reviewer?

Thanks,
Kimura Yukihiro

sspitsyn · 2023-10-30T09:42:39Z

This project has not yet received a notification from the bot that I can type the integrate command. Should I adde a noreg-self label on JBS, or should I find one more reviewer?

Thanks, Kimura Yukihiro

You can see this message from bot:

@sspitsyn The change author (@yukikimmura) must issue an integrate command before the integration can be sponsored.

So, please, go ahead and issue the command /integrate .
Then I'll sponsor this fix again.

sspitsyn · 2023-10-30T09:43:23Z

I've added the label noreg-self.

yukikimmura · 2023-10-30T09:59:09Z

/integrate

openjdk · 2023-10-30T10:00:38Z

@yukikimmura This pull request has not yet been marked as ready for integration.

kevinjwalls · 2023-10-30T12:19:15Z

There is an "integration blocker" note above, the title of this PR needs updating:
Title mismatch between PR and JBS for issue JDK-8318410

openjdk · 2023-10-30T23:19:35Z

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

8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows

Reviewed-by: sspitsyn, kevinw

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

1ca2cfa: 8310933: Copying from runtime image to application image should not follow symlinks
d3534b0: 8318677: (ch) Add implNote about minBufferCap to main variant of Channels.newWriter
864a876: 8313621: test/jdk/jdk/internal/math/FloatingDecimal/TestFloatingDecimal should use RandomFactory
4679e9a: 8310239: Add missing cross modifying fence in nmethod entry barriers
b9983c7: 8318961: increase javacserver connection timeout values and max retry attempts
a581897: 8317507: C2 compilation fails with "Exceeded _node_regs array"
c4c66ab: 8316271: JfrJvmtiAgent::retransformClasses failed: JVMTI_ERROR_FAILS_VERIFICATION
88b9cda: 8318981: compiler/compilercontrol/TestConflictInlineCommands.java fails intermittent with 'disallowed by CompileCommand' missing from stdout/stderr
c9d23c3: 8315532: Compiler Implementation for Unnamed Variables & Patterns
3934127: 8318709: Improve System.nanoTime performance on Windows
... and 164 more: https://git.openjdk.org/jdk/compare/9843c97695fab3fec1e319027b14974d0e84bf0a...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sspitsyn, @kevinjwalls) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

yukikimmura · 2023-10-30T23:23:18Z

/integrate

I am grateful for your support.

openjdk · 2023-10-30T23:24:32Z

@yukikimmura
Your change (at version 03a3d01) is now ready to be sponsored by a Committer.

sspitsyn · 2023-10-30T23:25:36Z

/sponsor

openjdk · 2023-10-30T23:26:19Z

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

1ca2cfa: 8310933: Copying from runtime image to application image should not follow symlinks
d3534b0: 8318677: (ch) Add implNote about minBufferCap to main variant of Channels.newWriter
864a876: 8313621: test/jdk/jdk/internal/math/FloatingDecimal/TestFloatingDecimal should use RandomFactory
4679e9a: 8310239: Add missing cross modifying fence in nmethod entry barriers
b9983c7: 8318961: increase javacserver connection timeout values and max retry attempts
a581897: 8317507: C2 compilation fails with "Exceeded _node_regs array"
c4c66ab: 8316271: JfrJvmtiAgent::retransformClasses failed: JVMTI_ERROR_FAILS_VERIFICATION
88b9cda: 8318981: compiler/compilercontrol/TestConflictInlineCommands.java fails intermittent with 'disallowed by CompileCommand' missing from stdout/stderr
c9d23c3: 8315532: Compiler Implementation for Unnamed Variables & Patterns
3934127: 8318709: Improve System.nanoTime performance on Windows
... and 164 more: https://git.openjdk.org/jdk/compare/9843c97695fab3fec1e319027b14974d0e84bf0a...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-30T23:26:33Z

@sspitsyn @yukikimmura Pushed as commit d051f22.

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

sspitsyn · 2023-10-30T23:28:18Z

Seems to be integrated now. :)

8318410: [TESTBUG] jdk/java/lang/instrument/BootClassPath/BootClassPa…

25f09bf

…thTest.sh fails on Japanese Windows

openjdk bot added the rfr Pull request is ready for review label Oct 18, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 18, 2023

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Oct 18, 2023

sspitsyn reviewed Oct 19, 2023

View reviewed changes

sspitsyn approved these changes Oct 19, 2023

View reviewed changes

8318410: [TESTBUG] jdk/java/lang/instrument/BootClassPath/BootClassPa…

03a3d01

…thTest.sh fails on Japanese Windows

sspitsyn approved these changes Oct 19, 2023

View reviewed changes

kevinjwalls approved these changes Oct 30, 2023

View reviewed changes

yukikimmura changed the title ~~8318410: [TESTBUG] jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows~~ 8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows Oct 30, 2023

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 30, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 30, 2023

openjdk bot closed this Oct 30, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows #16235

8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows #16235

yukikimmura commented Oct 18, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 18, 2023

openjdk bot commented Oct 18, 2023

mlbridge bot commented Oct 18, 2023 •

edited

Loading

AlanBateman commented Oct 18, 2023

openjdk bot commented Oct 18, 2023

sspitsyn Oct 19, 2023

yukikimmura Oct 19, 2023

sspitsyn left a comment

yukikimmura commented Oct 19, 2023

sspitsyn left a comment

yukikimmura commented Oct 23, 2023

sspitsyn commented Oct 28, 2023

openjdk bot commented Oct 28, 2023

sspitsyn commented Oct 28, 2023

yukikimmura commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

yukikimmura commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

kevinjwalls commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

yukikimmura commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows #16235

8318410: jdk/java/lang/instrument/BootClassPath/BootClassPathTest.sh fails on Japanese Windows #16235

Conversation

yukikimmura commented Oct 18, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 18, 2023

openjdk bot commented Oct 18, 2023

mlbridge bot commented Oct 18, 2023 • edited Loading

Webrevs

AlanBateman commented Oct 18, 2023

openjdk bot commented Oct 18, 2023

sspitsyn Oct 19, 2023

Choose a reason for hiding this comment

yukikimmura Oct 19, 2023

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

yukikimmura commented Oct 19, 2023

sspitsyn left a comment

Choose a reason for hiding this comment

yukikimmura commented Oct 23, 2023

sspitsyn commented Oct 28, 2023

openjdk bot commented Oct 28, 2023

sspitsyn commented Oct 28, 2023

yukikimmura commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

yukikimmura commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

kevinjwalls commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

yukikimmura commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

sspitsyn commented Oct 30, 2023

yukikimmura commented Oct 18, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 18, 2023 •

edited

Loading