Hi Roger, On Thu, Oct 25, 2018 at 10:45 PM Roger Riggs <roger.ri...@oracle.com> wrote: > > Hi Thomas, > > In an abundance of caution, I was thinking that it would be a change right > at the beginning of a new release so it gets the most exercise and > users in early access, etc. >
Okay, I understand that. Over the next days I will run tests with posix_spawn enabled by default on our landscape. We have many different Linuxes on different architectures and different levels of glibc, so this is a reasonable test. If I do not encounter red flags, I would consider the posix_spawn path tested well enough to ship it at least as a non-default, experimental option. Like David originally intended. Then, start of next release, we can make it default and see how that goes. Does that sound ok to you? > And before that it needs to be put into more regular usage and > some more unusual environments. The default is currently selected > by virtual of being first in the argument list; that doesn't lend itself > to being configurable with either configure or make arguments. I do not understand the last sentence: it was never my intention of adding a configure option? > > Roger > Thanks, Thomas > > On 10/25/2018 06:33 AM, Thomas Stüfe 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 > > >
