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