[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(XS): 8215966: GeneratePropertyPassword.sh uses bash syntax (was Re: [PATCH] JDK-8025886: Rep
From: "Hohensee, Paul" <hohensee () amazon ! com>
Date: 2018-12-28 23:20:42
Message-ID: 186DA688-7AAA-49C9-9CAF-BE82593BBAA7 () amazon ! com
[Download RAW message or body]
The change is Sergei's original proposal (not included in this message), but I'll \
take what I can get. :) Pushed.
Thanks,
Paul
On 12/28/18, 1:44 PM, "David Holmes" <david.holmes@oracle.com> wrote:
Hi Paul,
On 29/12/2018 4:05 am, Hohensee, Paul 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.
To be clear, it isn't that this change didn't make it into post 9, the
change was reverted by JDK-8192953 in June 2018.
> 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.
The patch looks good to me. Reviewed.
> Lgtm, so that's one review. May we have another please?
Point of order: the change proposed is not the change that Sergei
requested a review of. This is new code proposed by you, so you can't
review your own contribution.
But I've Reviewed it, and Sergei is okay with it too so that's a
"r"eview and so it is good to go.
Thanks,
David
>
> Thanks,
>
> Paul
>
> *From: *serviceability-dev <serviceability-dev-bounces@openjdk.java.net>
> on behalf of Sergei Ustimenko <merkel05@gmail.com>
> *Date: *Friday, December 28, 2018 at 8:40 AM
> *To: *serviceability-dev <serviceability-dev@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@gmail.com
> <mailto:merkel05@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@gmail.com
> <mailto:merkel05@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@google.com <mailto:martinrb@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@gmail.com <mailto:merkel05@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@google.com <mailto:martinrb@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@gmail.com <mailto:merkel05@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@google.com
> <mailto:martinrb@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 >
> )
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic