8349288: runtime/os/windows/TestAvailableProcessors.java fails on localized Windows platform #23536

ktakakuri · 2025-02-10T06:18:42Z

To resolve runtime/os/windows/TestAvailableProcessors.java failure, I made "systeminfo.exe" executed with "chcp 437". This ensures that the English message "OS Version: " is output on localized windows platforms.
I added the path C:\Windows\System32 to make chcp command recognized on the GHA Windows test machine. Without this addition, GHA will fail.
After this fix, I verified that the test passed.

#22142
I refer to this fix.
tools/jpackage/windows/Win8301247Test.java does not run in GHA, so the problems with recognition of chcp command did not occur before.

Thanks.

Progress

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

Issue

JDK-8349288: runtime/os/windows/TestAvailableProcessors.java fails on localized Windows platform (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23536

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…alized Windows platform

bridgekeeper · 2025-02-10T06:19:34Z

👋 Welcome back ktakakuri! 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 · 2025-02-10T06:19:43Z

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

8349288: runtime/os/windows/TestAvailableProcessors.java fails on localized Windows platform

Reviewed-by: dholmes, 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 2002 new commits pushed to the master branch:

53b3e05: 8366588: VectorAPI: Re-intrinsify VectorMask.laneIsSet where the input index is a variable
8cd4e7d: 8365192: post_meth_exit should be in vm state when calling get_jvmti_thread_state
f964039: 8361376: Regressions 1-6% in several Renaissance in 26-b4 only MacOSX aarch64
... and 1999 more: https://git.openjdk.org/jdk/compare/bcc33d5ef3bdbfaee51c45014851c54028da03f1...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 (@dholmes-ora, @AlanBateman) 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).

openjdk · 2025-02-10T06:20:10Z

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

hotspot-runtime

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.

mlbridge · 2025-02-10T06:23:37Z

Webrevs

…alized Windows platform

dholmes-ora

@ktakakuri I need to evaluate whether this is a general purpose fix that should work everywhere.

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

dholmes-ora · 2025-02-12T02:38:02Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

+        command.addAll(List.of("cmd.exe", "/c", "set", "PATH=%PATH%;C:\\Windows\\System32;C:\\Windows\\System32\\wbem", "&&"));
+        command.addAll(List.of("chcp", "437", ">nul", "2>&1", "&&"));


I see the use of chcp in the test ./tools/jpackage/helpers/jdk/jpackage/test/Executor.java so that is okay. However it doesn't set the path and it does not seem necessary to do so, and potentially assumes what the paths are anyway. So I think you can simplify this a little.

As well as tools/jpackage/windows/Win8301247Test.java, the tools tests is not included in jdk/tier1, so it will not run in GHA?

Sorry I'm not sure what you are asking.

I think tools/jpackage/helpers/jdk/jpackage/test/Executor.java is not run in GHA.

Correct. That is a tier2 test and IIUC GHA only runs tier1.

But I'm not sure why you're asking. I was pointing out that the jpackage test uses chcp in a simpler way than you have used, so I was suggesting you simplify your code and see if it still works.

Running TestAvailableProcessors.java in GHA without adding the PATH failed. Additional processing is required to pass GHA.
Since the jpackage tests are not run in GHA, there is no need to add PATH.

Hmmm .... okay. This is ony a test so if things go wrong we can re-adjust the test.

…alized Windows platform

dholmes-ora · 2025-03-21T12:01:04Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

+        command.addAll(List.of("cmd.exe", "/c", "set", "PATH=%PATH%;C:\\Windows\\System32;C:\\Windows\\System32\\wbem", "&&"));
+        command.addAll(List.of("chcp", "437", ">nul", "2>&1", "&&"));


Hmmm .... okay. This is ony a test so if things go wrong we can re-adjust the test.

ktakakuri · 2025-03-24T07:42:25Z

/integrate

openjdk · 2025-03-24T07:42:56Z

@ktakakuri
Your change (at version 890f013) is now ready to be sponsored by a Committer.

dholmes-ora · 2025-03-25T01:29:21Z

@ktakakuri hotspot changes require two reviews. I will try to drum up another reviewer.

sendaoYan · 2025-03-25T02:47:59Z

/reviewers 2

openjdk · 2025-03-25T02:48:09Z

@sendaoYan Only the author of the pull request or Reviewers are allowed to change the number of required reviewers.

AlanBateman · 2025-03-25T06:57:45Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

-        var processBuilder = new ProcessBuilder(systeminfoPath);
+        List<String> command = new ArrayList<>();
+        // Force language to English before running systeminfo to get the OS version
+        command.addAll(List.of("cmd.exe", "/c", "set", "PATH=%PATH%;C:\\Windows\\System32;C:\\Windows\\System32\\wbem", "&&"));


Not sure about hardcoding C:\Windows in tests as Windows could be installed on a different volume. Don't you need to use the value of %SystemRoot% to the directory?

I thought it unfortunate that a path needed to be specified, but there is a precedent for assuming C:\Windows:

./jdk/java/lang/ProcessHandle/ProcessUtil.java: return Platform.isWindows() && p.info().command().orElse("").endsWith("C:\\Windows\\System32\\conhost.exe");

though a more general approach would be similar to ./jdk/java/lang/RuntimeTests/exec/WinCommand.java:

File systemRoot = getenv("SystemRoot") != null ? new File(getenv("SystemRoot")) : getenv("WINDIR") != null ? new File(getenv ("WINDIR")) : null; if (systemRoot == null || ! systemRoot.isDirectory()) return; // Not Windows as we know it String systemDirW = new File(systemRoot, "System32").getPath();

I was also reminded that testing under non-US-English locales is not really supported:
https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk/master/doc/building.html#windows

The recommended and supported way of building the JDK on Windows is to set both the system locale and the user locale to US English. The system setting can be changed by going to the Control Panel, choosing "Regional Settings" -> "Administrative" and then pressing on the "Change System Locale" button.

Since this is annoying for users who prefer another locale, we strive to get the building and testing to work on other locales as well. This is on a "best effort" level, so beware! You might get odd results in both building and testing. If you do, remember that locales other than US English are not supported nor recommended.

It is also imperative to install the US English language pack in Visual Studio. For details, see Microsoft Visual Studio.

Thank you for your review. I changed it to use System.getenv().

…alized Windows platform

dholmes-ora · 2025-04-14T03:34:15Z

I need to re-test this in our CI system.

dholmes-ora · 2025-04-14T07:00:10Z

@ktakakuri please merge this with latest master as it is getting along way behind - thanks.

dholmes-ora · 2025-04-14T08:07:53Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

+            System.getenv("WINDIR")     != null ? new File(System.getenv ("WINDIR")) :
+            null;
+        if (systemRoot == null || ! systemRoot.isDirectory())
+            return; // Not Windows as we know it


This doesn't compile - the function returns a String.

Even if systemRoot fails to recognize the system, it does not matter if chcp can be executed, so I removed the judgment.

…alized Windows platform

dholmes-ora

Thanks for update. Nothing further from me.

As we are passed RDP1 this will get commited into JDK 26, but as a test fix it is also eligible to be backported to JDK 25 if you chose.

ktakakuri · 2025-06-17T07:52:20Z

@AlanBateman This PR requires two approvals. I apologize for the long delay, but could you review it?

AlanBateman · 2025-06-20T09:23:47Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

+        if (systemRoot == null) {
+            throw new RuntimeException("SystemRoot or WINDIR environment variable is not set.");
+        }
+        String systemDirW = new File(systemRoot, "System32").getPath();


A drive-by comment is that you could remove the duplicate calls to getenv and just use a simple Path resolve. e.g.

String systemRoot = System.getenv("SystemRoot"); if (systemRoot == null) { systemRoot = System.getenv("WINDIR"); if (systemRoot == null) { // throw } } String system32 = Path.of(systemRoot, "System32").toString();

AlanBateman · 2025-06-20T09:27:00Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

+        // Force language to English before running systeminfo to get the OS version
+        command.addAll(List.of("cmd.exe", "/c", "set", "PATH=%PATH%;" + systemDirW + ";" + systemDirW + "\\wbem", "&&"));
+        command.addAll(List.of("chcp", "437", ">nul", "2>&1", "&&"));
+        command.add(systeminfoPath);


I assume it would be more correct to say that it switches the active code page to cp437, the default code page for US english.

AlanBateman · 2025-06-20T09:27:43Z

@AlanBateman This PR requires two approvals. I apologize for the long delay, but could you review it?

Sure, just have some drive by comments, switching to cp437 should be okay.

…alized Windows platform

ktakakuri · 2025-06-25T08:15:07Z

@AlanBateman Thank you for the suggested fixes. I modified the code as such.

bridgekeeper · 2025-07-23T09:26:53Z

@ktakakuri 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 issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2025-07-28T09:24:09Z

/reviewers 2

openjdk · 2025-07-28T09:24:52Z

@ktakakuri
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

ktakakuri · 2025-07-28T09:29:11Z

@AlanBateman I fixed the code as you suggested. Could you please review it?

ktakakuri · 2025-07-29T12:26:43Z

@dholmes-ora I have made some fixes, so please review it again.

dholmes-ora

Sorry I did not see that further updates had occurred.

Seems okay.

Thanks

bridgekeeper · 2025-08-30T11:16:33Z

@ktakakuri 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 issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ktakakuri · 2025-09-04T01:17:19Z

@AlanBateman I know you're busy, but I'm looking forward to your review.

AlanBateman · 2025-09-09T12:43:30Z

test/hotspot/jtreg/runtime/os/windows/TestAvailableProcessors.java

 * @run testng TestAvailableProcessors
 */

+import java.io.File;


I assume this is a left-over from early iterations.

AlanBateman

Looks okay to me, thanks for moving away from the hardcoded root.

ktakakuri · 2025-09-10T02:08:39Z

/integrate

openjdk · 2025-09-10T02:09:33Z

@ktakakuri
Your change (at version 3490e65) is now ready to be sponsored by a Committer.

dholmes-ora · 2025-09-10T06:36:18Z

/sponsor

openjdk · 2025-09-10T06:37:25Z

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

b7b01d6: 8366984: Remove delay slot support
2705e88: 8366764: Deproblemlist java/awt/ScrollPane/ScrollPositionTest.java
8ab8d02: 8366938: Test runtime/handshake/HandshakeTimeoutTest.java crashed
... and 2003 more: https://git.openjdk.org/jdk/compare/bcc33d5ef3bdbfaee51c45014851c54028da03f1...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-10T06:37:42Z

@dholmes-ora @ktakakuri Pushed as commit 9e3fa32.

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

ktakakuri added 3 commits February 4, 2025 17:41

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

11142f2

…alized Windows platform

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

78b01d3

…alized Windows platform

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

aa14bd3

…alized Windows platform

openjdk bot added the rfr Pull request is ready for review label Feb 10, 2025

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 10, 2025

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

b18bbda

…alized Windows platform

dholmes-ora reviewed Feb 10, 2025

View reviewed changes

dholmes-ora suggested changes Feb 12, 2025

View reviewed changes

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

890f013

…alized Windows platform

dholmes-ora approved these changes Mar 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 21, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 24, 2025

AlanBateman reviewed Mar 25, 2025

View reviewed changes

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

d3fc95a

…alized Windows platform

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Apr 11, 2025

dholmes-ora suggested changes Apr 14, 2025

View reviewed changes

ktakakuri added 2 commits April 22, 2025 17:37

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

7355435

…alized Windows platform

Merge branch 'master' into 8349288

64b72c9

dholmes-ora approved these changes Jun 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 9, 2025

AlanBateman reviewed Jun 20, 2025

View reviewed changes

8349288: runtime/os/windows/TestAvailableProcessors.java fails on loc…

3490e65

…alized Windows platform

openjdk bot removed the ready Pull request is ready to be integrated label Jun 25, 2025

dholmes-ora approved these changes Aug 2, 2025

View reviewed changes

AlanBateman reviewed Sep 9, 2025

View reviewed changes

AlanBateman approved these changes Sep 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 10, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 10, 2025

openjdk bot closed this Sep 10, 2025

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 Sep 10, 2025

		command.addAll(List.of("cmd.exe", "/c", "set", "PATH=%PATH%;C:\\Windows\\System32;C:\\Windows\\System32\\wbem", "&&"));
		command.addAll(List.of("chcp", "437", ">nul", "2>&1", "&&"));

8349288: runtime/os/windows/TestAvailableProcessors.java fails on localized Windows platform #23536

8349288: runtime/os/windows/TestAvailableProcessors.java fails on localized Windows platform #23536

Uh oh!

Conversation

ktakakuri commented Feb 10, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 10, 2025

Uh oh!

openjdk bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 10, 2025

Uh oh!

mlbridge bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktakakuri Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktakakuri commented Mar 24, 2025

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

dholmes-ora commented Mar 25, 2025

Uh oh!

sendaoYan commented Mar 25, 2025

Uh oh!

openjdk bot commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Apr 14, 2025

Uh oh!

dholmes-ora commented Apr 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

ktakakuri commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Jun 20, 2025

Uh oh!

ktakakuri commented Feb 10, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 10, 2025 •

edited

Loading

mlbridge bot commented Feb 10, 2025 •

edited

Loading

ktakakuri Mar 21, 2025 •

edited

Loading