RFR(XS): 8215966: GeneratePropertyPassword.sh uses bash syntax (was Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests)

[Hohensee, Paul]

Send a patch directly to me (hohensee at amazon.com<mailto:hohensee at amazon.com>) and I’ll file an RFE and post a webrev so you can ask for a review.

Thanks,

Paul

From: Sergei Ustimenko <merkel05 at gmail.com>
Date: Friday, December 28, 2018 at 10:55 AM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: serviceability-dev <serviceability-dev at openjdk.java.net>, Martin Buchholz <martinrb at google.com>
Subject: Re: RFR(XS): 8215966: GeneratePropertyPassword.sh uses bash syntax (was Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests)

Hi Paul,

Thanks for taking care of this and for your time!

Agree, initial patch was mainly about forward porting
lost changes as you've just mentioned.

Regarding filling another RFE as I don't have a JBS account it
would be a bit of a trouble for me I guess. I could try my best at
https://bugreport.java.com/bugreport/ though I guess that's
not the right place for it. So if I may borrow a bit more of your
time, could you please help me with filling the RFE? I guess missing
shebang is not critical, but might lead to a nasty situation.

Thanks,
Sergei

On Fri, 28 Dec 2018 at 19:05, Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com>> wrote:
I’ll sponsor it. I’ve filed JDK-8215966<https://bugs.openjdk.java.net/browse/JDK-8215966> for it: it’s a simple forward port.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215966
Webrev: http://cr.openjdk.java.net/~phh/8215966/webrev.00/

While I agree with Martin that we might as well specify bash, imo in this specific case it’s better to do a forward port since the code already exists in JDK9. The webrev therefore dups the JDK-8025886<https://bugs.openjdk.java.net/browse/JDK-8025886> patch. If we want to make scripts explicitly depend on bash, I’d prefer to see a separate RFE for it.

Lgtm, so that’s one review. May we have another please?

Thanks,

Paul

From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net<mailto:serviceability-dev-bounces at openjdk.java.net>> on behalf of Sergei Ustimenko <merkel05 at gmail.com<mailto:merkel05 at gmail.com>>
Date: Friday, December 28, 2018 at 8:40 AM
To: serviceability-dev <serviceability-dev at openjdk.java.net<mailto:serviceability-dev at openjdk.java.net>>
Subject: Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

Hi,

Could anyone please help with review?

Regards,
Sergei

On Sat, 22 Dec 2018 at 17:09, Sergei Ustimenko <merkel05 at gmail.com<mailto:merkel05 at gmail.com>> wrote:
Hi,

Could anyone please review and sponsor
this small patch to add a shebang line to
the test script?

diff --git a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
--- a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
+++ b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
@@ -1,3 +1,5 @@
+#!/bin/bash
+
 #
 # Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Thanks,
Sergei

On Wed, Dec 19, 2018, 21:04 Sergei Ustimenko <merkel05 at gmail.com<mailto:merkel05 at gmail.com> wrote:
Quickfix on my previous mail: I've found no scripts
with the same problem, except this one in the patch.

>Yeah, let's fix !

Perfect!

I will just need some help with sponsorship to push this.
I've signed the OCA, so no problems from my side.

Regards,
Sergei

On Wed, 19 Dec 2018 at 20:22, Martin Buchholz <martinrb at google.com<mailto:martinrb at google.com>> wrote:
Did we really have shell scripts without a shebang line?
Yeah, let's fix !

On Wed, Dec 19, 2018 at 11:03 AM Sergei Ustimenko <merkel05 at gmail.com<mailto:merkel05 at gmail.com>> wrote:
HI Martin,

As you've suggested I've simply added bash's shebang.
It wouldn't add any problem since, as David have mentioned,
no bash - no build. I've also quickly checked for similar cases
and found one.

An updated patch is below.

diff --git a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
--- a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
+++ b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
@@ -1,3 +1,5 @@
+#!/bin/bash
+
 #
 # Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.


Regards,
Sergei

On Mon, 10 Dec 2018 at 22:27, Martin Buchholz <martinrb at google.com<mailto:martinrb at google.com>> wrote:
I don't know if there's an official policy on how ultra-portable tests are supposed to be.  In practice, you probably won't be able to build openjdk on a system without bash.

On Mon, Dec 10, 2018 at 1:12 PM Sergei Ustimenko <merkel05 at gmail.com<mailto:merkel05 at gmail.com>> wrote:
Hi Martin,

That sounds good!

I've counted all the sh-shebangs and it appears that
there are at least 66 of them inside the test/ directory,
where only 12 bashes.

I've also ran the search  in order to identify all the
occurrences that use either [[ or == and found only
three of them that use "==". That one for example:
http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63
of course `dash` reports failure in that case.

So I'm quite hesitant in that case and not really sure
what to do. I haven't also found any existent JBS ticket
for such /bin/sh => /bin/bash a replacement.

So any advise in this case would be appreciated!

Regards,
Sergei

On Mon, 10 Dec 2018 at 18:32, Martin Buchholz <martinrb at google.com<mailto:martinrb at google.com>> wrote:
I would not try to remove all bash-isms from openjdk. Instead I would find instances of /bin/sh that need to be changed to /bin/bash.

(Ubuntu's use of /bin/sh -> /bin/dash is technically correct, but caused much suffering
https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181228/e9e3d290/attachment-0001.html>