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

8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment #9389

Closed
wants to merge 6 commits into from

Conversation

tkiriyama
Copy link
Contributor

@tkiriyama tkiriyama commented Jul 6, 2022

I removed a section of via JDK_JAVA_OPTIONS because including main class is not allowed in the specification.
This behavior is added in JDK-8170832, which add JAVA_OPTIONS environment variable. At this time, this test is mismatch with the specification.
I tried to test and get Passed on Japanese Windows environment.
Could you review this fix, please?


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-8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9389

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 6, 2022

👋 Welcome back tkiriyama! 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 openjdk bot added the rfr Pull request is ready for review label Jul 6, 2022
@openjdk
Copy link

openjdk bot commented Jul 6, 2022

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

  • core-libs
  • hotspot-runtime

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.

@openjdk openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jul 6, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 6, 2022

Webrevs

@tkiriyama tkiriyama changed the title 8289797: [TESTBUG]tools/launcher/I18NArgTest.java fails on Japanese Windows enviromnent 8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows enviromnent Jul 7, 2022
@jaikiran
Copy link
Member

jaikiran commented Jul 8, 2022

The change looks fine to me since in its current form the usage of JDK_JAVA_OPTIONS is incorrect.

Having said that and looking at the history of this file, I think, this part of the test was added to check if the java launcher would be able to read an environment variable (specifically the JDK_JAVA_OPTIONS) whose value was in MS932 encoding, and pass it along as a correctly encoded String, all the way to the main method of the application. If we remove this section of the test, then we would be skipping that code path completely.

Perhaps this part of the test could be modified a bit to let it pass the JDK_JAVA_OPTIONS environment variable with a MS932 encoded value that acts some option to the java launcher, instead of being the argument to the main method? That way, you won't have to specify the main class name in the value of the environment variable. Specifically, something like:

diff --git a/test/jdk/tools/launcher/I18NArgTest.java b/test/jdk/tools/launcher/I18NArgTest.java
index fa09736da2f..2ba724d6f1d 100644
--- a/test/jdk/tools/launcher/I18NArgTest.java
+++ b/test/jdk/tools/launcher/I18NArgTest.java
@@ -97,12 +97,17 @@ public class I18NArgTest extends TestHelper {
 
         // Test via JDK_JAVA_OPTIONS
         Map<String, String> env = new HashMap<>();
-        String cmd = "-Dtest.src=" + TEST_SOURCES_DIR.getAbsolutePath() +
-                " -Dtest.classes=" + TEST_CLASSES_DIR.getAbsolutePath() +
-                " -cp " + TEST_CLASSES_DIR.getAbsolutePath() +
-                " I18NArgTest " + unicodeStr + " " + hexValue;
-        env.put("JDK_JAVA_OPTIONS", cmd);
-        tr = doExec(env, javaCmd);
+        String sysPropName = "foo.bar";
+        // pass "-Dfoo.bar=<unicodestr>" through the JDK_JAVA_OPTIONS environment variable.
+        // we expect that system property value to be passed along to the main method with the
+        // correct encoding
+        String jdkJavaOpts = "-D" + sysPropName + "=" + unicodeStr;
+        env.put("JDK_JAVA_OPTIONS", jdkJavaOpts);
+        tr = doExec(env,javaCmd,
+                "-Dtest.src=" + TEST_SOURCES_DIR.getAbsolutePath(),
+                "-Dtest.classes=" + TEST_CLASSES_DIR.getAbsolutePath(),
+                "-cp", TEST_CLASSES_DIR.getAbsolutePath(),
+                "I18NArgTest", unicodeStr, hexValue, sysPropName);
         System.out.println(tr.testOutput);
         if (!tr.isOK()) {
             System.err.println(tr);
@@ -125,5 +130,23 @@ public class I18NArgTest extends TestHelper {
                 "expected:" + expected + " obtained:" + hexValue;
             throw new RuntimeException(message);
         }
+        if (args.length == 3) {
+            // verify the value of the system property matches the expected value
+            String sysPropName = args[2];
+            String sysPropVal = System.getProperty(sysPropName);
+            if (sysPropVal == null) {
+                throw new RuntimeException("Missing system property " + sysPropName);
+            }
+            String sysPropHexVal = "";
+            for (int i = 0; i < sysPropVal.length(); i++) {
+                sysPropHexVal = sysPropHexVal.concat(Integer.toHexString(sysPropVal.charAt(i)));
+            }
+            System.out.println("System property " + sysPropName + " computed hex value: "
+                    + sysPropHexVal);
+            if (!sysPropHexVal.equals(expected)) {
+                throw new RuntimeException("Unexpected value in system property, expected "
+                        + expected + ", but got " + sysPropHexVal);
+            }
+        }
     }
 }

I haven't tested this change, so you might have to experiment with it a bit. What do you think?

@naotoj
Copy link
Member

naotoj commented Jul 8, 2022

I agree with Jai here. It would be desirable to convert the incorrect test to something originally intended, ie., tests whether JDK_JAVA_OPTIONS env can correctly handle non-ASCII variables.

@tkiriyama
Copy link
Contributor Author

@jaikiran
Thank you for your comment. I agree to the additional check.
I run the test you proposed, but it failed. I'm surveying that reason.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 16, 2022

@tkiriyama 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk openjdk bot removed the rfr Pull request is ready for review label Aug 26, 2022
@tkiriyama
Copy link
Contributor Author

@jaikiran @naotoj

I am sorry for the late response.
I added the test pattern you proposed.
According to "Using the JDK_JAVA_OPTIONS Launcher Environment Variable" at JDK 18 Documentation of Oracle, the encoding requirement for JDK_JAVA_OPTIONS environment variable is the same as the java command line on the system, so I modified the processing for determining the result of the test via JDK_JAVA_OPTIONS to use contains method in the same way as the test without JDK_JAVA_OPTIONS. Also, I added the processing to add double quotes around space or tab in system property value specified in JDK_JAVA_OPTIONS.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 26, 2022
Copy link
Member

@naotoj naotoj left a comment

Choose a reason for hiding this comment

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

LGTM

// When pass "-Dfoo.bar=<unicodestr>" via the JDK_JAVA_OPTIONS environment variable,
// we expect that system property value to be passed along to the main method with the
// correct encoding
// If <unicodestr> contains space or tab, it should beenclosed with double quotes.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: space is missing between "be" and "enclosed"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment, I fixed it.

Comment on lines 145 to 147
for (int i = 0; i < sysPropVal.length(); i++) {
sysPropHexVal = sysPropHexVal.concat(Integer.toHexString(sysPropVal.charAt(i)));
}
Copy link
Member

Choose a reason for hiding this comment

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

It's OK as it stands, but this loop could be replaced with a HexFormat one-liner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I can't replace this code with a HexFormat one-line. Do you have an example code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Extract to a byte array with the appropriate charset and then HexFormat the bytearray.

    var SysPropHexVal = HexFormat.of().formatHex(sysPropVal.getBytes(StandardCharsets.UTF_16));

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, Roger. You beat me to it 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I fixed it. In addition, since the return value of formatHex is four digits, I fixed the second argument passed to execTest to match that format.

@naotoj
Copy link
Member

naotoj commented Aug 27, 2022

BTW, please correct the PR title (contains a typo) which doesn't agree with the JBS issue.

@tkiriyama tkiriyama changed the title 8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows enviromnent 8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment Sep 16, 2022
@openjdk
Copy link

openjdk bot commented Sep 16, 2022

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

8289797: tools/launcher/I18NArgTest.java fails on Japanese Windows environment

Reviewed-by: naoto

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

  • 1ddc92f: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
  • 1e222bc: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
  • 43eff2b: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
  • b88ee1e: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
  • aca4276: 8294379: Missing comma after copyright year
  • 1f521a1: 8225012: sanity/client/SwingSet/src/ToolTipDemoTest.java fails on Windows
  • 5ae6bc2: 8234262: Unmask SIGQUIT in a child process
  • 968af74: 8293567: AbstractSplittableWithBrineGenerator: salt has digits that duplicate the marker
  • 36b61c5: 8293872: Make runtime/Thread/ThreadCountLimit.java more robust
  • 2be3158: 4797982: Setting negative size of JSplitPane divider leads to unexpected results.
  • ... and 997 more: https://git.openjdk.org/jdk/compare/649f2d8835027128c6c8cf37236808094a12a35f...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 (@naotoj) 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 openjdk bot added the ready Pull request is ready to be integrated label Sep 16, 2022
Copy link
Member

@naotoj naotoj left a comment

Choose a reason for hiding this comment

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

Looks good. Please restore the newline at the end before integrating the changes

}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: don't remove the newline at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry for my mistake. I fixed it.

@tkiriyama
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@tkiriyama
Your change (at version 73c9972) is now ready to be sponsored by a Committer.

@naotoj
Copy link
Member

naotoj commented Sep 27, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 27, 2022

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

  • 7151128: 8294317: Insufficient build rules for tzdb.dat
  • fb4979c: 8290401: Support dump all phases and print nodes in ascending order of index
  • 112ca2b: 8293964: Unused check_for_duplicates parameter in ClassLoaderExt::process_jar_manifest
  • 99017b0: 8293064: Remove unused NET_xxx functions
  • 3419363: 8294361: Cleanup usages of StringBuffer in SQLOutputImpl
  • 1abf971: 8249627: Degrade Thread.suspend and Thread.resume
  • bc12e95: 8292969: Bad Thread Utilization in ForkJoinPool
  • dd51f7e: 8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop
  • 14c6ac4: 8293998: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
  • 02ea338: 8293887: AArch64 build failure with GCC 12 due to maybe-uninitialized warning in libfdlibm k_rem_pio2.c
  • ... and 1009 more: https://git.openjdk.org/jdk/compare/649f2d8835027128c6c8cf37236808094a12a35f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 27, 2022
@openjdk openjdk bot closed this Sep 27, 2022
@openjdk 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 27, 2022
@openjdk
Copy link

openjdk bot commented Sep 27, 2022

@naotoj @tkiriyama Pushed as commit a11477c.

💡 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 hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants