Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v4]

[Dan Lutker]

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

Dan Lutker has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix path exists check

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17798/files
  - new: https://git.openjdk.org/jdk/pull/17798/files/1df8876c..3464b664

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17798=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=17798=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17798.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17798/head:pull/17798

PR: https://git.openjdk.org/jdk/pull/17798

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v3]

[Dan Lutker]

On Tue, 13 Feb 2024 22:02:15 GMT, Roger Riggs  wrote:

>> Any reason not to use 
>> [exesanity_SimpleNativeLauncher](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/native_sanity/simplenativelauncher/exesanity_SimpleNativeLauncher.c)
>>  or 
>> [exeBasicSleep](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/java/lang/ProcessBuilder/exeBasicSleep.c)
>>  in all cases?
>
> I'd be ok with exeBasicSleep; (currently only used on Windows and if its been 
> built by the make files).
> Note the discovery mechanism used in Basic.java `initSleepPath()` to locate 
> and identify the path where its built or to fallback to the OS sleep.

I opted to just check for coreutils existing as a signal that it was single 
executable. With Fedora and AL2023 coreutils-single package this will work. 
Some other distro could choose to use the symlink version of this or exclude 
sleep from coreutils-single in the future, but we can deal with that if it 
every actually comes up.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1525573046

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v3]

[Dan Lutker]

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

Dan Lutker has updated the pull request incrementally with two additional 
commits since the last revision:

 - Remove extra whitespace
 - Simplify check for coreutils single executable

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17798/files
  - new: https://git.openjdk.org/jdk/pull/17798/files/3f42d85e..1df8876c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17798=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=17798=01-02

  Stats: 17 lines in 2 files changed: 0 ins; 12 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/17798.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17798/head:pull/17798

PR: https://git.openjdk.org/jdk/pull/17798

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Andrey Turbanov]

On Tue, 13 Feb 2024 18:03:14 GMT, Dan Lutker  wrote:

>> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
>> package and no coreutils executable as well as AmazonLinux 2023 that uses 
>> `--enable-single-binary`
>
> Dan Lutker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove unnecessary var.

test/lib/jdk/test/lib/Platform.java line 140:

> 138: }
> 139: }
> 140: } catch(Exception e) {

Suggestion:

} catch (Exception e) {

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1489133848

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Roger Riggs]

On Tue, 13 Feb 2024 21:23:29 GMT, Dan Lutker  wrote:

>> At the time `sleep` seemed ubiquitous and was native so it ran quickly. 
>> (Much quicker than running java).
>> Running another program would be fine.  For example ,selecting an executable 
>> based on the OS type and giving the expected string in the output would be 
>> preferred.
>> The isBusyBox check is also a bad hack, but lighter weight since its not 
>> executing a program.
>> As the number of minor variations of platforms appear a more general 
>> approach would be useful.
>> 
>> I'd code it all in the test itself, doing a lookup based on the operating 
>> system name with the respective executable and expected string in the info 
>> arguments[0].
>> (With a fallback to sleep, so the table does not have to list every os).
>
> Any reason not to use 
> [exesanity_SimpleNativeLauncher](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/native_sanity/simplenativelauncher/exesanity_SimpleNativeLauncher.c)
>  or 
> [exeBasicSleep](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/java/lang/ProcessBuilder/exeBasicSleep.c)
>  in all cases?

I'd be ok with exeBasicSleep; (currently only used on Windows and if its been 
built by the make files).
Note the discovery mechanism used in Basic.java `initSleepPath()` to locate and 
identify the path where its built or to fallback to the OS sleep.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488640847

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Dan Lutker]

On Tue, 13 Feb 2024 19:23:50 GMT, Roger Riggs  wrote:

>>> what's the output for `coreutils --help` for a single executable binary?
>> 
>> 
>> # coreutils --help
>> Usage: coreutils --coreutils-prog=PROGRAM_NAME [PARAMETERS]... 
>> Execute the PROGRAM_NAME built-in program with the given PARAMETERS.
>> 
>>   --help display this help and exit
>>   --version  output version information and exit
>> 
>> Built-in programs:
>>  [ arch b2sum base32 base64 basename basenc cat chcon chgrp chmod chown 
>> chroot cksum comm cp csplit cut date dd df dir dircolors dirname du echo env 
>> expand expr factor false fmt fold ginstall groups head hostid id join link 
>> ln logname ls md5sum mkdir mkfifo mknod mktemp mv nice nl nohup nproc numfmt 
>> od paste pathchk pinky pr printenv printf ptx pwd readlink realpath rm rmdir 
>> runcon seq sha1sum sha224sum sha256sum sha384sum sha512sum shred shuf sleep 
>> sort split stat stdbuf stty sum sync tac tail tee test timeout touch tr true 
>> truncate tsort tty uname unexpand uniq unlink users vdir wc who whoami yes
>> 
>> Use: 'coreutils --coreutils-prog=PROGRAM_NAME --help' for individual program 
>> help.
>> 
>> GNU coreutils online help: 
>> Report any translation bugs to 
>> Full documentation 
>> or available locally via: info '(coreutils) Multi-call invocation'
>> 
>> 
>> This test is about parameter checking, so I am inclined to agree with @ecki 
>> and not check the process name or change the exe.
>
> At the time `sleep` seemed ubiquitous and was native so it ran quickly. (Much 
> quicker than running java).
> Running another program would be fine.  For example ,selecting an executable 
> based on the OS type and giving the expected string in the output would be 
> preferred.
> The isBusyBox check is also a bad hack, but lighter weight since its not 
> executing a program.
> As the number of minor variations of platforms appear a more general approach 
> would be useful.
> 
> I'd code it all in the test itself, doing a lookup based on the operating 
> system name with the respective executable and expected string in the info 
> arguments[0].
> (With a fallback to sleep, so the table does not have to list every os).

Any reason not to use 
[exesanity_SimpleNativeLauncher](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/native_sanity/simplenativelauncher/exesanity_SimpleNativeLauncher.c)
 or 
[exeBasicSleep](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/java/lang/ProcessBuilder/exeBasicSleep.c)
 in all cases?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488594104

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Roger Riggs]

On Tue, 13 Feb 2024 18:24:10 GMT, Dan Lutker  wrote:

>> To be fair, this looks similar to what `isMusl()` does: searching for 
>> strings in outputs. But I do wonder if `--help` would include "sleep" for 
>> some innocuous description somewhere, and this code would accidentally 
>> match. @lutkerd, what's the output for `coreutils --help` for a single 
>> executable binary?
>
>> what's the output for `coreutils --help` for a single executable binary?
> 
> 
> # coreutils --help
> Usage: coreutils --coreutils-prog=PROGRAM_NAME [PARAMETERS]... 
> Execute the PROGRAM_NAME built-in program with the given PARAMETERS.
> 
>   --help display this help and exit
>   --version  output version information and exit
> 
> Built-in programs:
>  [ arch b2sum base32 base64 basename basenc cat chcon chgrp chmod chown 
> chroot cksum comm cp csplit cut date dd df dir dircolors dirname du echo env 
> expand expr factor false fmt fold ginstall groups head hostid id join link ln 
> logname ls md5sum mkdir mkfifo mknod mktemp mv nice nl nohup nproc numfmt od 
> paste pathchk pinky pr printenv printf ptx pwd readlink realpath rm rmdir 
> runcon seq sha1sum sha224sum sha256sum sha384sum sha512sum shred shuf sleep 
> sort split stat stdbuf stty sum sync tac tail tee test timeout touch tr true 
> truncate tsort tty uname unexpand uniq unlink users vdir wc who whoami yes
> 
> Use: 'coreutils --coreutils-prog=PROGRAM_NAME --help' for individual program 
> help.
> 
> GNU coreutils online help: 
> Report any translation bugs to 
> Full documentation 
> or available locally via: info '(coreutils) Multi-call invocation'
> 
> 
> This test is about parameter checking, so I am inclined to agree with @ecki 
> and not check the process name or change the exe.

At the time `sleep` seemed ubiquitous and was native so it ran quickly. (Much 
quicker than running java).
Running another program would be fine.  For example ,selecting an executable 
based on the OS type and giving the expected string in the output would be 
preferred.
The isBusyBox check is also a bad hack, but lighter weight since its not 
executing a program.
As the number of minor variations of platforms appear a more general approach 
would be useful.

I'd code it all in the test itself, doing a lookup based on the operating 
system name with the respective executable and expected string in the info 
arguments[0].
(With a fallback to sleep, so the table does not have to list every os).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488468243

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Dan Lutker]

On Tue, 13 Feb 2024 18:19:48 GMT, Aleksey Shipilev  wrote:

> what's the output for `coreutils --help` for a single executable binary?


# coreutils --help
Usage: coreutils --coreutils-prog=PROGRAM_NAME [PARAMETERS]... 
Execute the PROGRAM_NAME built-in program with the given PARAMETERS.

  --help display this help and exit
  --version  output version information and exit

Built-in programs:
 [ arch b2sum base32 base64 basename basenc cat chcon chgrp chmod chown chroot 
cksum comm cp csplit cut date dd df dir dircolors dirname du echo env expand 
expr factor false fmt fold ginstall groups head hostid id join link ln logname 
ls md5sum mkdir mkfifo mknod mktemp mv nice nl nohup nproc numfmt od paste 
pathchk pinky pr printenv printf ptx pwd readlink realpath rm rmdir runcon seq 
sha1sum sha224sum sha256sum sha384sum sha512sum shred shuf sleep sort split 
stat stdbuf stty sum sync tac tail tee test timeout touch tr true truncate 
tsort tty uname unexpand uniq unlink users vdir wc who whoami yes

Use: 'coreutils --coreutils-prog=PROGRAM_NAME --help' for individual program 
help.

GNU coreutils online help: 
Report any translation bugs to 
Full documentation 
or available locally via: info '(coreutils) Multi-call invocation'


This test is about parameter checking, so I am inclined to agree with @ecki and 
not check the process name or change the exe.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488380807

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Aleksey Shipilev]

On Tue, 13 Feb 2024 18:15:59 GMT, Dan Lutker  wrote:

>> Just ignore the binary name and only check for argv1 is imho enough.
>
> I couldn't think if a better way to detect what the host system had and 
> adding this seemed inline with the busybox check. 
> 
> The choice of running `sleep` seems like it was arbitrary and the test should 
> use something that is under JDK control rather than a platform specific tool 
> that can change from distro to distro. Is there any reason we don't use the 
> `java`, something else in the image, or even something built as part of the 
> test-image.

To be fair, this looks similar to what `isMusl()` does: searching for strings 
in outputs. But I do wonder if `--help` would include "sleep" for some 
innocuous description somewhere, and this code would accidentally match. 
@lutkerd, what's the output for `coreutils --help` for a single executable 
binary?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488374538

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Dan Lutker]

On Tue, 13 Feb 2024 18:08:03 GMT, Bernd  wrote:

>> test/lib/jdk/test/lib/Platform.java line 145:
>> 
>>> 143: }
>>> 144: 
>>> 145: public static boolean isOSX() {
>> 
>> This seems like a real hack. Is there really no better way to identity how 
>> sleep is implemented.
>> I'd probably favor an alternative that accepts multiple values for the 
>> expected value.
>
> Just ignore the binary name and only check for argv1 is imho enough.

I couldn't think if a better way to detect what the host system had and adding 
this seemed inline with the busybox check. 

The choice of running `sleep` seems like it was arbitrary and the test should 
use something that is under JDK control rather than a platform specific tool 
that can change from distro to distro. Is there any reason we don't use the 
`java`, something else in the image, or even something built as part of the 
test-image.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488368949

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Bernd]

On Tue, 13 Feb 2024 17:54:43 GMT, Roger Riggs  wrote:

>> Dan Lutker has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove unnecessary var.
>
> test/lib/jdk/test/lib/Platform.java line 145:
> 
>> 143: }
>> 144: 
>> 145: public static boolean isOSX() {
> 
> This seems like a real hack. Is there really no better way to identity how 
> sleep is implemented.
> I'd probably favor an alternative that accepts multiple values for the 
> expected value.

Just ignore the binary name and only check for argv1 is imho enough.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488357647

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Dan Lutker]

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

Dan Lutker has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove unnecessary var.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17798/files
  - new: https://git.openjdk.org/jdk/pull/17798/files/986d35f5..3f42d85e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17798=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17798=00-01

  Stats: 7 lines in 1 file changed: 0 ins; 6 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17798.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17798/head:pull/17798

PR: https://git.openjdk.org/jdk/pull/17798

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary

[Roger Riggs]

On Fri, 9 Feb 2024 23:40:00 GMT, Dan Lutker  wrote:

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

test/lib/jdk/test/lib/Platform.java line 145:

> 143: }
> 144: 
> 145: public static boolean isOSX() {

This seems like a real hack. Is there really no better way to identity how 
sleep is implemented.
I'd probably favor an alternative that accepts multiple values for the expected 
value.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488338803

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary

[Dan Lutker]

On Tue, 13 Feb 2024 10:11:36 GMT, Aleksey Shipilev  wrote:

>> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
>> package and no coreutils executable as well as AmazonLinux 2023 that uses 
>> `--enable-single-binary`
>
> test/jdk/java/lang/ProcessHandle/InfoTest.java line 321:
> 
>> 319: String[] args = info.arguments().get();
>> 320: if (args.length > 0) {
>> 321: if (timeIsLastParam) {
> 
> I gotta ask: is it true that time argument is the last param in _all_ testing 
> modes? If so, we don't need `timeIsLastParam` at all?

Hah, yes. I was trying too hard to not change the default behavior.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488259691

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary

[Aleksey Shipilev]

On Fri, 9 Feb 2024 23:40:00 GMT, Dan Lutker  wrote:

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

test/jdk/java/lang/ProcessHandle/InfoTest.java line 321:

> 319: String[] args = info.arguments().get();
> 320: if (args.length > 0) {
> 321: if (timeIsLastParam) {

I gotta ask: is it true that time argument is the last param in _all_ testing 
modes? If so, we don't need `timeIsLastParam` at all?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1487540871