RFE 4519026: (process) Process should support Unicode on Win NT, request for review

[Martin Buchholz]

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.wagner at 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:martinrb at google.com]
> Sent: Dienstag, 17. März 2009 02:32
> To: Heiko Wagner
> Cc: core-libs-dev at 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.wagner at 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
>>
>>
>
>