I'm in favor, for whatever that's worth. On Thu, Oct 25, 2018 at 5:33 AM Thomas Stüfe <thomas.stu...@gmail.com> wrote: > > Hi all, > > the more I mull over this, the more I would prefer to do the jump for > real and attempt switch the default to posix_spawn() for Linux. > > We have theoretically established that both glibc down to 2.4 and > muslc since always did "the right thing". > > We still have time in the 12 time line to test this thoroughly. > > What do you think? > > Thanks, Thomas > > On Wed, Oct 24, 2018 at 10:59 PM Thomas Stüfe <thomas.stu...@gmail.com> wrote: > > > > Hi Roger, > > > > On Wed 24. Oct 2018 at 21:39, Roger Riggs <roger.ri...@oracle.com> wrote: > >> > >> Hi Thomas, > >> > >> Sorry, I had the incantation for multiple tests wrong. > >> Separate test configurations need to be in separate /* */ comment blocks. > >> BTW, it creates separate .jtr files for each block. > >> > >> diff --git a/test/jdk/java/lang/ProcessBuilder/Basic.java > >> b/test/jdk/java/lang/ProcessBuilder/Basic.java > >> --- a/test/jdk/java/lang/ProcessBuilder/Basic.java > >> +++ b/test/jdk/java/lang/ProcessBuilder/Basic.java > >> @@ -33,8 +33,10 @@ > >> * @modules java.base/java.lang:open > >> * @run main/othervm/timeout=300 Basic > >> * @run main/othervm/timeout=300 -Djdk.lang.Process.launchMechanism=fork > >> Basic > >> - * > >> + */ > >> +/* > >> * @test > >> + * @modules java.base/java.lang:open > >> * @requires (os.family == "linux") > >> * @run main/othervm/timeout=300 > >> -Djdk.lang.Process.launchMechanism=posix_spawn Basic > >> * @author Martin Buchholz > >> > >> > >> As for the build configuration being out of scope... > >> > >> I don't want to see part of the work done and then the rest forgotten or > >> worse yet someone comes along in the future and incorrectly believes that > >> posix_spawn is ready for production and starts filing bugs or getting bit > >> by something. > >> There's a lot more testing to do before it could be come the default > >> and perhaps a significant warning should be attached to the code so it > >> does not get forgotten. > > > > > > I agree, we ideally should not roll out half tested changes. But where does > > that leave us wrt this patch? > > > > We could just not do it, requiring anyone willing to do the extensive > > testing necessary to switch the default to posix-spawn to apply this change > > first locally. This is not an insurmountable amount of work, especially > > since the base is quite static, so the patch won’t bit rot easily. > > > > We could push the patch in its current form, plus a large source comment? > > But then. Users do not read comments. A warning on stderr? Would play havoc > > with many tests parsing stderr. > > > > Do you have an idea how to proceed? > > > > Thanks, Thomas > > > >> > >> Regards, Roger > >> > >> > >> On 10/24/18 1:53 PM, Thomas Stüfe wrote: > >> > >> Hi Roger, > >> > >> On Wed, Oct 24, 2018 at 5:23 PM Roger Riggs <roger.ri...@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@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.ll...@redhat.com> wrote: > >> > >> On Wed, Oct 24, 2018 at 1:05 AM Thomas Stüfe <thomas.stu...@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 > >> > >>
-- - DML
