Hello,
Nice to see a good attempt at getting the new build in order. While this
looks like it works, I have a few comments:
In makefiles/CompileLaunchers.gmk
* OPENJDK_TARGET_CPU_LIBDIR is empty on macosx, so you can safely use
the same definition of BUILD_JSPAWNHELPER_DST_DIR for all platforms.
* For macosx BUILD_JSPAWNHELPER := 1 is set twice. I would remove this
variable and just put the ifneq logic directly around the macro call
since it's easily expressible with just one ifneq. Also, reusing the
same variable as the namespace for the macro call can be a bit confusing.
* The -m64 flag should already be present in LDFLAGS_JDKEXE on solaris.
Does it really need to be set on macosx?
* Since this executable is dependent on an object file from libjava, I
would like that dependency to be expressed explicitly so that a relink
is properly triggered on change. This could be achieved by adding the
following after the macro call:
$(BUILD_JSPAWNHELPER): $(LINK_JSPAWNHELPER_OBJECTS)
* Finally, since BUILD_JSPAWNHELPER_SRC and BUILD_JSPAWNHELPER_DST_DIR
are only used once, I would inline them into the macro call. At least I
find it easier to read that way.
/Erik
On 2012-12-20 03:28, Rob McKenna wrote:
Hi folks,
Thanks for the feedback so far. I've uploaded a new webrev to:
http://cr.openjdk.java.net/~robm/5049299/webrev.02/
<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.02/>
I've made the following headline changes:
- Initial effort to get this stuff into the new build-infra. Hoping
build-infra can steer me in the right direction. (note to build infra
reviewers: see links below)
- Source thats shared between jspawnhelper.c and UNIXProcess_md.c now
resides in childproc.h/c. This results in cleaner changes to the
makefiles.
- jspawnhelper moved to <jdk_home>/lib/<arch> on solaris (ipc
necessitate the use of a separate jspawnhelper for each arch) and just
/lib on macosx.
The following links to earlier threads are well worth reading for
additional context:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-November/012417.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-June/001747.html
-Rob
On 30/11/12 03:48, Rob McKenna wrote:
Hi David,
On 30/11/12 02:31, David Holmes wrote:
Hi Rob,
This is only a superficial scan.
The changes in java/java/makefile look pretty horrible. What are all
those -R entries?
Library search paths. Currently jprochelper is linked to libjava. I'm
hoping to either cut their number (by altering jprochelpers home) or
get rid of them altogether (by avoiding linking at all) in the next
draft, they are indeed ungainly.
We will need equivalent changes for the new build system before this
is pushed.
Indeed.
Is the spawn use BSD specific (as per UnixProcess.java.BSD) or Apple
specific (as per __APPLE_ in UNIXProcess_md.c) ?
Eesh, thanks, it applies to both platforms.
Are the UnixProcess.java files similar enough that we could use a
single template and generate the per-OS variants?
Before this change .bsd & .linux were identical (iirc) unfortunately,
no longer. Solaris has differences. When you say "generate the
per-OS variants" how do you mean? I'd like to keep it as
straightforward as possible from a sustaining perspective.
(personally I'd like to just extend a base class and try to get away
from the makefiles as much as possible - we can discuss this in
8000975 which I'll revisit once I get through this)
In UNIXProcess_md.c:
209 #ifdef _CS_PATH
210 char *pathbuf;
211 size_t n;
212 n = confstr(_CS_PATH,NULL,(size_t) 0);
213 pathbuf = malloc(n);
214 if (pathbuf == NULL)
215 abort();
216 confstr(_CS_PATH, pathbuf, n);
217 return pathbuf;
218 #else
what is _CS_PATH and why are we calling abort()? !!!!
As per Martins comments I'm going to separate this into another
change. See:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/001686.html
and
http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-November/012456.html
for context. I'll look to fall back to the previous code if the
pathbuf malloc fails.
What is all the xx_ naming ??
I believe Michael was using it to denote shared calls. (functions
called from both jprochelper and within UNIXProcess_md.c). I presumed
it was placeholder text actually, in any case it may go away in the
next iteration as per previous comments. If not, I'm happy to replace
it with whatever gets it past codereview.
-Rob
David
-----
On 23/11/2012 7:27 AM, Rob McKenna wrote:
Hi folks,
Looking for a review for the webrev below, which also resolves:
7175692: (process) Process.exec should use posix_spawn [macosx]
For additional context and a brief description it would be well worth
looking at the following thread started by Michael McMahon, who did
the
brunt of the work for this fix:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/thread.html#1644
Basically the fix aims to swap fork for posix_spawn as the default
process launch mechanism on Solaris and Mac OSX in order to avoid swap
exhaustion issues encountered with fork()/exec(). It also offers a
flag
(java.lang.useFork) to allow a user to revert to the old behaviour.
I'm having trouble seeing the wood for the trees at this point so I'm
anticipating plenty of feedback. In particular I'd appreciate some
discussion on:
- The binary launcher name & property name may need some work. A more
general property ("java.lang.launchMechanism") to allow a user to
specify a particular call was mooted too. It may be more future proof
and I'm completely open to that. (e.g.
launchMechanism=spawn|fork|vfork|clone - we would obviously ignore
inapplicable values on a per-platform basis)
- I'd like a more robust way of checking that someone isn't trying to
use jprochelper outside of the context for which it is meant.
- The decision around which call to use getting moved to the java
level
and away from the native preprocessor.
The webrev is at:
http://cr.openjdk.java.net/~robm/5049299/webrev.01/
<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.01/>
Thanks a lot,
-Rob
