Page MenuHomePhabricator

Bashisms in various /bin/sh scripts
Closed, ResolvedPublic

Description

I recently stumbled upon checkbashisms in devscripts and ran it on the operations/puppet repository:

modules/vrts/files/upgrade_vrts.sh
line 8 (alternative test command ([[ foo ]] should be [ foo ])): if [[ $? -ne 0 ]]; then
line 9 (alternative test command ([[ foo ]] should be [ foo ])): if [[ $? -ne 0 ]]; then
modules/profile/files/kerberos/client/kerberos_ticket_info.sh
line 7 (echo -e):    echo -e '\nFound a valid Kerberos ticket in the credential cache:'
line 10 (echo -e): echo -e '\nYou do not have a valid Kerberos ticket in the credential cache, remember to kinit.'
modules/profile/files/kerberos/client/kerberos_autorenew.sh
line 8 ('function' is useless): function autorenew_is_active {
line 12 ('function' is useless): function create_autorenew_timer {
line 13 (echo -e):  echo -e "\nCreating automatic Kerberos ticket renewal service"
line 23 (echo -e):  echo -e '\nYou have a valid Kerberos ticket.'
line 24 (echo -e):  autorenew_is_active && echo -e 'Your automatic Kerberos ticket renewal service is also active on this host\n' || create_autorenew_timer
line 26 (echo -e):  echo -e '\nYou do not have a valid Kerberos ticket in the credential cache, remember to kinit.'
modules/profile/files/monitoring/check_newest_file_age.sh
line 229 ([^] should be [!]): if [ `echo "$warning" | grep [^0-9]` ] || [ ! "$warning" ]; then
line 234 ([^] should be [!]): if [ `echo "$critical" | grep [^0-9]` ] || [ ! "$critical" ]; then
modules/install_server/files/autoinstall/scripts/reuse-parts.sh
line 268 (should be 'b = a'):  [ "$num" == "-1" ] && continue
modules/admin/files/home/jhathaway/.profile
line 7 ($BASH_SOMETHING): if [ -n "$BASH_VERSION" ]; then
modules/kubeadm/files/kubectl-sudo.sh
line 37 (read with option other than -r):  read -p "WARNING: Currently in context $(kubectl config current-context). Continue? (y/N) " yn

It is very unlikely that on a Linux system /bin/sh will not be bash :-), and some of the constructs (especially "${0##*/}" = "bash" -a "$_" = "$0") look like they were written by someone who knew what he was doing, but if someone wants to climb down the rabbit hole of shell compatibility (or just replace #!/bin/sh with #!/bin/bash), you're welcome :-).

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
JHathawayoperations/puppetproduction+3 -0
JHathawayoperations/puppetproduction+4 -2
Btullisoperations/puppetproduction+18 -33
Jbondoperations/puppetproduction+33 -18
Jbondoperations/puppetproduction+8 -10
Jbondoperations/puppetproduction+0 -0
Jbondoperations/puppetproduction+1 -1
Jbondoperations/puppetproduction+20 -23
Jbondoperations/puppetproduction+5 -5
Jbondoperations/puppetproduction+13 -9
Jbondoperations/puppetproduction+3 -3
Jbondoperations/puppetproduction+9 -9
Dzahnoperations/puppetproduction+3 -3
Dzahnoperations/puppetproduction+5 -4
Dzahnoperations/puppetproduction+1 -0
Dzahnoperations/puppetproduction+2 -2
Dzahnoperations/puppetproduction+2 -2
Dzahnoperations/puppetproduction+7 -7
Dzahnoperations/puppetproduction+2 -1
Dzahnoperations/puppetproduction+3 -0
Emaoperations/puppetproduction+11 -9
Jcrespooperations/puppetproduction+1 -1
Show related patches Customize query in gerrit

Event Timeline

scfc raised the priority of this task from to Lowest.
scfc updated the task description. (Show Details)
scfc added a project: Puppet.
scfc subscribed.
scfc raised the priority of this task from Lowest to Low.Apr 13 2015, 11:34 AM

"It is very unlikely that on a Linux system /bin/sh will not be bash :-)" ­­— well, except for that that I ran those tests on, and every other instance in Labs, and probably all Ubuntu and Debian systems. ls -l /bin/sh would have been very useful to execute before stating something obviously untrue.

So this needs to be tackled some time, but if the scripts are working at the moment, they are unlikely to fail tomorrow.

Change 316529 had a related patch set uploaded (by Ema):
Fix bashisms

https://gerrit.wikimedia.org/r/316529

We should also add a CI test, otherwise these will inevitably creep back in over time...

@scfc note at least some of those are 3rd party, and we may (not sure yet) decide to keep compatibility with upstream, and it should be reported there anyway. That is at least true for the mysql one. Independently of the decision, it should be reported upstream anyway.

Change 316532 had a related patch set uploaded (by Jcrespo):
mariadb: Fix bashism on eventlogging.init

https://gerrit.wikimedia.org/r/316532

Change 316532 merged by Jcrespo:
mariadb: Fix bashism on eventlogging.init

https://gerrit.wikimedia.org/r/316532

For 3rd party or we keep them as is because are 3rd party or when modifying them we also add a changelog entry in the header to keep track of the changes.

In all cases we should report it upstream and update our version once fixed upstream.

To prove I am not blocking this for the sake of blocking, I fixed this on:

  • mysql_wmf
  • mysql_multi_instance
  • eventlogging_sync

FYI: I've opened the related task T148494 that was actually on my backlog.

Change 631889 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] cdh/hiveserver2: add shebang, fix bashisms

https://gerrit.wikimedia.org/r/631889

Change 631890 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] turn a couple scripts without bashisms into sh scripts

https://gerrit.wikimedia.org/r/631890

Change 631891 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] opernstack: turn bash scripts without bashisms into sh scripts

https://gerrit.wikimedia.org/r/631891

Change 631892 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] dumps/homer/trafficserver: turn bash scripts into sh scripts

https://gerrit.wikimedia.org/r/631892

Change 631893 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] admins/bd808: add bash shebang to .bash scripts

https://gerrit.wikimedia.org/r/631893

Change 631895 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] admins/ori: add bash shebang to .z.sh

https://gerrit.wikimedia.org/r/631895

Change 631897 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] admins/rush: add shebangs to shell scripts

https://gerrit.wikimedia.org/r/631897

Adding a not here that ci was added to the puppet repo for any files ending in .sh in https://gerrit.wikimedia.org/r/c/operations/puppet/+/602693. however files in modules/admin/files/home are excluded from theses checks

Change 631893 merged by Dzahn:
[operations/puppet@production] admins/bd808: add bash shebang to .bash scripts

https://gerrit.wikimedia.org/r/631893

Change 631897 merged by Dzahn:
[operations/puppet@production] admins/rush: add shebangs to shell scripts

https://gerrit.wikimedia.org/r/631897

Change 631895 merged by Dzahn:
[operations/puppet@production] admins/ori: add bash shebang to .z.sh

https://gerrit.wikimedia.org/r/631895

Change 631890 merged by Dzahn:
[operations/puppet@production] turn a couple scripts without bashisms into sh scripts

https://gerrit.wikimedia.org/r/631890

Change 632353 had a related patch set uploaded (by Dzahn; owner: Dzahn):
[operations/puppet@production] icinga-downtime.sh: drop printf directive for long unsigned int

https://gerrit.wikimedia.org/r/632353

Change 632353 merged by Dzahn:
[operations/puppet@production] icinga-downtime.sh: use %s directive with printf, not %lu

https://gerrit.wikimedia.org/r/632353

Change 631892 merged by Dzahn:
[operations/puppet@production] dumps/homer: turn bash scripts into sh scripts

https://gerrit.wikimedia.org/r/631892

Change 631889 abandoned by Dzahn:
[operations/puppet@production] cdh/hiveserver2: add shebang, fix bashisms

Reason:

https://gerrit.wikimedia.org/r/631889

Change 631891 merged by Dzahn:
[operations/puppet@production] openstack: turn bash scripts without bashisms into sh scripts

https://gerrit.wikimedia.org/r/631891

jbond edited projects, added: Puppet CI; removed: Puppet.

Change 938894 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] vrts: drop bashisms and fix other CI issues

https://gerrit.wikimedia.org/r/938894

Change 938895 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] kerberos: fix bashisms

https://gerrit.wikimedia.org/r/938895

Change 938896 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] kerberos: Fix bashisms

https://gerrit.wikimedia.org/r/938896

Change 938897 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] monitoring: fix bashisms and other minor lint issues

https://gerrit.wikimedia.org/r/938897

Change 938898 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] install_server: updaate to use bash

https://gerrit.wikimedia.org/r/938898

Change 938899 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] kubeadm: the use of read -p suggest this should be using bash

https://gerrit.wikimedia.org/r/938899

Change 938894 merged by Jbond:

[operations/puppet@production] vrts: drop bashisms and fix other CI issues

https://gerrit.wikimedia.org/r/938894

Change 938895 merged by Jbond:

[operations/puppet@production] kerberos: fix bashisms

https://gerrit.wikimedia.org/r/938895

Change 938896 merged by Jbond:

[operations/puppet@production] kerberos: Fix bashisms

https://gerrit.wikimedia.org/r/938896

Change 939250 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] kerberos: fix carriage return

https://gerrit.wikimedia.org/r/939250

Change 939250 merged by Jbond:

[operations/puppet@production] kerberos: fix carriage return

https://gerrit.wikimedia.org/r/939250

Change 938897 merged by Jbond:

[operations/puppet@production] monitoring: fix bashisms and other minor lint issues

https://gerrit.wikimedia.org/r/938897

Change 938898 merged by Jbond:

[operations/puppet@production] install_server: drop Bashisms

https://gerrit.wikimedia.org/r/938898

Change 938899 merged by Jbond:

[operations/puppet@production] kubeadm: the use of read -p suggest this should be using bash

https://gerrit.wikimedia.org/r/938899

Change 940376 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] vrts: drop bashisms and fix other CI issues

https://gerrit.wikimedia.org/r/940376

Change 940376 abandoned by Jbond:

[operations/puppet@production] vrts: drop bashisms and fix other CI issues

Reason:

its not lost i was looking at the wrong file

https://gerrit.wikimedia.org/r/940376

Change 940379 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] vtrs: drop bashisms and fix other CI issues

https://gerrit.wikimedia.org/r/940379

Change 940379 merged by Jbond:

[operations/puppet@production] vtrs: drop bashisms and fix other CI issues

https://gerrit.wikimedia.org/r/940379

jbond claimed this task.

theses have all been cleaned up now and standard CI should catch any new issues

Change 944189 had a related patch set uploaded (by Btullis; author: Btullis):

[operations/puppet@production] Revert "install_server: drop Bashisms"

https://gerrit.wikimedia.org/r/944189

Change 944189 merged by Btullis:

[operations/puppet@production] Revert "install_server: drop Bashisms"

https://gerrit.wikimedia.org/r/944189

just noting that the changes to reuse-[parts.sh was reverted due to issues see T332570#9058404

I reverted the change to modules/install_server/files/autoinstall/scripts/reuse-parts.sh in https://gerrit.wikimedia.org/r/c/operations/puppet/+/944189 because it caused a regression of some kind which is described here: T332570#9058181

I can happily look at trying to fix this forward, but for now all I've managed to do is to validate the hypothesis that this commit was responsible.

Change 972061 had a related patch set uploaded (by JHathaway; author: JHathaway):

[operations/puppet@production] reuse-parts.sh: remove bashisms

https://gerrit.wikimedia.org/r/972061

Change 972061 merged by JHathaway:

[operations/puppet@production] reuse-parts.sh: remove bashisms

https://gerrit.wikimedia.org/r/972061

Change 972426 had a related patch set uploaded (by JHathaway; author: JHathaway):

[operations/puppet@production] icinga-init.sh: add shellcheck directive

https://gerrit.wikimedia.org/r/972426

Change 972426 merged by JHathaway:

[operations/puppet@production] icinga-init.sh: add shellcheck directive

https://gerrit.wikimedia.org/r/972426

Shellcheck no longer reports any POSIX violations in our repo, which is equivalent to, but more robust than the checkbashisms command:

find . \( -path ./vendor -o -path ./vendor_modules -o -path ./modules/admin/files/home \) -prune -o -name '*.sh' -exec shellcheck --include=SC30{01..60} {} \;

Please reopen if this command missed any scripts.