Patchwork [1/6] runqemu: Use OE_TMPDIR

login
register
mail settings
Submitter Bernhard Reutner-Fischer
Date May 3, 2012, 5:12 p.m.
Message ID <1336065154-8513-1-git-send-email-rep.dot.nop@gmail.com>
Download mbox | patch
Permalink /patch/26973/
State Accepted
Commit 7b633d0a4cf9aa05e6243974bab2b780c246f8ba
Headers show

Comments

Bernhard Reutner-Fischer - May 3, 2012, 5:12 p.m.
The error message erroneously talked about TMPDIR.
Just use OE_TMPDIR everywhere to make the name of the variable obvious.

Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
---
 scripts/runqemu |   27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)
Scott Garman - May 4, 2012, 9:18 p.m.
On 05/03/2012 10:12 AM, Bernhard Reutner-Fischer wrote:
> The error message erroneously talked about TMPDIR.
> Just use OE_TMPDIR everywhere to make the name of the variable obvious.
>
> Signed-off-by: Bernhard Reutner-Fischer<rep.dot.nop@gmail.com>

Bernhard, these are great improvements - thanks for refactoring this!

Full series Acked-by: Scott Garman <scott.a.garman@intel.com>

Scott
Scott Garman - May 7, 2012, 11:56 p.m.
On 05/03/2012 10:12 AM, Bernhard Reutner-Fischer wrote:
> The error message erroneously talked about TMPDIR.
> Just use OE_TMPDIR everywhere to make the name of the variable obvious.
>
> Signed-off-by: Bernhard Reutner-Fischer<rep.dot.nop@gmail.com>

Bernhard,

This change wasn't well-tested and has broken the runqemu script for 
both bash and dash. :(

 From what I can tell, the =~ regex operator is a bashism. It's also one 
that helps a lot with the code readability. So now that we're faced with 
re-writing the script to avoid using that operator, I'm having second 
thoughts about whether the runqemu script really needs to be 
shell-agnostic. The alternative of invoking grep or other commands to 
process the name patterns does not appeal to me.

I can understand why we're trying to ensure our build system doesn't 
require /bin/sh to be bash, but I think support scripts like runqemu 
might be a special case.

What do other people in the community think of this? The runqemu script 
isn't trivial, and it has to run in a lot of different contexts. Should 
we put the time in to make it shell-agnostic, or allow it to require bash?

Scott
Peter Seebach - May 8, 2012, 7:07 a.m.
On Mon, 7 May 2012 16:56:11 -0700
Scott Garman <scott.a.garman@intel.com> wrote:

>  From what I can tell, the =~ regex operator is a bashism. It's also
> one that helps a lot with the code readability. So now that we're
> faced with re-writing the script to avoid using that operator, I'm
> having second thoughts about whether the runqemu script really needs
> to be shell-agnostic. The alternative of invoking grep or other
> commands to process the name patterns does not appeal to me.
> 
> I can understand why we're trying to ensure our build system doesn't 
> require /bin/sh to be bash, but I think support scripts like runqemu 
> might be a special case.
> 
> What do other people in the community think of this? The runqemu
> script isn't trivial, and it has to run in a lot of different
> contexts. Should we put the time in to make it shell-agnostic, or
> allow it to require bash?

Hmm.  I am honestly not a big fan of the =~, simply because I almost
never remember it, and I can never think whether it's like perl's ~=
or Lua's ~=.  (One is "matches", the other is "is not".)

I tend to write stuff like this as

case $name in
*pat1* | *pat2* | ... )
  # code goes here
  ;;
esac

because that's the natural shell idiom.  It can't do full regex
processing, but we really don't need that here; we just want an
unanchored pattern match.  (And I'm not even sure we *want* a
fully-unanchored match.)  I think the bash [[ ]] thing is one of the
kshisms, but "bash or ksh" is not much better.  :P

From a maintenance standpoint, I like the case construct better
than [[]].  My interest in reading the bash man page to figure out what
some unfamiliar bit of punctuation means this week has declined over
the years.

-s
Marko Lindqvist - May 14, 2012, 10:51 p.m.
On 8 May 2012 02:56, Scott Garman <scott.a.garman@intel.com> wrote:
>
> I can understand why we're trying to ensure our build system doesn't require
> /bin/sh to be bash, but I think support scripts like runqemu might be a
> special case.
>
> What do other people in the community think of this? The runqemu script
> isn't trivial, and it has to run in a lot of different contexts. Should we
> put the time in to make it shell-agnostic, or allow it to require bash?

 1) Do not require /bin/sh to be bash
 2) It's ok, and for "right tool for the job" -reasons often prefered,
to require that development machine has also bash installed.

 So I'm happy with how runqemu currently has #!/bin/bash shebang. It
requires bash to be present, but not necessarily as /bin/sh


 - ML
Mark Hatle - May 14, 2012, 11:15 p.m.
On 5/14/12 5:51 PM, Marko Lindqvist wrote:
> On 8 May 2012 02:56, Scott Garman<scott.a.garman@intel.com>  wrote:
>>
>> I can understand why we're trying to ensure our build system doesn't require
>> /bin/sh to be bash, but I think support scripts like runqemu might be a
>> special case.
>>
>> What do other people in the community think of this? The runqemu script
>> isn't trivial, and it has to run in a lot of different contexts. Should we
>> put the time in to make it shell-agnostic, or allow it to require bash?
>
>   1) Do not require /bin/sh to be bash
>   2) It's ok, and for "right tool for the job" -reasons often prefered,
> to require that development machine has also bash installed.
>
>   So I'm happy with how runqemu currently has #!/bin/bash shebang. It
> requires bash to be present, but not necessarily as /bin/sh

I don't mind it using bash as long as it has bash-isms.  Note, the recent =~ 
stuff didn't work on my machine, even with bash.

--Mark

>
>   - ML
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core

Patch

diff --git a/scripts/runqemu b/scripts/runqemu
index caabf61..7c2c4b3 100755
--- a/scripts/runqemu
+++ b/scripts/runqemu
@@ -55,11 +55,6 @@  SCRIPT_QEMU_OPT=""
 SCRIPT_QEMU_EXTRA_OPT=""
 SCRIPT_KERNEL_OPT=""
 
-# Don't use TMPDIR from the external environment, it may be a distro
-# variable pointing to /tmp (e.g. within X on OpenSUSE)
-# Instead, use OE_TMPDIR for passing this in externally.
-TMPDIR="$OE_TMPDIR"
-
 # Determine whether the file is a kernel or QEMU image, and set the
 # appropriate variables
 process_filename() {
@@ -273,8 +268,8 @@  SPITZ_DEFAULT_KERNEL=zImage-spitz.bin
 SPITZ_DEFAULT_FSTYPE=ext3
 
 setup_tmpdir() {
-    if [ -z "$TMPDIR" ]; then
-        # Try to get TMPDIR from bitbake
+    if [ -z "$OE_TMPDIR" ]; then
+        # Try to get OE_TMPDIR from bitbake
         type -P bitbake &>/dev/null || {
             echo "In order for this script to dynamically infer paths";
             echo "to kernels or filesystem images, you either need";
@@ -282,11 +277,11 @@  setup_tmpdir() {
             echo "before running this script" >&2;
             exit 1; }
 
-        # We have bitbake in PATH, get TMPDIR from bitbake
-        TMPDIR=`bitbake -e | grep ^TMPDIR=\" | cut -d '=' -f2 | cut -d '"' -f2`
-        if [ -z "$TMPDIR" ]; then
+        # We have bitbake in PATH, get OE_TMPDIR from bitbake
+        OE_TMPDIR=`bitbake -e | grep ^TMPDIR=\" | cut -d '=' -f2 | cut -d '"' -f2`
+        if [ -z "$OE_TMPDIR" ]; then
             echo "Error: this script needs to be run from your build directory,"
-            echo "or you need to explicitly set TMPDIR in your environment"
+            echo "or you need to explicitly set OE_TMPDIR in your environment"
             exit 1
         fi
     fi
@@ -303,7 +298,7 @@  setup_sysroot() {
         BUILD_OS=`uname | tr '[A-Z]' '[a-z]'`
         BUILD_SYS="$BUILD_ARCH-$BUILD_OS"
 
-        OECORE_NATIVE_SYSROOT=$TMPDIR/sysroots/$BUILD_SYS
+        OECORE_NATIVE_SYSROOT=$OE_TMPDIR/sysroots/$BUILD_SYS
     fi 
 }
 
@@ -348,7 +343,7 @@  fi
 if [ -z "$KERNEL" ]; then
     setup_tmpdir
     eval kernel_file=\$${machine2}_DEFAULT_KERNEL
-    KERNEL=$TMPDIR/deploy/images/$kernel_file
+    KERNEL=$OE_TMPDIR/deploy/images/$kernel_file
 
     if [ -z "$KERNEL" ]; then
         echo "Error: Unable to determine default kernel for MACHINE [$MACHINE]"
@@ -372,13 +367,13 @@  fi
 # core-image-sato
 if [ "$LAZY_ROOTFS" = "true" ]; then
     setup_tmpdir
-    echo "Assuming $ROOTFS really means $TMPDIR/deploy/images/$ROOTFS-$MACHINE.$FSTYPE"
-    ROOTFS=$TMPDIR/deploy/images/$ROOTFS-$MACHINE.$FSTYPE
+    echo "Assuming $ROOTFS really means $OE_TMPDIR/deploy/images/$ROOTFS-$MACHINE.$FSTYPE"
+    ROOTFS=$OE_TMPDIR/deploy/images/$ROOTFS-$MACHINE.$FSTYPE
 fi
 
 if [ -z "$ROOTFS" ]; then
     setup_tmpdir
-    T=$TMPDIR/deploy/images
+    T=$OE_TMPDIR/deploy/images
     eval rootfs_list=\$${machine2}_DEFAULT_ROOTFS
     findimage $T $MACHINE $FSTYPE