RFR: JDK-8212828 Allow POSIX_SPAWN to be used for ProcessImpl on Linux

[Thomas Stüfe]

Hi Roger,

On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs <Roger.Riggs at oracle.com> wrote:
>
> Hi Thomas,
>
> Thanks for adding the test.
>
> There's a feature of jtreg that was exposed a couple of month ago that
> can be used to deal with running the test too many times.
>
> There can be multiple @test blocks with different @requires.
>
> * @run main/othervm/timeout=300 Basic * @run main/othervm/timeout=300
> -Djdk.lang.Process.launchMechanism=fork Basic * * @test * @requires
> (os.family == "linux") * @run main/othervm/timeout=300
> -Djdk.lang.Process.launchMechanism=posix_spawn Basic
>
>

Does not seem to work, sorry:

  24 /*
  25  * @test
  26  * @bug 4199068 4738465 4937983 4930681 4926230 4931433 4932663 4986689
  27  *      5026830 5023243 5070673 4052517 4811767 6192449 6397034 6413313
  28  *      6464154 6523983 6206031 4960438 6631352 6631966 6850957 6850958
  29  *      4947220 7018606 7034570 4244896 5049299 8003488 8054494 8058464
  30  *      8067796
  31  * @key intermittent
  32  * @summary Basic tests for Process and Environment Variable code
  33  * @modules java.base/java.lang:open
  34  * @run main/othervm/timeout=300 Basic
  35  * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=fork Basic
  36  * @test
  37  * @requires (os.family == "liinux")
  38  * @run main/othervm/timeout=300
-Djdk.lang.Process.launchMechanism=posix_spawn Basic
  39  * @author Martin Buchholz
  40  */

If I have @requires (os.family == "linux") all three variants are executed - ok.

Then I wanted to have a negative test, so I misnamed linux as "liinux"
and would have expected the first @test block to fire, the second to
be ignored. But in fact:

thomas at mainframe /shared/projects/openjdk/jtreg $
../jtreg-prebuilt/jtreg/bin/jtreg -jdk
../jdk-jdk/output-slowdebug/images/jdk/
../jdk-jdk/source/test/jdk/java/lang/ProcessBuilder/Basic.java
Test results: no tests selected

So to me it looks like as if the @requires tag is valid for both @test
blocks, not only the one it appears in.

> I'll run this through our build system too.
>
> To fully test out using posix_spawn in many more different scenarios the
> build system should be augmented to be able to use it as the default for
> an entire build/CI/CD/test runs.

Sure! But I think this is out of scope for this patch.

Thanks, Thomas

>
> Thanks, Roger
>
> On 10/24/2018 10:35 AM, Thomas Stüfe wrote:
> > Hi all,
> >
> > version 2 of Davids patch, with test changes:
> >
> > http://cr.openjdk.java.net/~stuefe/webrevs/JDK-8212828-posix_spawn.patch/webrev.01/webrev/
> >
> > Executed the test on my local Ubuntu box, works fine. Submit job runs.
> >
> > About the test: I added a new line:
> >
> >    * @run main/othervm/timeout=300 Basic
> >    * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork Basic
> > + * @run main/othervm/timeout=300
> > -Djdk.lang.Process.launchMechanism=posix_spawn Basic
> >
> > The way I understand those tests work is that the first line causes
> > the test to be run with the default launch mechanism, the second line
> > with jdk.lang.Process.launchMechanism=fork explicitly. The third line,
> > added by me, will now explicitly test with posix_spawn. I did not
> > limit the platforms, since posix_spawn should now be available on all
> > platforms, so it should work on all platforms. But then, on all other
> > platforms it has already been the default.
> >
> > This is still a bit iffy: On Windows, jdk.lang.Process.launchMechanism
> > gets ignored, so we just executed the same test twice (now three
> > times)? And now we execute the posix_spawn variant twice on all
> > platforms where this is the default, so line 1 and 3 are the same? You
> > see I am not a jtreg expert :) Can I specify a @run directive for only
> > one platform? In that case I would limit the explicit posix_spawn test
> > to Linux.
> >
> > Note however that if we really abondon vfork in the future and make
> > posix_spawn the default, this test becomes simpler too.
> >
> > Best, Thomas
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > On Wed, Oct 24, 2018 at 3:36 PM David Lloyd <david.lloyd at redhat.com> wrote:
> >> On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> >>> Review:
> >>>
> >>> - copyright dates need updating on the C-sources
> >>>
> >>> - I opt for "#if defined(__solaris__) || defined(_ALLBSD_SOURCE) ||
> >>> defined(_AIX) || defined(__linux__)" to be removed completely from
> >>> unix-specific source files. The ifdef now covers all OpenJDK Unix
> >>> platforms.
> >> Here's a version with these changes.
> >>
> >> --
> >> - DML
>