| Message ID | 1406826517-6106-3-git-send-email-fabio.porcedda@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Fabio Porcedda <fabio.porcedda@gmail.com> schreef: >The [[...]] construct it's an improved and safer construct than the >[...] construct. It's safer because it doesn't do "word splitting" so >we don't need to enclose variable expansion with double quotes, e.g: > > [ -e "$file" ] -> [[ -e $file ]] >--- I'm not so convinced by this patch. [[]] is a bashism, it is not supported in all shells. Yes, using single brackets means that you need to take care, but anyway shell programming has many pitfalls. In the past people have sent patches to make Buildroot more compatible with non-bash shells, so moving in the opposite direction is not a good idea IMO. Best regards, Thomas
Thomas, All, On 2014-07-31 19:19 +0200, Thomas De Schampheleire spake thusly: > Fabio Porcedda <fabio.porcedda@gmail.com> schreef: > >The [[...]] construct it's an improved and safer construct than the > >[...] construct. It's safer because it doesn't do "word splitting" so > >we don't need to enclose variable expansion with double quotes, e.g: > > > > [ -e "$file" ] -> [[ -e $file ]] > >--- > > > I'm not so convinced by this patch. [[]] is a bashism, it is not > supported in all shells. Well, apply-patches is bash script (it has the correct sha-bang.) But it is almost POSIX. The only bashism currently used is the 'function' keyword to declare functions. REmove this, and we have a POSIX script. > Yes, using single brackets means that you need to take care, > but anyway shell programming has many pitfalls. > > In the past people have sent patches to make Buildroot > more compatible with non-bash shells, so moving in the opposite > direction is not a good idea IMO. Agreed. Besides, the gains from that change are not worth it, IMHO. Regards, Yann E. MORIN.
On Thu, Jul 31, 2014 at 7:28 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Thomas, All, > > On 2014-07-31 19:19 +0200, Thomas De Schampheleire spake thusly: >> Fabio Porcedda <fabio.porcedda@gmail.com> schreef: >> >The [[...]] construct it's an improved and safer construct than the >> >[...] construct. It's safer because it doesn't do "word splitting" so >> >we don't need to enclose variable expansion with double quotes, e.g: >> > >> > [ -e "$file" ] -> [[ -e $file ]] >> >--- >> >> >> I'm not so convinced by this patch. [[]] is a bashism, it is not >> supported in all shells. > > Well, apply-patches is bash script (it has the correct sha-bang.) > But it is almost POSIX. The only bashism currently used is the > 'function' keyword to declare functions. REmove this, and we have a > POSIX script. > >> Yes, using single brackets means that you need to take care, >> but anyway shell programming has many pitfalls. >> >> In the past people have sent patches to make Buildroot >> more compatible with non-bash shells, so moving in the opposite >> direction is not a good idea IMO. On the TODO list i didn't found a line about removing the use of bash: http://elinux.org/Buildroot I've found just 4 commit made 4 years ago made by Yann, there are recent commits about that effort? right now bash is a requirement for buildroot: Makefile: # we want bash as shell SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \ else if [ -x /bin/bash ]; then echo /bin/bash; \ else echo sh; fi; fi) It seems to me that right now there is no effort to remove the use of bash. What are the advantage on removing the use of bash other that no requring bash? There are platforms supported by buildroot where is not available bash? > > Agreed. Besides, the gains from that change are not worth it, IMHO. It's worth or not based also on the effort, because the work is already done, it's just a gain without effort so IMHO it's worth it. IMHO rejecting this patch it is like proposing that removing the use of bash it's worth it, so removing bash is worth it? it is really a problem requiring bash? there is someone that is working on removing the use of bash? I just want to understand properly the situation. Thanks for reviewing and best regards
diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh index 37f2d81..a157398 100755 --- a/support/scripts/apply-patches.sh +++ b/support/scripts/apply-patches.sh @@ -37,11 +37,11 @@ patchpattern=${@-*} # use a well defined sorting order export LC_COLLATE=C -if [ ! -d "${builddir}" ] ; then +if [[ ! -d ${builddir} ]] ; then echo "Aborting. '${builddir}' is not a directory." exit 1 fi -if [ ! -d "${patchdir}" ] ; then +if [[ ! -d ${patchdir} ]] ; then echo "Aborting. '${patchdir}' is not a directory." exit 1 fi @@ -79,13 +79,13 @@ function apply_patch { esac echo "" echo "Applying $patch using ${type}: " - if [ ! -e "${path}/$patch" ] ; then + if [[ ! -e ${path}/$patch ]] ; then echo "Error: missing patch file ${path}/$patch" exit 1 fi echo $patch >> ${builddir}/.applied_patches_list ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N - if [ $? != 0 ] ; then + if [[ $? != 0 ]] ; then echo "Patch failed! Please fix ${patch}!" exit 1 fi @@ -98,13 +98,13 @@ function scan_patchdir { # If there is a series file, use it instead of using ls sort order # to apply patches. Skip line starting with a dash. - if [ -e "${path}/series" ] ; then + if [[ -e ${path}/series ]] ; then for i in `grep -Ev "^#" ${path}/series 2> /dev/null` ; do apply_patch "$path" "$i" done else for i in `cd $path; ls -d $patches 2> /dev/null` ; do - if [ -d "${path}/$i" ] ; then + if [[ -d ${path}/$i ]] ; then scan_patchdir "${path}/$i" elif echo "$i" | grep -q -E "\.tar(\..*)?$|\.tbz2?$|\.tgz$" ; then unpackedarchivedir="$builddir/.patches-$(basename $i)-unpacked" @@ -122,7 +122,7 @@ function scan_patchdir { scan_patchdir "$patchdir" "$patchpattern" # Check for rejects... -if [ "`find $builddir/ '(' -name '*.rej' -o -name '.*.rej' ')' -print`" ] ; then +if [[ `find $builddir/ '(' -name '*.rej' -o -name '.*.rej' ')' -print` ]] ; then echo "Aborting. Reject files found." exit 1 fi