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