Messages in this thread | | | Date | Thu, 1 Nov 2007 08:24:57 -0700 | From | Randy Dunlap <> | Subject | Re: [PATCH/RFC] eradicate bashisms in scripts/patch-kernel |
| |
On Thu, 1 Nov 2007 13:11:33 +0100 Andreas Mohr wrote:
> On Wed, Oct 31, 2007 at 03:24:22PM -0700, Randy Dunlap wrote: > > Andreas Mohr wrote: > > >- the "source" statement most likely needs the ./ prepended, as can be > > >gathered from e.g. http://osdir.com/ml/colinux.devel/2005-12/msg00036.html > > > > That email isn't very convincing to me. > > Actually that place simply was where I found how the > > .: 188: .tmpver.eC4856.1: not found > > dash error that happened when I replaced the bash-specific "source" > statement with its "." counterpart could be ""fixed"". > > > http://www.opengroup.org/onlinepubs/009695399/utilities/dot.html just says > > that > > if <filename> does not contain a slash, the search PATH shall be used > > (searched). > > If you want to prevent that, it's OK to use ./, but at least say that in the > > patch description, please. > > So this part seems to indicate that merely prepending ./ is problematic, > since ./ implies relative path resolution whereas $TMPFILE.1 could > theoretically be an absolute path already. > It would probably be best to add the ./ to mktemp already, to make sure > we source *exactly* the filename expression we originally mktemp'd. > > OTOH prepending ./ to mktemp by default would deny mktemp any > "search for a temporary-capable directory and create the file there" > capabilities. Hmm. I should investigate POSIX shell sourcing > mechanisms more.
As I indicated, I don't object to this part of the patch or to the s/==/=/ part either. The ./ just needs better patch description.
> > >@@ -202,13 +202,7 @@ > > > EXTRAVER= > > > if [ x$EXTRAVERSION != "x" ] > > > then > > >- if [ ${EXTRAVERSION:0:1} == "." ]; then > > >- EXTRAVER=${EXTRAVERSION:1} > > >- else > > >- EXTRAVER=$EXTRAVERSION > > >- fi > > >- EXTRAVER=${EXTRAVER%%[[:punct:]]*} > > >- #echo "$PNAME: changing EXTRAVERSION from $EXTRAVERSION to $EXTRAVER" > > > > What's the problem above? > > The variable manipulation seems bash-specific, the resulting dash error was: > > linux-2.6.23/scripts/patch-kernel: 205: Syntax error: Bad substitution > > which pointed at the > > if [ ${EXTRAVERSION:0:1} == "." ]; then > > line. > > > > > > >- SUBLEVEL=$((SUBLEVEL + 1)) > > >+ SUBLEVEL=$(( $SUBLEVEL + 1 )) > > > > Why are the added spaces needed? > > They weren't strictly needed, what was needed was to add the missing $ sign > (you could perhaps say that I added the spaces to emphasize the $ sign). > Maybe they're best removed. > > > Did you look at > > http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html ? > > No. I should have started by looking at the opengroup.org site when > looking for POSIX specs... > > > > > > FULLVERSION="$VERSION.$PATCHLEVEL.$SUBLEVEL" > > > #echo "#___ trying $FULLVERSION ___" > > > > > >- if [ $((SUBLEVEL)) -gt $((STOPSUBLEVEL)) ]; then > > >+ if [ $SUBLEVEL -gt $STOPSUBLEVEL ]; then > > > > What's the problem here? > > I got > > linux-2.6.23/scripts/patch-kernel: 270: arith: syntax error: "SUBLEVEL" > > thus I simply removed braces since that "fixed" the issue. > May be incorrect, though... > > > I'll think a bit more about these couple changed places (and whether > this still truly works as intended) and mail a patch then. > > (and a big NOTE: I'm no POSIX vs. non-POSIX shell guru at all, only a > semi-versed shell script writer, thus these changes should be reviewed > quite thoroughly)
Neither am I. I read those web pages quickly yesterday, so after you read them, we can discuss more and/or review more patches.
Thanks.
--- ~Randy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |