[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests
From: Sergei Ustimenko <merkel05 () gmail ! com>
Date: 2018-12-28 16:39:33
Message-ID: CAPpfMiU67YtB-bHq6=k-unvu9aXh_WOX-7xRQNp69ratrQA01Q () mail ! gmail ! com
[Download RAW message or body]
Hi,
Could anyone please help with review?
Regards,
Sergei
On Sat, 22 Dec 2018 at 17:09, Sergei Ustimenko <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 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>
> > 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>
> > > 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>
> > > > 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>
> > > > > 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>
> > > > > > 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
> > > > > > > )
> > > > > > >
> > > > > >
[Attachment #3 (text/html)]
<div dir="ltr"><div>Hi,</div><div><br></div><div>Could anyone please help with \
review?</div><div><br></div><div>Regards,</div><div>Sergei<br></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Sat, 22 Dec 2018 at 17:09, Sergei Ustimenko \
<<a href="mailto:merkel05@gmail.com">merkel05@gmail.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div \
dir="auto">Hi,<div dir="auto"><br></div><div dir="auto">Could anyone please review \
and sponsor</div><div dir="auto">this small patch to add a shebang line to \
</div><div dir="auto">the test script?</div><div dir="auto"><br></div><div \
dir="auto"><span style="font-family:sans-serif">diff --git \
a/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh \
b/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">--- \
a/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">+++ \
b/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">@@ -1,3 +1,5 \
@@</span><br style="font-family:sans-serif"><span \
style="font-family:sans-serif">+#!/bin/bash</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">+</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> #</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> # Copyright (c) \
2003, 2018, Oracle and/or its affiliates. All rights reserved.</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> # DO NOT ALTER \
OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.</span><br></div><div \
dir="auto"><span style="font-family:sans-serif"><br></span></div><div \
dir="auto"><span style="font-family:sans-serif">Thanks,</span></div><div \
dir="auto"><span style="font-family:sans-serif">Sergei</span></div><div \
dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr">On Wed, Dec 19, \
2018, 21:04 Sergei Ustimenko <<a href="mailto:merkel05@gmail.com" \
target="_blank">merkel05@gmail.com</a> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Quickfix on my previous mail: \
I've found no scripts</div><div>with the same problem, except this one in the \
patch.<br></div><div><br></div><div>>Yeah, let's fix \
!</div><div><br></div><div>Perfect!</div><div><br></div><div>I will just need some \
help with sponsorship to push this.<br></div><div>I've signed the OCA, so no \
problems from my side.<br></div><div><br></div><div>Regards,</div><div>Sergei<br></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Wed, 19 Dec 2018 at 20:22, Martin Buchholz \
<<a href="mailto:martinrb@google.com" rel="noreferrer" \
target="_blank">martinrb@google.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr">Did we really have shell scripts \
without a shebang line?<div>Yeah, let's fix !</div></div><br><div \
class="gmail_quote"><div dir="ltr">On Wed, Dec 19, 2018 at 11:03 AM Sergei Ustimenko \
<<a href="mailto:merkel05@gmail.com" rel="noreferrer" \
target="_blank">merkel05@gmail.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>HI \
Martin,</div><div><br></div><div>As you've suggested I've simply added \
bash's shebang.</div><div>It wouldn't add any problem since, as David have \
mentioned,</div><div>no bash - no build. I've also quickly checked for similar \
cases <br></div><div>and found one.<br></div><div><br></div><div>An updated patch is \
below.</div><div><br></div><div><span style="font-family:sans-serif">diff --git \
a/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh \
b/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">--- \
a/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">+++ \
b/test/jdk/sun/management/</span><span \
style="font-family:sans-serif">jmxremote/bootstrap/</span><span \
style="font-family:sans-serif">GeneratePropertyPassword.sh</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">@@ -1,3 +1,5 \
@@</span><br style="font-family:sans-serif"><span \
style="font-family:sans-serif">+#!/bin/bash</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif">+</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> #</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> # Copyright (c) \
2003, 2018, Oracle and/or its affiliates. All rights reserved.</span><br \
style="font-family:sans-serif"><span style="font-family:sans-serif"> # DO NOT ALTER \
OR REMOVE COPYRIGHT NOTICES OR THIS FILE \
HEADER.</span><br></div><div><br></div><div><br></div><div>Regards,</div><div>Sergei<br></div></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Mon, 10 Dec 2018 at 22:27, Martin Buchholz \
<<a href="mailto:martinrb@google.com" rel="noreferrer" \
target="_blank">martinrb@google.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.</div><br><div \
class="gmail_quote"><div dir="ltr">On Mon, Dec 10, 2018 at 1:12 PM Sergei Ustimenko \
<<a href="mailto:merkel05@gmail.com" rel="noreferrer" \
target="_blank">merkel05@gmail.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">Hi \
Martin,<div><br></div><div>That sounds good!</div><div><br></div><div>I've \
counted all the sh-shebangs and it appears that</div><div>there are at least 66 of \
them inside the test/ directory,</div><div>where only 12 \
bashes.</div><div><br></div><div>I've also ran the search in order to identify \
all the </div><div>occurrences that use either [[ or == and found \
only</div><div>three of them that use "==". That one for \
example:</div><div><a \
href="http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63" \
rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63</a></div><div>of \
course `dash` reports failure in that case.</div><div><br></div><div>So I'm quite \
hesitant in that case and not really sure</div><div>what to do. I haven't also \
found any existent JBS ticket</div><div>for such /bin/sh => /bin/bash a \
replacement.</div><div><br></div><div>So any advise in this case would be \
appreciated!</div><div><br></div><div>Regards,</div><div>Sergei \
</div></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, 10 Dec 2018 at \
18:32, Martin Buchholz <<a href="mailto:martinrb@google.com" rel="noreferrer" \
target="_blank">martinrb@google.com</a>> wrote:<br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">I would not try to \
remove all <span class="gmail-m_-3692218182541013287m_4995032525543241182gmail-m_-757 \
7016553615907804gmail-m_-3747258800265171587gmail-m_-7076505635983534101gmail-m_3397460288199466450gmail-m_1537696570238721258gmail-il">bash</span>-isms \
from openjdk. Instead I would find instances of /bin/sh that need to be changed to \
/bin/<span class="gmail-m_-3692218182541013287m_4995032525543241182gmail-m_-7577016553 \
615907804gmail-m_-3747258800265171587gmail-m_-7076505635983534101gmail-m_3397460288199 \
466450gmail-m_1537696570238721258gmail-il">bash</span>.<div><br></div><div>(Ubuntu's \
use of /bin/sh -> /bin/dash is technically correct, but caused much \
suffering</div><div><a \
href="https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463" rel="noreferrer" \
target="_blank">https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463</a><br></div><div>)</div></div></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div></div>
</blockquote></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic