On 04/15/2014 02:44 PM, Clint Byrum wrote:
Excerpts from Ben Nemec's message of 2014-04-14 09:26:17 -0700:
tldr: I propose we use bash explicitly for all diskimage-builder scripts
(at least for the short-term - see details below).
This is something that was raised on my linting changes to enable set -o
pipefail. That is a bash-ism, so it could break in the
diskimage-builder scripts that are run using /bin/sh. Two possible
fixes for that: switch to /bin/bash, or don't use -o pipefail
What about this:
if ! [ "$SHEBANG" = "#!/bin/bash" ] ; then
report_warning Non bash shebang, skipping script lint
fi
I was thinking along the same lines, although at least for the moment I
would like to leave the +x check enabled for all shebangs since it still
doesn't make sense to have a shebang without +x.
But I think this raises a bigger question - does diskimage-builder
require bash? If so, I think we should just add a rule to enforce that
/bin/bash is the shell used for everything. I know we have a bunch of
bash-isms in the code already, so at least in the short-term I think
this is probably the way to go, so we can get the benefits of things
like -o pipefail and lose the ambiguity we have right now. For
reference, a quick grep of the diskimage-builder source shows we have
150 scripts using bash explicitly and only 24 that are plain sh, so
making the code truly shell-agnostic is likely to be a significant
amount of work.
Yes, diskimage-builder is bash, not posix shell. We're not masochists.
;)
In the long run it might be nice to have cross-shell compatibility, but
if we're going to do that I think we need a couple of things: 1) Someone
to do the work (I don't have a particular need to run dib in not-bash,
so I'm not signing up for that :-) 2) Testing in other shells -
obviously just changing /bin/bash to /bin/sh doesn't mean we actually
support anything but bash. We really need to be gating on other shells
if we're going to make a significant effort to support them. It's not
good to ask reviewers to try to catch every bash-ism proposed in a
change. This also relates to some of the unit testing work that is
going on right now too - if we had better unit test coverage of the
scripts we would be able to do this more easily.
I suggest that diskimage-builder's included elements should be /bin/bash
only. When we have an element linting tool, non bash shebangs should be
warnings we should enforce "no warnings". For t-i-e, we can strive for
no warnings, but that would be a stretch goal and may involve refining
the warnings.
This doesn't seem to be a problem in dib - almost everything was
explicitly bash already, and the scripts that weren't are pretty trivial.
The ramdisk init script remains a thorn in my side for this though -
based on our conversation last Friday we don't want that to have the
same set -eu behavior as the other scripts (since init failing causes a
kernel panic), and based on Chris's comment in
http://lists.openstack.org/pipermail/openstack-dev/2014-April/032753.html we
don't even want that to be explicitly a bash script, except that it is
today. So I think we need an exception mechanism like the pep8 #noqa
tag (or something similar) to note that init should basically be ignored
by the lint checks since it needs to violate most of the current ones.
For the moment, I'm thinking I'll silently ignore /bin/sh scripts,
convert init to be one of those, and work on the warning/exception
mechanism in the future.
-Ben
_______________________________________________
OpenStack-dev mailing list
[email protected]
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
