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 <hohen...@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-boun...@openjdk.java.net> > on behalf of Sergei Ustimenko <merke...@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 <merke...@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 <merke...@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 <marti...@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 <merke...@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 <marti...@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 <merke...@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 <marti...@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 > > ) > >
