RFR: 8315097: Rename createJavaProcessBuilder [v3]

[Mark Sheppard]

On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth <lkorinth at openjdk.org> wrote:

>> Rename createJavaProcessBuilder so that it is not used by mistake instead of createTestJvm.
>> 
>> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"`
>> 
>> Then I have manually modified ProcessTools.java. In that file I have moved one version of createJavaProcessBuilder so that it is close to the other version. Then I have added a javadoc comment in bold telling:
>> 
>>    /**
>>      * Create ProcessBuilder using the java launcher from the jdk to
>>      * be tested.
>>      *
>>      * <p><b> Please observe that you likely should use
>>      * createTestJvm() instead of this method because createTestJvm()
>>      * will add JVM options from "test.vm.opts" and "test.java.opts"
>>      * </b> and this method will not do that.
>>      *
>>      * @param command Arguments to pass to the java command.
>>      * @return The ProcessBuilder instance representing the java command.
>>      */
>> 
>> 
>> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a better name I could do a rename of the method. I kind of like that it is long and clumsy, that makes it harder to use...
>> 
>> I have run tier 1 testing, and I have started more exhaustive testing.
>
> Leo Korinth has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix static import

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<String> 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

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1698985460