Thank you Roger for looking at this.

On 27.02.2014 2:19, roger riggs wrote:
Hi Ivan,

A few comments on UnixCommands:

- The trailing "_" on the method names looks very odd, they could all use some suffix "Pgm" "X", etc.

I replaced true_() and false_() with explicit calls to findCommand().

 - I'm not sure the specific command methods do you any good
(over a method with a string argument). since they immediately revert to the string command name.

They are shorter :-)

- The final "0" in findCommand0 is using the convention for a native method but it is not native.

Yes. But I think the convention is more general -- the f0() is usually a function of a lower level than f().
Grepping shows some examples:
./jdk/share/classes/java/security/KeyStore.java: run0() is called from run()
./jdk/share/classes/java/util/regex/Pattern.java: match0() is called from match() and In the tests, main0() sometimes is called from main(), test0() from test(), etc.

No problem to rename it, but I don't think it should confuse too much.

- Use the File methods for concatenation rather than string concatenation.
   i.e. new File (parent, child).

Good point, thanks!

Please find the updated webrev here:
http://cr.openjdk.java.net/~igerasim/6943190/2/webrev/

Sincerely yours,
Ivan


Roger




On 2/26/2014 4:30 PM, Ivan Gerasimov wrote:

On 24.02.2014 23:38, Martin Buchholz wrote:
Thanks for working on these ancient Process tests.
I would prefer to see them using "feature tests".

You wanna do "cat /dev/zero"? Then look for cat in /bin and /usr/bin and check for /dev/zero as well, e.g. using File.isExecutable

OK. Here's another iteration.
I introduced a utility class UnixCommands, which encapsulates searching for the command under several fixed directories.

I didn't touch /dev/null though. I think we can assume it exists in every Unix system. Many other tests depend on it anyways.

Would you please have a chance to review the updated wevrev?
http://cr.openjdk.java.net/~igerasim/6943190/1/webrev/

Sincerely yours,
Ivan

Doing OS-specific things like:

+    static final boolean isLinux =
+ System.getProperty("os.name <http://os.name>", "unknown").startsWith("Linux");

should be a last resort.


On Mon, Feb 24, 2014 at 9:26 AM, Ivan Gerasimov <ivan.gerasi...@oracle.com <mailto:ivan.gerasi...@oracle.com>> wrote:

    Hello!

    We've got one quite old report about been unable to run the test
    under Android.
    https://bugs.openjdk.java.net/browse/JDK-6943190

    That was due to hard-coded path to the cat utility as /bin/cat.
    When investigating, I found two other tests under the same
    directory that use /usr/bin/cat and /usr/bin/echo.
These two test seem to (almost) never run because of the unusual path.

    Here's the first version of the webrev, with the fix to only three
    tests mentioned above:
    http://cr.openjdk.java.net/~igerasim/6943190/0/webrev/
<http://cr.openjdk.java.net/%7Eigerasim/6943190/0/webrev/>

    java/lang/Runtime/exec/ directory has also got several other
    tests, which run shell commands.
    Some of them have absolute path and some of them don't.

    So I have a general question: Why would we ever need the absolute
    path for the commands?
    Why wouldn't we rely on the PATH env variable?

    Sincerely yours,
    Ivan








Reply via email to