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

Fri Dec 28 20:14:53 UTC 2018

Hi,

I've checked if there are scripts (*.sh) that miss shebang and
found some (~170 in test/). As an example those:

...
test/jdk/java/lang/instrument/StressGetObjectSizeTest.sh
test/jdk/java/lang/instrument/RedefineClassWithNativeMethod.sh
test/jdk/java/lang/instrument/RedefineMethodWithAnnotations.sh
test/jdk/java/io/File/GetXSpace.sh
test/jdk/java/rmi/activation/Activatable/extLoadedImpl/ext.sh
test/jdk/java/util/Formatter/Basic.sh
test/jdk/java/util/zip/3GBZipFiles.sh
test/jdk/tools/launcher/6842838/Test6842838.sh
test/jdk/tools/jjs/common.sh
...
test/jdk/sun/rmi/rmic/covariantReturns/run.sh
test/jdk/sun/rmi/rmic/manifestClassPath/Util.sh
test/jdk/sun/rmi/rmic/oldjavacRemoved/sunToolsJavacMain.sh
test/jdk/sun/management/jmxremote/RunTest.sh
test/jdk/sun/management/jmxremote/bootstrap/RmiBootstrapTest.sh
test/jdk/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh
test/jdk/sun/management/jmxremote/bootstrap/RmiSslNoKeyStoreTest.sh
test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
test/jdk/sun/net/sdp/sanity.sh
test/jdk/sun/jvmstat/testlibrary/utils.sh
...

Some of them are tests others are utility scripts. I wonder what we
can do in that case? Can we fill an RFE? Relying on default here might lead
to a nasty situations in future, as it was with Ubuntu sh -> dash.

As a small remark all the scripts that miss shebang are located in the
test/ dir.

Regards,
Sergei

On Fri, 28 Dec 2018 at 20:01, Hohensee, Paul <hohensee at amazon.com> wrote:

> Send a patch directly to me (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> 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>
> on behalf of Sergei Ustimenko <merkel05 at gmail.com>
> *Date: *Friday, December 28, 2018 at 8:40 AM
> *To: *serviceability-dev <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> 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 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> 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>
> 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> 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>
> 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> 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/947576df/attachment-0001.html>