8315097: Rename createJavaProcessBuilder #15452

lkorinth · 2023-08-28T15:54:08Z

This pull request renames createJavaProcessBuilder to createLimitedTestJavaProcessBuilder and renames createTestJvm to createTestJavaProcessBuilder. Both are implemented through a private createJavaProcessBuilder. It also updates the java doc.

This is so that it should be harder to by mistake use createLimitedTestJavaProcessBuilder that is problematic because it will not forward JVM flags to the tested JVM.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)

Issue

JDK-8315097: Rename createJavaProcessBuilder (Sub-task - P4)

Reviewers

Leonid Mesnik (@lmesnik - Reviewer) ⚠️ Review applies to 2f57a32d
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 4b2d1711
Roger Riggs (@RogerRiggs - Reviewer)
Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 2f57a32d

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15452

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-28T15:55:54Z

👋 Welcome back lkorinth! 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-08-28T15:57:44Z

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

client
core-libs
hotspot
i18n
jmx
nio
security
serviceability
shenandoah

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-08-28T16:01:52Z

Webrevs

lmesnik

The. changes looks good. Please update copyrights for changed filed.
I expect that you completed execution of all tests to ensure that nothing is broken.

openjdk · 2023-08-28T19:04:57Z

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

8315097: Rename createJavaProcessBuilder

Reviewed-by: lmesnik, dholmes, rriggs, stefank

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

40a3c35: 8318723: RISC-V: C2 UDivL
3885dc5: 8318737: Fallback linker passes bad JNI handle
9864951: 8318447: Move NMT source code to own subdirectory
744e089: 8318700: MacOS Zero cannot run gtests due to wrong JVM path
ec1bf23: 8318801: Parallel: Remove unused verify_all_young_refs_precise
3cea892: 8318805: RISC-V: Wrong comments instructions cost in riscv.ad
bc1ba24: 8316437: JFR: assert(!tl->has_java_buffer()) failed: invariant
970cd20: 8318788: java/net/Socks/SocksSocketProxySelectorTest.java fails on machines with no IPv6 link-local addresses
37c40a1: 8318705: [macos] ProblemList java/rmi/registry/multipleRegistries/MultipleRegistries.java
723db2d: 8305321: Remove unused exports in java.desktop
... and 116 more: https://git.openjdk.org/jdk/compare/15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9...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.

dholmes-ora

I think the admonition about using this method is a bit strong in that it is natural to use this plain process builder method when a test is going to set its own specific flags for the exec'd process. But I'm okay with renaming to avoid copy'n'paste errors that accidentally use the wrong version.

Thanks

RogerRiggs

I don't think this is the best change across so many files.
It gives a very ugly name to a common test function and affects a very large number of tests.

RogerRiggs · 2023-08-29T14:07:07Z

/reviewers 3 reviewer

openjdk · 2023-08-29T14:07:15Z

@RogerRiggs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).

lkorinth · 2023-08-29T15:10:03Z

@RogerRiggs If it is only the name you want changed, maybe you can offer a better name?

RogerRiggs · 2023-08-29T16:45:12Z

@RogerRiggs If it is only the name you want changed, maybe you can offer a better name?
@lkorinth

Sorry for the too short comment; I wanted to make sure it wasn't integrated before I could look at it more closely.
Neither the bug report or the PR describe the problem and its ramifications, only the solution.
Can you elaborate on the conditions that lead to this. (and include them in the bug report).

plummercj · 2023-08-29T19:22:08Z

I kind of like that it is long and clumsy, that makes it harder to use...

...but it's used in 462 files. That implies it is commonly used. Are you suggesting nearly all those uses are incorrect and eventually should be converted to createTestJvm()?

dholmes-ora · 2023-08-30T07:19:34Z

@RogerRiggs , @plummercj , please see the additional discussion in the parent bug: https://bugs.openjdk.org/browse/JDK-8314823

msheppar · 2023-08-30T11:31:20Z

I don't think a name change is required. The method is createJavaProcessBuilder with a "list" of argurments and a builder is returned. As such, there is no inference, in the name, that test args will be propagated. Adding the additional java doc description should be sufficient to dispell any misconceptions.
The createTestJvm is a misnomer as it returns a ProcessBulder rather than a Process, which is what I would expected from createTestJvm, without looking at its signature.

So you could create a single createJavaProcessBuilder with add an additional parameter boolean addTestOpts e.g.
createJavaProcessBuilder(List command, boolean addTestOpts) { ... }

createProcessBuilderIgnoreJavaTestOpt(cmdArgs) maps to createJavaProcessBuilder(cmdArgs, false)

createTestJvm(cmdArgs) maps to createJavaProcessBuilder(cmdArgs, true)

But this is a lot more work.

alternatively change createTestJvm to createTestJavaProcessBuilder or createJavaProcessBuilderAddTestOpts

RogerRiggs · 2023-08-30T15:07:51Z

In the context of the goal is to declaratively identify tests that do or do not benefit from additional test flags renaming the createjavaProcessBuilder method does not further that goal.

The method name and javadoc of createjavaProcessBuilder do not imply that the test options are consulted or used; it only says it creates a ProcessBuilder, and does not promise or document more than that. The javadoc should probably describe the use of the the three properties that modify the way that the java is launched.

lkorinth · 2023-10-20T08:32:06Z

Just ignore what I just pushed, I will have a new version out

lkorinth · 2023-10-20T08:32:23Z

sorry...

openjdk · 2023-10-24T07:45:27Z

@lkorinth this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout _8315097
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

lkorinth · 2023-10-24T09:07:42Z

Hi, if you want to see what I have modified manually, you can do my sed commands and compare to this pull request:

git switch -c _reproduce 15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9

find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createLimitedTestJavaProcessBuilder(/g"
find -name "*.java" | xargs -n 1 sed -i -e "s/createTestJvm(/createTestJavaProcessBuilder(/g"
find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createJavaProcessBuilder/import static jdk.test.lib.process.ProcessTools.createLimitedTestJavaProcessBuilder/g"
find -name "*.java" | xargs -n 1 sed -i -e "s/import static jdk.test.lib.process.ProcessTools.createTestJvm/import static jdk.test.lib.process.ProcessTools.createTestJavaProcessBuilder/g"
git add -u; git commit -m sed

git diff-tree --no-commit-id --name-only -r 15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9..HEAD | xargs sed -i -e "s%^\( \* Copyright (c) ....\)[^[:alpha:]]*\(Oracle.*\)%\1, 2023, \2%"
git ls-files -m | xargs sed -i -e "s%\(Copyright (c) 2023\), 2023, \(Oracle.*\)%\1, \2%"
git add -u; git commit -m copyright

git diff HEAD 2f57a32df8d17da51a04177563327ca2a75e8061

It will give you an easier way to review.

RogerRiggs · 2023-10-24T19:36:28Z

test/lib/jdk/test/lib/process/ProcessTools.java

+    public static ProcessBuilder createTestJavaProcessBuilder(List<String> command) {
+        return createTestJavaProcessBuilder(command.toArray(String[]::new));


The javadoc shoul d describe all of the options being added to the ProcessBuilder.
They were inadequated described previously and still are.
The other options (seem to be from the code), test.noclasspath, java.class.path, and test.thread.factory.
The description of test.thread.factory and addTestThreadFactoryArgs method seems inadequately described.

RogerRiggs · 2023-10-24T19:37:57Z

test/lib/jdk/test/lib/process/ProcessTools.java

+    /**
+     * Create ProcessBuilder using the java launcher from the jdk to
+     * be tested.
+     *


As above, should described the limited options that are added to the ProcessBuilder, the same as for reateTestJavaProcessBuilder(...)

RogerRiggs · 2023-10-24T19:38:18Z

test/lib/jdk/test/lib/process/ProcessTools.java

+   /**
+     * Create ProcessBuilder using the java launcher from the jdk to
+     * be tested.
+     *


As above, should described the limited options that are added to the ProcessBuilder, the same as for reateTestJavaProcessBuilder(...)

RogerRiggs · 2023-10-24T19:39:07Z

test/lib/jdk/test/lib/process/ProcessTools.java

@@ -562,7 +596,7 @@ public static OutputAnalyzer executeTestJvm(List<String> cmds) throws Exception
     * @return The output from the process.
     */
    public static OutputAnalyzer executeTestJvm(String... cmds) throws Exception {
-        ProcessBuilder pb = createTestJvm(cmds);
+        ProcessBuilder pb = createTestJavaProcessBuilder(cmds);


This should also describe all of the options being set in the ProcessBuilder before executing the process.

stefank · 2023-10-25T07:25:06Z

@RogerRiggs You seem to know what you want w.r.t. the extra java doc comments. Could you help write those? Could we also do that as a separate RFE? I think that would make it easier to get this PR and the javadoc update through the door.

RogerRiggs

Suggestions to complete the descriptions of the createXXXJavaProcessBuilder methods.

RogerRiggs · 2023-10-25T21:52:38Z

test/lib/jdk/test/lib/process/ProcessTools.java

+     * createLimitedTestJavaProcessBuilder() you should probably use
+     * it in combination with <b>@requires vm.flagless</b> JTREG
+     * anotation as to not waste energy and test resources.
+     *


Consider adding this description of what this method does.

Suggested change

*

* Unless the "test.noclasspath" property is "true"

* the classpath property "java.class.path" is appended to the command line and

* the environment of the ProcessBuilder is modified to remove "CLASSPATH".

* If the property "test.thread.factory" is provided the command args are

* updated and appended to invoke ProcessTools main() and provide the

* name of the thread factory.

* The "-Dtest.thread.factory" is appended to the arguments with the thread factory value.

* The remaining command args are scanned for unsupported options and

* are appended to the ProcessBuilder.

RogerRiggs · 2023-10-25T21:53:15Z

test/lib/jdk/test/lib/process/ProcessTools.java

+     * createLimitedTestJavaProcessBuilder() you should probably use
+     * it in combination with <b>@requires vm.flagless</b> JTREG
+     * anotation as to not waste energy and test resources.
+     *


Suggested change

*

* Unless the "test.noclasspath" property is "true"

* the classpath property "java.class.path" is appended to the command line and

* the environment of the ProcessBuilder is modified to remove "CLASSPATH".

* If the property "test.thread.factory" is provided the command args are

* updated and appended to invoke ProcessTools main() and provide the

* name of the thread factory.

* The "-Dtest.thread.factory" is appended to the arguments with the thread factory value.

* The remaining command args are scanned for unsupported options and

* are appended to the ProcessBuilder.

RogerRiggs · 2023-10-25T21:55:54Z

test/lib/jdk/test/lib/process/ProcessTools.java

@@ -527,10 +517,54 @@ public static ProcessBuilder createTestJvm(List<String> command) {
     * @param command Arguments to pass to the java command.
     * @return The ProcessBuilder instance representing the java command.
     */
-    public static ProcessBuilder createTestJvm(String... command) {
+    public static ProcessBuilder createTestJavaProcessBuilder(String... command) {


Include the same description of other properties that are included in creating the ProcessBuilder...

* Unless the "test.noclasspath" property is "true" * the classpath property "java.class.path" is appended to the command line and * the environment of the ProcessBuilder is modified to remove "CLASSPATH". * If the property "test.thread.factory" is provided the command args are * updated and appended to invoke ProcessTools main() and provide the * name of the thread factory. * The "-Dtest.thread.factory" is appended to the arguments with the thread factory value. * The remaining command args are scanned for unsupported options and * are appended to the ProcessBuilder.

RogerRiggs · 2023-10-25T21:56:47Z

test/lib/jdk/test/lib/process/ProcessTools.java

@@ -512,8 +502,8 @@ private static void printStack(Thread t, StackTraceElement[] stack) {
     * @param command Arguments to pass to the java command.
     * @return The ProcessBuilder instance representing the java command.
     */
-    public static ProcessBuilder createTestJvm(List<String> command) {
-        return createTestJvm(command.toArray(String[]::new));
+    public static ProcessBuilder createTestJavaProcessBuilder(List<String> command) {


Include the same description of other properties that are included in creating the ProcessBuilder...

* the classpath property "java.class.path" is appended to the command line and * the environment of the ProcessBuilder is modified to remove "CLASSPATH". * If the property "test.thread.factory" is provided the command args are * updated and appended to invoke ProcessTools main() and provide the * name of the thread factory. * The "-Dtest.thread.factory" is appended to the arguments with the thread factory value. * The remaining command args are scanned for unsupported options and * are appended to the ProcessBuilder.

lkorinth · 2023-10-26T15:30:33Z

Would it be okay if we handle the new method documentation in a separate pull request?

After applying your changes, I also noted that the existing description The command line will be like: {test.jdk}/bin/java {test.vm.opts} {test.java.opts} cmds is not only incorrect (or at least incomplete), but now also clashes with the added description. I then removed the sentence, but after I did that I also found out that similar wording exist in executeTestJvm and I fear that if I continue to pull strings, I will create more and more changes that you will have opinions on.

Is it all right if we push what we have now, and that I create a new pull requests with these improvements in documentation that are actually not related to the changes in this pull request?

RogerRiggs

ok, to correct the javadoc in a subsequent PR.

lkorinth · 2023-10-27T08:46:46Z

/integrate

openjdk · 2023-10-27T08:47:27Z

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

957703b: 8307168: Inconsistent validation and handling of --system flag arguments
5b5fd36: 8316632: Shenandoah: Raise OOME when gc threshold is exceeded
abad040: 8313781: Add regression tests for large page logging and user-facing error messages
9123961: 8318096: Introduce AsymmetricKey interface with a getParams method
4a142c3: 8274122: java/io/File/createTempFile/SpecialTempFile.java fails in Windows 11
77fe0fd: 8272215: Add InetAddress methods for parsing IP address literals
a9b31b5: 8318689: jtreg is confused when folder name is the same as the test name
e1a458e: 8318834: s390x: Debug builds are missing debug helpers
40a3c35: 8318723: RISC-V: C2 UDivL
3885dc5: 8318737: Fallback linker passes bad JNI handle
... and 124 more: https://git.openjdk.org/jdk/compare/15acf4b8d7cffcd0d74bf1b9c43cde9acaf31ea9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-27T08:47:38Z

@lkorinth Pushed as commit d52a995.

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

lkorinth · 2023-10-27T08:57:56Z

Thanks, see:
https://bugs.openjdk.org/browse/JDK-8318962

8315097: Rename createJavaProcessBuilder

4b2d171

openjdk bot added the rfr Pull request is ready for review label Aug 28, 2023

lmesnik approved these changes Aug 28, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 28, 2023

dholmes-ora approved these changes Aug 29, 2023

View reviewed changes

copyright

f3418c8

RogerRiggs suggested changes Aug 29, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Aug 29, 2023

fix static import

27da715

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

lkorinth added 6 commits October 20, 2023 15:51

fix test/lib/jdk/test/lib/process/ProcessTools.java

b9ea82a

sed

050416f

whitespace

fe6c578

whitespace

caf208b

copyright

a3f1291

static import

2f57a32

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 24, 2023

stefank approved these changes Oct 24, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Oct 24, 2023

lmesnik approved these changes Oct 24, 2023

View reviewed changes

RogerRiggs reviewed Oct 24, 2023

View reviewed changes

fix copyright year and indentation

4cc3865

RogerRiggs reviewed Oct 25, 2023

View reviewed changes

RogerRiggs approved these changes Oct 26, 2023

View reviewed changes

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

openjdk bot closed this Oct 27, 2023

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

		public static ProcessBuilder createTestJavaProcessBuilder(List<String> command) {
		return createTestJavaProcessBuilder(command.toArray(String[]::new));

-     *
+     * Unless the "test.noclasspath" property is "true"
+     * the classpath property "java.class.path" is appended to the command line and
+     * the environment of the ProcessBuilder is modified to remove "CLASSPATH".
+     * If the property "test.thread.factory" is provided the command args are
+     * updated and appended to invoke ProcessTools main() and provide the
+     * name of the thread factory.
+     * The "-Dtest.thread.factory" is appended to the arguments with the thread factory value.
+     * The remaining command args are scanned for unsupported options and
+     * are appended to the ProcessBuilder.

8315097: Rename createJavaProcessBuilder #15452

8315097: Rename createJavaProcessBuilder #15452

Conversation

lkorinth commented Aug 28, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 28, 2023

openjdk bot commented Aug 28, 2023

mlbridge bot commented Aug 28, 2023 • edited

Webrevs

lmesnik left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Aug 28, 2023 • edited

dholmes-ora left a comment

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

RogerRiggs commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

lkorinth commented Aug 29, 2023

RogerRiggs commented Aug 29, 2023

plummercj commented Aug 29, 2023

dholmes-ora commented Aug 30, 2023

msheppar commented Aug 30, 2023

RogerRiggs commented Aug 30, 2023

lkorinth commented Oct 20, 2023

lkorinth commented Oct 20, 2023

openjdk bot commented Oct 24, 2023

lkorinth commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank commented Oct 25, 2023

RogerRiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkorinth commented Oct 26, 2023

RogerRiggs left a comment

Choose a reason for hiding this comment

lkorinth commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

lkorinth commented Oct 27, 2023

lkorinth commented Aug 28, 2023 •

edited by openjdk bot

mlbridge bot commented Aug 28, 2023 •

edited

lmesnik left a comment •

edited

openjdk bot commented Aug 28, 2023 •

edited