On Wed, 30 Aug 2023 09:23:55 GMT, Leo Korinth <lkori...@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
