Heiko, Thanks for your continuing work on this. ProcessBuilder/Basic.java has most of the tests related to subprocesses. I can't explain a return code of 6, since the test uses 5, 7, and 8.
ProcessBuilder has a lot of infrastructure to help you write a test in this area, but it can be intimidating to newcomers (i.e. anyone but myself). The JDK C code is quite inconsistent, but please use the style /* * comments * here */ for block comments, and /* inline comments */ like this. You need to remove the comment /* selected based on exe type */ which is no longer correct. If you fix the style things, and get the tests to pass, and add a test for what you're actually trying to fix, I am ready to approve the change. Martin On Fri, Mar 20, 2009 at 03:33, Heiko Wagner <heiko.wag...@apis.de> wrote: > Thanks for your reply. I have updated both the source file and the diff > file. (URL see in previous post below) > I have made the following changes: > > - replace C++ style comments with plain C style comments > - replace tabs with spaces in source (maybe using VC 2008 as editor in the > first place was no good idea ;-)) > - remove function releaseStringCopy and call free() directly > - fix the types you suggested "path.md.c => path_md.c" and "*(res + len) = > 0; => res[len] = L'\0';" > > I have tried to set up jtreg and run the regression tests. Somehow, I still > haven't managed to get all things > working. Some tests in ProcessBuilder/Basic.java fail, because the exit code > of the invocation of the java child > is 6 insted of 0. I am still working on that issue. > > P.S.: Is there any kind of guide line how to write the comments, so I can > fix them as well? > > > -----Original Message----- > From: Martin Buchholz [mailto:marti...@google.com] > Sent: Dienstag, 17. März 2009 02:32 > To: Heiko Wagner > Cc: core-libs-dev@openjdk.java.net; Xueming Shen; Alan Bateman > Subject: Re: RFE 4519026: (process) Process should support Unicode on > Win NT, request for review > > > Sorry for the delayed response. I've been busy. > (Probably I should not have volunteered for this review.) > > Heiko, thanks for the patch. > > JDK engineers (Xueming, Alan?) will need to help > with testing, architectural issues, and shepherding. > I no longer use windows. > > I approve of the general approach being taken here. > We need a general purpose version of JVM_NativePath, > as you have coded it up, but it needs to go into some shared location > for use by other JDK native code - not sure quite where that would be. > I guess until we have another client for it, living > in ProcessImpl_md.c is not so bad. > > Make sure you run the java.lang regression tests, especially > ProcessBuilder/Basic.java > > There's a fair bit of cleanup that will be required. > Use of white space and commenting style are non-standard > and will need to be fixed (even if you've copied them > from other parts of the JDK) > > I think the spec for free() guarantees that free(null) is a no-op, > and the JDK already relies on this, so no need for > releaseStringCopy. > > Typos/suggestions: > > path.md.c => path_md.c > > *(res + len) = 0; => res[len] = L'\0'; > > Martin > > > On Mon, Mar 9, 2009 at 02:57, Heiko Wagner <heiko.wag...@apis.de> wrote: >> This is related to my previous post at >> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-February/001145.ht >> ml and my first contribution to the JDK7 project. As Martin suggested, I >> have worked on a wide char version of ProcessImpl_md.c. For discussion of > my >> proposed chages a copy of my source can be found at: >> >> http://www.apis.de/pub/jdk7/ProcessImpl_md.c >> >> and a diff at: >> >> http://www.apis.de/pub/jdk7/ProcessImpl_md.c.diff >> >> This patch enables launching executables residing on a path containing >> non-ansi characters. My next goal is to get the java launcher working on a >> unicode path. I think this needs additional coordination with the hotspot >> team, since some of the code in os.cpp also has issues in a unicode path >> when loading verify.dll and java.dll. >> >> Regards >> Heiko >> >> > >
