raisin: Refactor to be more like a library

raisin: Refactor to be more like a library

[George Dunlap]

Update to the core.sh calling convention, which will parse var=value
arguments.

Also tweak some Fedora dependencies and refactor the build dependency
checking / installation code.

CC: Stefano Stabellini <stefano.stabellini@citrix.com>

[PATCH 1/8] Add core.sh and wrapper function

[George Dunlap]

Add core functionality and an executable to run it

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 lib/core.sh | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 raise       |  16 +++++++
 2 files changed, 163 insertions(+)
 create mode 100644 lib/core.sh
 create mode 100755 raise

diff --git a/lib/core.sh b/lib/core.sh
new file mode 100644
index 0000000..46b02e0
--- /dev/null
+++ b/lib/core.sh
@@ -0,0 +1,147 @@
+#!/usr/bin/env bash
+
+RAISIN_HELP=()
+
+# Core calling convention functionality
+# 
+# $arg_parse
+#
+#   Parse $@.  Begin with every element that a '=' in it being treated
+#   as a variable assignment: declare the LHS a local variable and set
+#   it to the RHS.  Once you reach an element without a '=' in it, or
+#   a '--', put all further elements into the array args.
+# 
+# $requireargs VARNAME1 [VARNAME2, ...]
+#
+#   Check to see that VARNAME1 is defined, and throw an error if not.
+#
+# default VARNAME "VALUE" ; $default_post
+#
+#   Check to see is VARNAME is defined; if not, declare it a local
+#   variable and set it to VALUE.
+#
+# Example usage:
+#
+#   function log()
+#   {
+#       local i;
+#
+#       $arg_parse
+#
+#       default tmpdir "/tmp"; $default_post
+#
+#       $requireargs filename
+#
+#       for $i in ${args[@]} ; do
+#           echo "$i" > $tmpdir/$filename
+#       done
+#
+#       [[ -n "$flower" ] && echo $flower
+#   }
+
+arg_parse_cmd=\
+"local -a args;
+local _a;
+local _vn;
+local _m;
+
+_m=true;
+
+for _a in \"\$@\" ; do
+    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
+    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
+        false && echo Parameter;
+        _vn=\${_a%%=*};
+        eval \"local \$_vn\";
+        eval \"\$_a\";
+    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
+        false && echo Separator;
+        _m=false;
+    else
+        false && echo Argument;
+        _m=false;
+        args+=(\"\$_a\");
+    fi;
+done"
+
+arg_parse="eval $arg_parse_cmd"
+
+# Pass in either the current function name, or the name of the script
+requireargs="eval _func=\"\$FUNCNAME\" ; eval [[ -n \\\"\$_func\\\" ]] || _func=\$0 ; eval _require-args \$_func"
+
+function _require-args()
+{
+    local _arg
+    local _args
+
+    _args=($@)
+
+    for _arg in ${_args[@]:1} ; do
+	eval "[[ -n \"\${$_arg}\" ]] || fail \"${_args[0]}: Missing $_arg\""
+    done
+}
+
+function default()
+{
+    # l0: eval i="5"
+    # l1: default_post="eval $1=\"$2\""
+    # l3: eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval \$1=\\\"$2\\\"\" ; fi"
+    eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval local \$1=\\\"$2\\\"\" ; else unset default_post ; fi"
+}
+
+function fail()
+{
+   echo FATAL "$@" 1>&2
+   [[ -n "$fail_cleanup" ]] && $fail_cleanup
+   exit 1
+}
+
+function info()
+{
+   echo INFO "$@" 1>&2
+}
+
+function error()
+{
+   echo ERROR "$@" 1>&2
+}
+
+RAISIN_HELP+=("help            List available commands")
+function help()
+{
+    echo "Usage: $0 command [arguments...]"
+    echo "  Run '$0 help' for a list of commands, or '$0 command-help' for"
+    echo "  help on a particular command."
+    echo
+
+    for i in "${RAISIN_HELP[@]}" ; do
+	echo "$i"
+    done
+
+    echo
+
+    exit 0
+}
+
+function cmdline()
+{
+    local cmd;
+
+    [[ "$#" -eq "0" ]] && help
+
+    $arg_parse
+
+    # If the command is not defined, then print help and exit
+    if ! type "${args[0]}" 2>/dev/null | grep -q 'function' ; then
+	echo "Unknown function: ${args[0]}"
+	echo
+	help
+    fi
+
+    info Running "${args[0]}"
+    "${args[0]}" "${args[@]:1}" || exit 1
+
+    if ! [[ -z "$RET" ]] ; then
+	echo $RET
+    fi
+}
diff --git a/raise b/raise
new file mode 100755
index 0000000..7f3faae
--- /dev/null
+++ b/raise
@@ -0,0 +1,16 @@
+#!/usr/bin/env bash
+
+# Include your defaults
+if [[ -e "./config" ]] ; then
+    . ./config
+fi
+
+# To use this as a library, set RAISIN_PATH appropriately
+[[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
+
+# Then as many as the sub-libraries as you need
+. ${RAISIN_PATH}/core.sh
+
+# And do your own thing rather than running commands
+# I suggest defining a "main" function of your own and running it like this.
+cmdline "$@"
-- 
1.9.1

[PATCH 2/8] Remove redundant "source" from component definitions

[George Dunlap]

These should be called from other places that have already done the
appropriate "source"

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/grub    | 4 ----
 components/libvirt | 4 ----
 components/xen     | 4 ----
 3 files changed, 12 deletions(-)

diff --git a/components/grub b/components/grub
index ab4f583..5a42000 100644
--- a/components/grub
+++ b/components/grub
@@ -1,9 +1,5 @@
 #!/usr/bin/env bash
 
-source "$BASEDIR"/config
-source "$BASEDIR"/scripts/common-functions.sh
-
-
 function _grub_install_dependencies() {
     local DEP_Debian_common="build-essential tar autoconf bison flex"
     local DEP_Debian_x86_32="$DEP_Debian_common"
diff --git a/components/libvirt b/components/libvirt
index e35de8c..e22996e 100644
--- a/components/libvirt
+++ b/components/libvirt
@@ -1,9 +1,5 @@
 #!/usr/bin/env bash
 
-source "$BASEDIR"/config
-source "$BASEDIR"/scripts/common-functions.sh
-
-
 function _libvirt_install_dependencies() {
     local DEP_Debian_common="build-essential libtool autoconf autopoint \
                              xsltproc libxml2-utils pkg-config python-dev   \
diff --git a/components/xen b/components/xen
index d6286b2..a49a1d1 100644
--- a/components/xen
+++ b/components/xen
@@ -1,9 +1,5 @@
 #!/usr/bin/env bash
 
-source "$BASEDIR"/config
-source "$BASEDIR"/scripts/common-functions.sh
-
-
 function _xen_install_dependencies() {
     local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
              libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
-- 
1.9.1

[PATCH 3/8] Move common-functions.sh and git-checkout.sh into lib

[George Dunlap]

"script" implies something which is designed to be run standalone.

"lib" implies that this is going to be sourced from another bash script.

Also change "git-checkout" to be a function rather than a script

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/grub                      |  2 +-
 components/libvirt                   |  2 +-
 components/xen                       |  2 +-
 {scripts => lib}/common-functions.sh |  0
 lib/git-checkout.sh                  | 32 ++++++++++++++++++++++++++++++++
 raise.sh                             |  3 ++-
 scripts/git-checkout.sh              | 30 ------------------------------
 unraise.sh                           |  2 +-
 8 files changed, 38 insertions(+), 35 deletions(-)
 rename {scripts => lib}/common-functions.sh (100%)
 create mode 100755 lib/git-checkout.sh
 delete mode 100755 scripts/git-checkout.sh

diff --git a/components/grub b/components/grub
index 5a42000..a5aa27d 100644
--- a/components/grub
+++ b/components/grub
@@ -29,7 +29,7 @@ function grub_build() {
     cd "$BASEDIR"
     rm -f memdisk.tar
     tar cf memdisk.tar -C data grub.cfg
-    ./scripts/git-checkout.sh $GRUB_UPSTREAM_URL $GRUB_UPSTREAM_REVISION grub-dir
+    git-checkout $GRUB_UPSTREAM_URL $GRUB_UPSTREAM_REVISION grub-dir
     cd grub-dir
     ./autogen.sh
     ## GRUB32
diff --git a/components/libvirt b/components/libvirt
index e22996e..6602dcf 100644
--- a/components/libvirt
+++ b/components/libvirt
@@ -26,7 +26,7 @@ function libvirt_build() {
     _libvirt_install_dependencies
 
     cd "$BASEDIR"
-    ./scripts/git-checkout.sh $LIBVIRT_UPSTREAM_URL $LIBVIRT_UPSTREAM_REVISION libvirt-dir
+    git-checkout $LIBVIRT_UPSTREAM_URL $LIBVIRT_UPSTREAM_REVISION libvirt-dir
     cd libvirt-dir
     CFLAGS="-I$INST_DIR/$PREFIX/include" \
     LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
diff --git a/components/xen b/components/xen
index a49a1d1..70b72b0 100644
--- a/components/xen
+++ b/components/xen
@@ -23,7 +23,7 @@ function xen_build() {
     _xen_install_dependencies
 
     cd "$BASEDIR"
-    ./scripts/git-checkout.sh $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
+    git-checkout $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
     cd xen-dir
     ./configure --prefix=$PREFIX
     $MAKE
diff --git a/scripts/common-functions.sh b/lib/common-functions.sh
similarity index 100%
rename from scripts/common-functions.sh
rename to lib/common-functions.sh
diff --git a/lib/git-checkout.sh b/lib/git-checkout.sh
new file mode 100755
index 0000000..2ca8f25
--- /dev/null
+++ b/lib/git-checkout.sh
@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+
+function git-checkout() {
+    if [[ $# -lt 3 ]]
+    then
+	echo "Usage: $0 <tree> <tag> <dir>"
+	exit 1
+    fi
+
+    TREE=$1
+    TAG=$2
+    DIR=$3
+
+    set -e
+
+    if [[ ! -d $DIR-remote ]]
+    then
+	rm -rf $DIR-remote $DIR-remote.tmp
+	mkdir -p $DIR-remote.tmp; rmdir $DIR-remote.tmp
+	$GIT clone $TREE $DIR-remote.tmp
+	if [[ "$TAG" ]]
+	then
+	    cd $DIR-remote.tmp
+	    $GIT branch -D dummy >/dev/null 2>&1 ||:
+	    $GIT checkout -b dummy $TAG
+	    cd ..
+	fi
+	mv $DIR-remote.tmp $DIR-remote
+    fi
+    rm -f $DIR
+    ln -sf $DIR-remote $DIR
+}
diff --git a/raise.sh b/raise.sh
index 3c8281e..422fbe4 100755
--- a/raise.sh
+++ b/raise.sh
@@ -3,7 +3,8 @@
 set -e
 
 source config
-source scripts/common-functions.sh
+source lib/common-functions.sh
+source lib/git-checkout.sh
 
 _help() {
     echo "Usage: ./build.sh <options> <command>"
diff --git a/scripts/git-checkout.sh b/scripts/git-checkout.sh
deleted file mode 100755
index 912bfae..0000000
--- a/scripts/git-checkout.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/usr/bin/env bash
-
-if [[ $# -lt 3 ]]
-then
-	echo "Usage: $0 <tree> <tag> <dir>"
-	exit 1
-fi
-
-TREE=$1
-TAG=$2
-DIR=$3
-
-set -e
-
-if [[ ! -d $DIR-remote ]]
-then
-	rm -rf $DIR-remote $DIR-remote.tmp
-	mkdir -p $DIR-remote.tmp; rmdir $DIR-remote.tmp
-	$GIT clone $TREE $DIR-remote.tmp
-	if [[ "$TAG" ]]
-	then
-		cd $DIR-remote.tmp
-		$GIT branch -D dummy >/dev/null 2>&1 ||:
-		$GIT checkout -b dummy $TAG
-		cd ..
-	fi
-	mv $DIR-remote.tmp $DIR-remote
-fi
-rm -f $DIR
-ln -sf $DIR-remote $DIR
diff --git a/unraise.sh b/unraise.sh
index 2f08901..50ce310 100755
--- a/unraise.sh
+++ b/unraise.sh
@@ -3,7 +3,7 @@
 set -e
 
 source config
-source scripts/common-functions.sh
+source lib/common-functions.sh
 
 
 # start execution
-- 
1.9.1

[PATCH 4/8] Import raise.sh and unraise.sh into library

[George Dunlap]

Make as few changes as possible to begin with, just to separate code
motion from changes.

For now, remove raise.sh and unraise.sh from package creation, until
we can figure out what to do instead.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 raise.sh => lib/build.sh | 62 ++++++++++++++++--------------------------------
 raise                    |  6 +++++
 scripts/mkdeb            |  3 ++-
 scripts/mkrpm            |  5 ++--
 unraise.sh               | 17 -------------
 5 files changed, 31 insertions(+), 62 deletions(-)
 rename raise.sh => lib/build.sh (77%)
 delete mode 100755 unraise.sh

diff --git a/raise.sh b/lib/build.sh
similarity index 77%
rename from raise.sh
rename to lib/build.sh
index 422fbe4..ab1e087 100755
--- a/raise.sh
+++ b/lib/build.sh
@@ -2,10 +2,6 @@
 
 set -e
 
-source config
-source lib/common-functions.sh
-source lib/git-checkout.sh
-
 _help() {
     echo "Usage: ./build.sh <options> <command>"
     echo "where options are:"
@@ -18,7 +14,9 @@ _help() {
     echo "    configure            Configure the system  (requires sudo)"
 }
 
-_build() {
+build() {
+    $arg_parse
+
     if [[ $YES != "y" ]]
     then
         echo "Do you want Raisin to automatically install build time dependencies for you? (y/n)"
@@ -50,7 +48,20 @@ _build() {
     build_package xen-system
 }
 
-_install() {
+unraise() {
+    $arg_parse
+
+    for_each_component clean
+
+    uninstall_package xen-system
+    for_each_component unconfigure
+
+    rm -rf "$INST_DIR"
+}
+
+install() {
+    $arg_parse
+
     # need single braces for filename matching expansion
     if [ ! -f xen-sytem*rpm ] && [ ! -f xen-system*deb ]
     then
@@ -60,7 +71,9 @@ _install() {
     install_package xen-system
 }
 
-_configure() {
+configure() {
+    $arg_parse
+
     if [[ $YES != "y" ]]
     then
         echo "Proceeding we'll make changes to the running system,"
@@ -82,38 +95,3 @@ _configure() {
     for_each_component configure
 }
 
-# start execution
-common_init
-
-# parameters check
-export VERBOSE=0
-export YES="n"
-export NO_DEPS=0
-while [[ $# -gt 1 ]]
-do
-  if [[ "$1" = "-v" || "$1" = "--verbose" ]]
-  then
-    VERBOSE=1
-    shift 1
-  elif [[ "$1" = "-y" || "$1" = "--yes" ]]
-  then
-    YES="y"
-    shift 1
-  else
-    _help
-    exit 1
-  fi
-done
-
-case "$1" in
-    "build" | "install" | "configure" )
-        COMMAND=$1
-        ;;
-    *)
-        _help
-        exit 1
-        ;;
-esac
-
-_$COMMAND
-
diff --git a/raise b/raise
index 7f3faae..142956d 100755
--- a/raise
+++ b/raise
@@ -10,6 +10,12 @@ fi
 
 # Then as many as the sub-libraries as you need
 . ${RAISIN_PATH}/core.sh
+. ${RAISIN_PATH}/common-functions.sh
+. ${RAISIN_PATH}/git-checkout.sh
+. ${RAISIN_PATH}/build.sh
+
+# Set up basic functionality
+common_init
 
 # And do your own thing rather than running commands
 # I suggest defining a "main" function of your own and running it like this.
diff --git a/scripts/mkdeb b/scripts/mkdeb
index 46ade07..cb2a1b6 100755
--- a/scripts/mkdeb
+++ b/scripts/mkdeb
@@ -35,7 +35,8 @@ mkdir -p deb/opt/raisin
 cp -r data deb/opt/raisin
 cp -r components deb/opt/raisin
 cp -r scripts deb/opt/raisin
-cp config raise.sh unraise.sh deb/opt/raisin
+# FIXME
+#cp config raise.sh unraise.sh deb/opt/raisin
 
 
 # Debian doesn't use /usr/lib64 for 64-bit libraries
diff --git a/scripts/mkrpm b/scripts/mkrpm
index c530466..90d9bdc 100755
--- a/scripts/mkrpm
+++ b/scripts/mkrpm
@@ -48,8 +48,9 @@ cp -r "$BASEDIR"/data \$RPM_BUILD_ROOT/opt/raisin
 cp -r "$BASEDIR"/components \$RPM_BUILD_ROOT/opt/raisin
 cp -r "$BASEDIR"/scripts \$RPM_BUILD_ROOT/opt/raisin
 cp "$BASEDIR"/config \$RPM_BUILD_ROOT/opt/raisin
-cp "$BASEDIR"/raise.sh \$RPM_BUILD_ROOT/opt/raisin
-cp "$BASEDIR"/unraise.sh \$RPM_BUILD_ROOT/opt/raisin
+# FIXME
+# cp "$BASEDIR"/raise.sh \$RPM_BUILD_ROOT/opt/raisin
+# cp "$BASEDIR"/unraise.sh \$RPM_BUILD_ROOT/opt/raisin
 
 %clean
 
diff --git a/unraise.sh b/unraise.sh
deleted file mode 100755
index 50ce310..0000000
--- a/unraise.sh
+++ /dev/null
@@ -1,17 +0,0 @@
-#!/usr/bin/env bash
-
-set -e
-
-source config
-source lib/common-functions.sh
-
-
-# start execution
-common_init
-
-for_each_component clean
-
-uninstall_package xen-system
-for_each_component unconfigure
-
-rm -rf "$INST_DIR"
-- 
1.9.1

[PATCH 5/8] Allow the user's config to live outside of git

[George Dunlap]

Move the default config to "defconfig" and add config to .gitignore.
If config doesn't exist, then copy defconfig to it in common_init.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 .gitignore          |  1 +
 config => defconfig |  0
 raise               | 13 ++++++++-----
 3 files changed, 9 insertions(+), 5 deletions(-)
 rename config => defconfig (100%)

diff --git a/.gitignore b/.gitignore
index 47e9874..35a46c6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -28,6 +28,7 @@ cscope.in.out
 cscope.out
 cscope.po.out
 .config
+config
 
 xen-dir
 xen-dir-remote
diff --git a/config b/defconfig
similarity index 100%
rename from config
rename to defconfig
diff --git a/raise b/raise
index 142956d..ae435d9 100755
--- a/raise
+++ b/raise
@@ -1,10 +1,5 @@
 #!/usr/bin/env bash
 
-# Include your defaults
-if [[ -e "./config" ]] ; then
-    . ./config
-fi
-
 # To use this as a library, set RAISIN_PATH appropriately
 [[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
 
@@ -14,6 +9,14 @@ fi
 . ${RAISIN_PATH}/git-checkout.sh
 . ${RAISIN_PATH}/build.sh
 
+# Include your defaults
+if [[ ! -e "./config" ]] ; then
+   echo "No config file found, copying default config"
+   cp defconfig config
+fi
+
+. ./config
+
 # Set up basic functionality
 common_init
 
-- 
1.9.1

[PATCH 6/8] xen: Replace iasl with acpica-tools

[George Dunlap]

acpica-tools is the new package that replaces iasl.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/xen | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/components/xen b/components/xen
index 70b72b0..e8cbb2b 100644
--- a/components/xen
+++ b/components/xen
@@ -12,7 +12,7 @@ function _xen_install_dependencies() {
     local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
              ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
              patch pixman-devel glibc-devel bridge-utils grub2"
-    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 iasl texinfo"
+    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
     local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
 
     echo installing Xen dependencies
-- 
1.9.1

[PATCH 7/8] xen: Require wget

[George Dunlap]

The xen build requires eiter wget or ftp.  Use wget.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/xen | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/components/xen b/components/xen
index e8cbb2b..7a9f22d 100644
--- a/components/xen
+++ b/components/xen
@@ -3,7 +3,7 @@
 function _xen_install_dependencies() {
     local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
              libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
-             libssl-dev libpixman-1-dev bridge-utils"
+             libssl-dev libpixman-1-dev bridge-utils wget"
     local DEP_Debian_x86_32="$DEP_Debian_common bcc iasl bin86 texinfo"
     local DEP_Debian_x86_64="$DEP_Debian_x86_32 libc6-dev-i386"
     local DEP_Debian_arm32="$DEP_Debian_common libfdt-dev"
@@ -11,7 +11,7 @@ function _xen_install_dependencies() {
 
     local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
              ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
-             patch pixman-devel glibc-devel bridge-utils grub2"
+             patch pixman-devel glibc-devel bridge-utils grub2 wget"
     local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
     local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
 
-- 
1.9.1

[PATCH 8/8] Refactor package dependency checking and installation

[George Dunlap]

First, create a new global variable, PKGTYPE.  At the moment support "deb" and "rpm".

Define _check-package-$PKGTYPE which returns true if the package is
installed, false otherwise, and _install-package-$PKGTYPE which will
install a list of packages.

Define check-package(), which will take a list of packages, and check
to see if they're installed.  Any missing packages will be added to an
array called "missing".

Change _${COMPONENT}_install_dependencies to
${COMPONENT}_check_package.  Have these call check-package.

Don't call _${COMPONENT}_install_dependencies from ${COMPONENT}_build.

Define check-builddeps().  Define an empty "missing" array.  Call
check-package for "raisin" dependincies (like git and rpmbuild).  Then
call for_each_component check_package.

At this point we have an array with all missing packages.  If it's
empty, be happy.  If it's non-empty, and deps=true, try to install the
packages; otherwise print the missing packages and exit.

Add install-builddeps(), which is basically check-builddeps() with
deps=true by default.

Call check-builddeps from build() to close the loop.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
---
 components/grub         |  6 ++--
 components/libvirt      |  6 ++--
 components/xen          | 10 +++---
 lib/build.sh            | 87 +++++++++++++++++++++++++++++++++++------------
 lib/common-functions.sh | 89 +++++++++++++++++++++++++++++++++++++++----------
 5 files changed, 148 insertions(+), 50 deletions(-)

diff --git a/components/grub b/components/grub
index a5aa27d..839e001 100644
--- a/components/grub
+++ b/components/grub
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-function _grub_install_dependencies() {
+function grub_check_package() {
     local DEP_Debian_common="build-essential tar autoconf bison flex"
     local DEP_Debian_x86_32="$DEP_Debian_common"
     local DEP_Debian_x86_64="$DEP_Debian_common libc6-dev-i386"
@@ -18,8 +18,8 @@ function _grub_install_dependencies() {
         echo grub is only supported on x86_32 and x86_64
         return
     fi
-    echo installing Grub dependencies
-    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
+    echo checking Grub dependencies
+    eval check-package \$DEP_"$DISTRO"_"$ARCH"
 }
 
 
diff --git a/components/libvirt b/components/libvirt
index 6602dcf..b106970 100644
--- a/components/libvirt
+++ b/components/libvirt
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-function _libvirt_install_dependencies() {
+function libvirt_check_package() {
     local DEP_Debian_common="build-essential libtool autoconf autopoint \
                              xsltproc libxml2-utils pkg-config python-dev   \
                              libxml-xpath-perl libyajl-dev libxml2-dev      \
@@ -18,8 +18,8 @@ function _libvirt_install_dependencies() {
     local DEP_Fedora_x86_32="$DEP_Fedora_common"
     local DEP_Fedora_x86_64="$DEP_Fedora_common"
 
-    echo installing Libvirt dependencies
-    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
+    echo checking Libvirt dependencies
+    eval check-package \$DEP_"$DISTRO"_"$ARCH"
 }
 
 function libvirt_build() {
diff --git a/components/xen b/components/xen
index 7a9f22d..ce46e3d 100644
--- a/components/xen
+++ b/components/xen
@@ -1,6 +1,8 @@
 #!/usr/bin/env bash
 
-function _xen_install_dependencies() {
+function xen_check_package() {
+    $requireargs DISTRO ARCH
+
     local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
              libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
              libssl-dev libpixman-1-dev bridge-utils wget"
@@ -15,13 +17,11 @@ function _xen_install_dependencies() {
     local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
     local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
 
-    echo installing Xen dependencies
-    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
+    echo Checking Xen dependencies
+    eval check-package \$DEP_"$DISTRO"_"$ARCH"
 }
 
 function xen_build() {
-    _xen_install_dependencies
-
     cd "$BASEDIR"
     git-checkout $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
     cd xen-dir
diff --git a/lib/build.sh b/lib/build.sh
index ab1e087..a453874 100755
--- a/lib/build.sh
+++ b/lib/build.sh
@@ -2,32 +2,72 @@
 
 set -e
 
-_help() {
-    echo "Usage: ./build.sh <options> <command>"
+RAISIN_HELP+=("check-builddep   Check to make sure we have all dependencies installed")
+function check-builddep() {
+    local -a missing
+
+    $arg_parse
+
+    default deps "false" ; $default_post
+    
+    $requireargs PKGTYPE DISTRO
+
+    check-package git
+
+    if [[ $DISTRO = "Fedora" ]] ; then
+        check-package rpm-build
+    fi
+
+    for_each_component check_package
+
+    if [[ -n "${missing[@]}" ]] ; then
+	echo "Missing packages: ${missing[@]}"
+	if $deps ; then
+	    echo "Installing..."
+	    install-package "${missing[@]}"
+	else
+	    echo "Please install, or run ./raise install-builddep"
+	    exit 1
+	fi
+    fi
+}
+
+RAISIN_HELP+=("install-builddep Install all packages required for build.  Uses sudo.")
+function install-builddep() {
+    local pkgtype
+
+    $arg_parse
+
+    check-builddep deps=true
+}
+
+build-help() {
+    echo "Usage: $0 build opt=val"
     echo "where options are:"
-    echo "    -n | --no-deps       Do no install build-time dependencies"
-    echo "    -v | --verbose       Verbose"
-    echo "    -y | --yes           Do not ask questions and continue"
-    echo "where commands are:"
-    echo "    build                Build the components enabled in config"
-    echo "    install              Install binaries under /  (requires sudo)"
-    echo "    configure            Configure the system  (requires sudo)"
+    echo "    yes=(true|false)     Don't ask before doing anything dangerous.  Defaults to false."
+    echo "    deps=(true|false)    Attempt to install dependencies.  Defaults to false."
+    echo "    verbose=(true|false) Increased verbosity.  Defaults to false."
 }
 
+RAISIN_HELP+=("build           Clone and build components enabled in config")
 build() {
     $arg_parse
 
-    if [[ $YES != "y" ]]
-    then
-        echo "Do you want Raisin to automatically install build time dependencies for you? (y/n)"
+    default deps "false" ; $default_post
+    default verbose "false" ; $default_post
+    default yes "false" ; $default_post
+
+    if $deps && ! $yes ; then
+        echo "WARNING: You have chosen to automatically install software as root."
+        echo -n "Are you sure that you want to continue? (y/n)"
         while read answer
         do
             if [[ "$answer" = "n" ]]
             then
-                NO_DEPS=1
-                break
+                exit 0
             elif [[ "$answer" = "y" ]]
             then
+		yes="true"
                 break
             else
                 echo "Reply y or n"
@@ -35,12 +75,10 @@ build() {
         done
     fi
 
+    check-builddep
+    
     mkdir -p "$INST_DIR" &>/dev/null
-    install_dependencies git
-    if [[ $DISTRO = "Fedora" ]]
-    then
-        install_dependencies rpm-build
-    fi
+
     # build and install under $DESTDIR
     for_each_component clean
     for_each_component build
@@ -59,6 +97,7 @@ unraise() {
     rm -rf "$INST_DIR"
 }
 
+RAISIN_HELP+=("install         Install binaries under /  (requires sudo)")
 install() {
     $arg_parse
 
@@ -71,12 +110,15 @@ install() {
     install_package xen-system
 }
 
+RAISIN_HELP+=("configure       Configure the system  (requires sudo)")
 configure() {
     $arg_parse
 
-    if [[ $YES != "y" ]]
-    then
-        echo "Proceeding we'll make changes to the running system,"
+    default verbose "false" ; $default_post
+    default yes "false" ; $default_post
+
+    if $deps && ! $yes ; then
+        echo "WARNING: Proceeding we'll make changes to the running system,"
         echo "are you sure that you want to continue? (y/n)"
         while read answer
         do
@@ -85,6 +127,7 @@ configure() {
                 exit 0
             elif [[ "$answer" = "y" ]]
             then
+		yes="true"
                 break
             else
                 echo "Reply y or n"
diff --git a/lib/common-functions.sh b/lib/common-functions.sh
index cfa0c7f..fd73f08 100644
--- a/lib/common-functions.sh
+++ b/lib/common-functions.sh
@@ -97,15 +97,23 @@ function get_distro() {
     case "$os_VENDOR" in
         "Debian"* | "Ubuntu"* | "LinuxMint"* )
             DISTRO="Debian"
+	    PKGTYPE="deb"
+            ;;
+        "Fedora" )
+            DISTRO="Fedora"
+	    PKGTYPE="rpm"
             ;;
         "SUSE"* )
             DISTRO="SUSE"
+	    PKGTYPE="rpm"
             ;;
         "OpenSUSE"* | "openSUSE"* )
             DISTRO="openSUSE"
+	    PKGTYPE="rpm"
             ;;
         "Red"* | "CentOS"* )
             DISTRO="CentOS"
+	    PKGTYPE="rpm"
             ;;
         *)
             DISTRO=$os_VENDOR
@@ -114,6 +122,7 @@ function get_distro() {
 
     export os_VENDOR os_RELEASE os_UPDATE os_CODENAME
     export DISTRO
+    export PKGTYPE
 }
 
 function get_arch() {
@@ -122,24 +131,70 @@ function get_arch() {
                 -e s/aarch64/arm64/`
 }
 
-function install_dependencies() {
-    if [[ "$NO_DEPS" && "$NO_DEPS" -eq 1 ]]
-    then
-        echo "Not installing any dependencies, as requested."
-        echo "Depency list: $*"
-        return 0
+function _check-package-deb() {
+    $arg_parse
+
+    $verbose && echo "Checking for package ${args[0]}"
+
+    if dpkg -s "${args[0]}" 2>/dev/null | grep -q "Status:.*installed" ; then
+	return 0
+    else
+	return 1
     fi
-    case $DISTRO in
-        "Debian" )
-        $SUDO apt-get install -y $*
-        ;;
-        "Fedora" )
-        $SUDO yum install -y $*
-        ;;
-        * )
-        echo "I don't know how to install dependencies on $DISTRO"
-        ;;
-    esac
+}
+
+function _install-package-deb() {
+    $arg_parse
+
+    $requireargs SUDO
+
+    $SUDO apt-get install -y "${args[@]}"
+}
+
+function _check-package-rpm() {
+    local pstatus
+
+    $arg_parse
+
+    $verbose && echo "Checking for package ${args[0]}"
+
+    if rpm -q "${args[0]}" 2>&1 >/dev/null ; then
+	return 0
+    else
+	return 1
+    fi
+}
+
+function _install-package-rpm() {
+    $arg_parse
+
+    $requireargs SUDO
+
+    $SUDO yum install -y "${args[@]}"
+}
+
+# Modifies inherited variable "missing"
+function check-package() {
+    local p
+
+    $arg_parse
+
+    $requireargs PKGTYPE
+
+    for p in "${args[@]}" ; do
+	if ! _check-package-${PKGTYPE} $p ; then
+	    missing+=("$p")
+	fi
+    done
+
+}
+
+function install-package() {
+    $arg_parse
+
+    $requireargs PKGTYPE
+
+    _install-package-${PKGTYPE} "${args[@]}"
 }
 
 function start_initscripts() {
-- 
1.9.1

Re: [PATCH 1/8] Add core.sh and wrapper function

[George Dunlap]

On Thu, Apr 9, 2015 at 8:29 PM, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> Add core functionality and an executable to run it
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>

Hmm, not sure where the 0/8 for this patch went... anyway, you can
also find this series here:

git://xenbits.xenproject.org/people/gdunlap/raisin.git out/librarify/v1

-George


> ---
>  lib/core.sh | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  raise       |  16 +++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 lib/core.sh
>  create mode 100755 raise
>
> diff --git a/lib/core.sh b/lib/core.sh
> new file mode 100644
> index 0000000..46b02e0
> --- /dev/null
> +++ b/lib/core.sh
> @@ -0,0 +1,147 @@
> +#!/usr/bin/env bash
> +
> +RAISIN_HELP=()
> +
> +# Core calling convention functionality
> +#
> +# $arg_parse
> +#
> +#   Parse $@.  Begin with every element that a '=' in it being treated
> +#   as a variable assignment: declare the LHS a local variable and set
> +#   it to the RHS.  Once you reach an element without a '=' in it, or
> +#   a '--', put all further elements into the array args.
> +#
> +# $requireargs VARNAME1 [VARNAME2, ...]
> +#
> +#   Check to see that VARNAME1 is defined, and throw an error if not.
> +#
> +# default VARNAME "VALUE" ; $default_post
> +#
> +#   Check to see is VARNAME is defined; if not, declare it a local
> +#   variable and set it to VALUE.
> +#
> +# Example usage:
> +#
> +#   function log()
> +#   {
> +#       local i;
> +#
> +#       $arg_parse
> +#
> +#       default tmpdir "/tmp"; $default_post
> +#
> +#       $requireargs filename
> +#
> +#       for $i in ${args[@]} ; do
> +#           echo "$i" > $tmpdir/$filename
> +#       done
> +#
> +#       [[ -n "$flower" ] && echo $flower
> +#   }
> +
> +arg_parse_cmd=\
> +"local -a args;
> +local _a;
> +local _vn;
> +local _m;
> +
> +_m=true;
> +
> +for _a in \"\$@\" ; do
> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
> +        false && echo Parameter;
> +        _vn=\${_a%%=*};
> +        eval \"local \$_vn\";
> +        eval \"\$_a\";
> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
> +        false && echo Separator;
> +        _m=false;
> +    else
> +        false && echo Argument;
> +        _m=false;
> +        args+=(\"\$_a\");
> +    fi;
> +done"
> +
> +arg_parse="eval $arg_parse_cmd"
> +
> +# Pass in either the current function name, or the name of the script
> +requireargs="eval _func=\"\$FUNCNAME\" ; eval [[ -n \\\"\$_func\\\" ]] || _func=\$0 ; eval _require-args \$_func"
> +
> +function _require-args()
> +{
> +    local _arg
> +    local _args
> +
> +    _args=($@)
> +
> +    for _arg in ${_args[@]:1} ; do
> +       eval "[[ -n \"\${$_arg}\" ]] || fail \"${_args[0]}: Missing $_arg\""
> +    done
> +}
> +
> +function default()
> +{
> +    # l0: eval i="5"
> +    # l1: default_post="eval $1=\"$2\""
> +    # l3: eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval \$1=\\\"$2\\\"\" ; fi"
> +    eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval local \$1=\\\"$2\\\"\" ; else unset default_post ; fi"
> +}
> +
> +function fail()
> +{
> +   echo FATAL "$@" 1>&2
> +   [[ -n "$fail_cleanup" ]] && $fail_cleanup
> +   exit 1
> +}
> +
> +function info()
> +{
> +   echo INFO "$@" 1>&2
> +}
> +
> +function error()
> +{
> +   echo ERROR "$@" 1>&2
> +}
> +
> +RAISIN_HELP+=("help            List available commands")
> +function help()
> +{
> +    echo "Usage: $0 command [arguments...]"
> +    echo "  Run '$0 help' for a list of commands, or '$0 command-help' for"
> +    echo "  help on a particular command."
> +    echo
> +
> +    for i in "${RAISIN_HELP[@]}" ; do
> +       echo "$i"
> +    done
> +
> +    echo
> +
> +    exit 0
> +}
> +
> +function cmdline()
> +{
> +    local cmd;
> +
> +    [[ "$#" -eq "0" ]] && help
> +
> +    $arg_parse
> +
> +    # If the command is not defined, then print help and exit
> +    if ! type "${args[0]}" 2>/dev/null | grep -q 'function' ; then
> +       echo "Unknown function: ${args[0]}"
> +       echo
> +       help
> +    fi
> +
> +    info Running "${args[0]}"
> +    "${args[0]}" "${args[@]:1}" || exit 1
> +
> +    if ! [[ -z "$RET" ]] ; then
> +       echo $RET
> +    fi
> +}
> diff --git a/raise b/raise
> new file mode 100755
> index 0000000..7f3faae
> --- /dev/null
> +++ b/raise
> @@ -0,0 +1,16 @@
> +#!/usr/bin/env bash
> +
> +# Include your defaults
> +if [[ -e "./config" ]] ; then
> +    . ./config
> +fi
> +
> +# To use this as a library, set RAISIN_PATH appropriately
> +[[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
> +
> +# Then as many as the sub-libraries as you need
> +. ${RAISIN_PATH}/core.sh
> +
> +# And do your own thing rather than running commands
> +# I suggest defining a "main" function of your own and running it like this.
> +cmdline "$@"
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 2/8] Remove redundant "source" from component definitions

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> These should be called from other places that have already done the
> appropriate "source"
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Yes, you are right about that. I committed this.


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/grub    | 4 ----
>  components/libvirt | 4 ----
>  components/xen     | 4 ----
>  3 files changed, 12 deletions(-)
> 
> diff --git a/components/grub b/components/grub
> index ab4f583..5a42000 100644
> --- a/components/grub
> +++ b/components/grub
> @@ -1,9 +1,5 @@
>  #!/usr/bin/env bash
>  
> -source "$BASEDIR"/config
> -source "$BASEDIR"/scripts/common-functions.sh
> -
> -
>  function _grub_install_dependencies() {
>      local DEP_Debian_common="build-essential tar autoconf bison flex"
>      local DEP_Debian_x86_32="$DEP_Debian_common"
> diff --git a/components/libvirt b/components/libvirt
> index e35de8c..e22996e 100644
> --- a/components/libvirt
> +++ b/components/libvirt
> @@ -1,9 +1,5 @@
>  #!/usr/bin/env bash
>  
> -source "$BASEDIR"/config
> -source "$BASEDIR"/scripts/common-functions.sh
> -
> -
>  function _libvirt_install_dependencies() {
>      local DEP_Debian_common="build-essential libtool autoconf autopoint \
>                               xsltproc libxml2-utils pkg-config python-dev   \
> diff --git a/components/xen b/components/xen
> index d6286b2..a49a1d1 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -1,9 +1,5 @@
>  #!/usr/bin/env bash
>  
> -source "$BASEDIR"/config
> -source "$BASEDIR"/scripts/common-functions.sh
> -
> -
>  function _xen_install_dependencies() {
>      local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
>               libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
> -- 
> 1.9.1
> 

Re: [PATCH 3/8] Move common-functions.sh and git-checkout.sh into lib

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> "script" implies something which is designed to be run standalone.
> 
> "lib" implies that this is going to be sourced from another bash script.
> 
> Also change "git-checkout" to be a function rather than a script
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>

It makes sense to separate library scripts from executables.
I committed this too.


>  components/grub                      |  2 +-
>  components/libvirt                   |  2 +-
>  components/xen                       |  2 +-
>  {scripts => lib}/common-functions.sh |  0
>  lib/git-checkout.sh                  | 32 ++++++++++++++++++++++++++++++++
>  raise.sh                             |  3 ++-
>  scripts/git-checkout.sh              | 30 ------------------------------
>  unraise.sh                           |  2 +-
>  8 files changed, 38 insertions(+), 35 deletions(-)
>  rename {scripts => lib}/common-functions.sh (100%)
>  create mode 100755 lib/git-checkout.sh
>  delete mode 100755 scripts/git-checkout.sh
> 
> diff --git a/components/grub b/components/grub
> index 5a42000..a5aa27d 100644
> --- a/components/grub
> +++ b/components/grub
> @@ -29,7 +29,7 @@ function grub_build() {
>      cd "$BASEDIR"
>      rm -f memdisk.tar
>      tar cf memdisk.tar -C data grub.cfg
> -    ./scripts/git-checkout.sh $GRUB_UPSTREAM_URL $GRUB_UPSTREAM_REVISION grub-dir
> +    git-checkout $GRUB_UPSTREAM_URL $GRUB_UPSTREAM_REVISION grub-dir
>      cd grub-dir
>      ./autogen.sh
>      ## GRUB32
> diff --git a/components/libvirt b/components/libvirt
> index e22996e..6602dcf 100644
> --- a/components/libvirt
> +++ b/components/libvirt
> @@ -26,7 +26,7 @@ function libvirt_build() {
>      _libvirt_install_dependencies
>  
>      cd "$BASEDIR"
> -    ./scripts/git-checkout.sh $LIBVIRT_UPSTREAM_URL $LIBVIRT_UPSTREAM_REVISION libvirt-dir
> +    git-checkout $LIBVIRT_UPSTREAM_URL $LIBVIRT_UPSTREAM_REVISION libvirt-dir
>      cd libvirt-dir
>      CFLAGS="-I$INST_DIR/$PREFIX/include" \
>      LDFLAGS="-L$INST_DIR/$PREFIX/lib -Wl,-rpath-link=$INST_DIR/$PREFIX/lib" \
> diff --git a/components/xen b/components/xen
> index a49a1d1..70b72b0 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -23,7 +23,7 @@ function xen_build() {
>      _xen_install_dependencies
>  
>      cd "$BASEDIR"
> -    ./scripts/git-checkout.sh $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
> +    git-checkout $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
>      cd xen-dir
>      ./configure --prefix=$PREFIX
>      $MAKE
> diff --git a/scripts/common-functions.sh b/lib/common-functions.sh
> similarity index 100%
> rename from scripts/common-functions.sh
> rename to lib/common-functions.sh
> diff --git a/lib/git-checkout.sh b/lib/git-checkout.sh
> new file mode 100755
> index 0000000..2ca8f25
> --- /dev/null
> +++ b/lib/git-checkout.sh
> @@ -0,0 +1,32 @@
> +#!/usr/bin/env bash
> +
> +function git-checkout() {
> +    if [[ $# -lt 3 ]]
> +    then
> +	echo "Usage: $0 <tree> <tag> <dir>"
> +	exit 1
> +    fi
> +
> +    TREE=$1
> +    TAG=$2
> +    DIR=$3
> +
> +    set -e
> +
> +    if [[ ! -d $DIR-remote ]]
> +    then
> +	rm -rf $DIR-remote $DIR-remote.tmp
> +	mkdir -p $DIR-remote.tmp; rmdir $DIR-remote.tmp
> +	$GIT clone $TREE $DIR-remote.tmp
> +	if [[ "$TAG" ]]
> +	then
> +	    cd $DIR-remote.tmp
> +	    $GIT branch -D dummy >/dev/null 2>&1 ||:
> +	    $GIT checkout -b dummy $TAG
> +	    cd ..
> +	fi
> +	mv $DIR-remote.tmp $DIR-remote
> +    fi
> +    rm -f $DIR
> +    ln -sf $DIR-remote $DIR
> +}
> diff --git a/raise.sh b/raise.sh
> index 3c8281e..422fbe4 100755
> --- a/raise.sh
> +++ b/raise.sh
> @@ -3,7 +3,8 @@
>  set -e
>  
>  source config
> -source scripts/common-functions.sh
> +source lib/common-functions.sh
> +source lib/git-checkout.sh
>  
>  _help() {
>      echo "Usage: ./build.sh <options> <command>"
> diff --git a/scripts/git-checkout.sh b/scripts/git-checkout.sh
> deleted file mode 100755
> index 912bfae..0000000
> --- a/scripts/git-checkout.sh
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -#!/usr/bin/env bash
> -
> -if [[ $# -lt 3 ]]
> -then
> -	echo "Usage: $0 <tree> <tag> <dir>"
> -	exit 1
> -fi
> -
> -TREE=$1
> -TAG=$2
> -DIR=$3
> -
> -set -e
> -
> -if [[ ! -d $DIR-remote ]]
> -then
> -	rm -rf $DIR-remote $DIR-remote.tmp
> -	mkdir -p $DIR-remote.tmp; rmdir $DIR-remote.tmp
> -	$GIT clone $TREE $DIR-remote.tmp
> -	if [[ "$TAG" ]]
> -	then
> -		cd $DIR-remote.tmp
> -		$GIT branch -D dummy >/dev/null 2>&1 ||:
> -		$GIT checkout -b dummy $TAG
> -		cd ..
> -	fi
> -	mv $DIR-remote.tmp $DIR-remote
> -fi
> -rm -f $DIR
> -ln -sf $DIR-remote $DIR
> diff --git a/unraise.sh b/unraise.sh
> index 2f08901..50ce310 100755
> --- a/unraise.sh
> +++ b/unraise.sh
> @@ -3,7 +3,7 @@
>  set -e
>  
>  source config
> -source scripts/common-functions.sh
> +source lib/common-functions.sh
>  
>  
>  # start execution
> -- 
> 1.9.1
> 

Re: [PATCH 6/8] xen: Replace iasl with acpica-tools

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> acpica-tools is the new package that replaces iasl.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked and committed


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/xen | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/components/xen b/components/xen
> index 70b72b0..e8cbb2b 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -12,7 +12,7 @@ function _xen_install_dependencies() {
>      local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
>               ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
>               patch pixman-devel glibc-devel bridge-utils grub2"
> -    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 iasl texinfo"
> +    local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
>      local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
>  
>      echo installing Xen dependencies
> -- 
> 1.9.1
> 

Re: [PATCH 7/8] xen: Require wget

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> The xen build requires eiter wget or ftp.  Use wget.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked and committed

> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/xen | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/components/xen b/components/xen
> index e8cbb2b..7a9f22d 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -3,7 +3,7 @@
>  function _xen_install_dependencies() {
>      local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
>               libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
> -             libssl-dev libpixman-1-dev bridge-utils"
> +             libssl-dev libpixman-1-dev bridge-utils wget"
>      local DEP_Debian_x86_32="$DEP_Debian_common bcc iasl bin86 texinfo"
>      local DEP_Debian_x86_64="$DEP_Debian_x86_32 libc6-dev-i386"
>      local DEP_Debian_arm32="$DEP_Debian_common libfdt-dev"
> @@ -11,7 +11,7 @@ function _xen_install_dependencies() {
>  
>      local DEP_Fedora_common="make gcc python-devel gettext libuuid-devel   \
>               ncurses-devel glib2-devel libaio-devel openssl-devel yajl-devel   \
> -             patch pixman-devel glibc-devel bridge-utils grub2"
> +             patch pixman-devel glibc-devel bridge-utils grub2 wget"
>      local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
>      local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
>  
> -- 
> 1.9.1
> 

Re: [PATCH 1/8] Add core.sh and wrapper function

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> Add core functionality and an executable to run it
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  lib/core.sh | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  raise       |  16 +++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 lib/core.sh
>  create mode 100755 raise
>
> diff --git a/lib/core.sh b/lib/core.sh
> new file mode 100644
> index 0000000..46b02e0
> --- /dev/null
> +++ b/lib/core.sh
> @@ -0,0 +1,147 @@
> +#!/usr/bin/env bash
> +
> +RAISIN_HELP=()
> +
> +# Core calling convention functionality
> +# 
> +# $arg_parse
> +#
> +#   Parse $@.  Begin with every element that a '=' in it being treated
> +#   as a variable assignment: declare the LHS a local variable and set
> +#   it to the RHS.  Once you reach an element without a '=' in it, or
> +#   a '--', put all further elements into the array args.
> +# 
> +# $requireargs VARNAME1 [VARNAME2, ...]
> +#
> +#   Check to see that VARNAME1 is defined, and throw an error if not.
> +#
> +# default VARNAME "VALUE" ; $default_post
> +#
> +#   Check to see is VARNAME is defined; if not, declare it a local
> +#   variable and set it to VALUE.
> +#
> +# Example usage:
> +#
> +#   function log()
> +#   {
> +#       local i;
> +#
> +#       $arg_parse
> +#
> +#       default tmpdir "/tmp"; $default_post
> +#
> +#       $requireargs filename
> +#
> +#       for $i in ${args[@]} ; do
> +#           echo "$i" > $tmpdir/$filename
> +#       done
> +#
> +#       [[ -n "$flower" ] && echo $flower
> +#   }
> +
> +arg_parse_cmd=\
> +"local -a args;
> +local _a;
> +local _vn;
> +local _m;
> +
> +_m=true;
> +
> +for _a in \"\$@\" ; do
> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
> +        false && echo Parameter;
> +        _vn=\${_a%%=*};
> +        eval \"local \$_vn\";
> +        eval \"\$_a\";
> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
> +        false && echo Separator;
> +        _m=false;
> +    else
> +        false && echo Argument;
> +        _m=false;
> +        args+=(\"\$_a\");
> +    fi;
> +done"

I am sorry but I cannot bear myself to introduce so much complexity into
the world.  raisin is supposed to be easy to read.  If we end up
actually needing something so powerful and so complex in the future we
can import it then, but I think that at the moment we can do nicely
without it.

But I do like the idea of a single unified raise script executing
commands.


> +arg_parse="eval $arg_parse_cmd"
> +
> +# Pass in either the current function name, or the name of the script
> +requireargs="eval _func=\"\$FUNCNAME\" ; eval [[ -n \\\"\$_func\\\" ]] || _func=\$0 ; eval _require-args \$_func"
> +
> +function _require-args()
> +{
> +    local _arg
> +    local _args
> +
> +    _args=($@)
> +
> +    for _arg in ${_args[@]:1} ; do
> +	eval "[[ -n \"\${$_arg}\" ]] || fail \"${_args[0]}: Missing $_arg\""
> +    done
> +}
> +
> +function default()
> +{
> +    # l0: eval i="5"
> +    # l1: default_post="eval $1=\"$2\""
> +    # l3: eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval \$1=\\\"$2\\\"\" ; fi"
> +    eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval local \$1=\\\"$2\\\"\" ; else unset default_post ; fi"
> +}
> +
> +function fail()
> +{
> +   echo FATAL "$@" 1>&2
> +   [[ -n "$fail_cleanup" ]] && $fail_cleanup
> +   exit 1
> +}
> +
> +function info()
> +{
> +   echo INFO "$@" 1>&2
> +}
> +
> +function error()
> +{
> +   echo ERROR "$@" 1>&2
> +}
> +
> +RAISIN_HELP+=("help            List available commands")
> +function help()
> +{
> +    echo "Usage: $0 command [arguments...]"
> +    echo "  Run '$0 help' for a list of commands, or '$0 command-help' for"
> +    echo "  help on a particular command."
> +    echo
> +
> +    for i in "${RAISIN_HELP[@]}" ; do
> +	echo "$i"
> +    done
> +
> +    echo
> +
> +    exit 0
> +}
> +
> +function cmdline()
> +{
> +    local cmd;
> +
> +    [[ "$#" -eq "0" ]] && help
> +
> +    $arg_parse
> +
> +    # If the command is not defined, then print help and exit
> +    if ! type "${args[0]}" 2>/dev/null | grep -q 'function' ; then
> +	echo "Unknown function: ${args[0]}"
> +	echo
> +	help
> +    fi
> +
> +    info Running "${args[0]}"
> +    "${args[0]}" "${args[@]:1}" || exit 1
> +
> +    if ! [[ -z "$RET" ]] ; then
> +	echo $RET
> +    fi
> +}
> diff --git a/raise b/raise
> new file mode 100755
> index 0000000..7f3faae
> --- /dev/null
> +++ b/raise
> @@ -0,0 +1,16 @@
> +#!/usr/bin/env bash

It is important to "set -e" immediately to catch all possible errors:

 "-e  Exit immediately if a command exits with a non-zero status."

Then you can drop set -e from raise.sh and unraise.sh when you turn them
into libraries.


> +# Include your defaults
> +if [[ -e "./config" ]] ; then
> +    . ./config
> +fi
> +
> +# To use this as a library, set RAISIN_PATH appropriately
> +[[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
> +
> +# Then as many as the sub-libraries as you need
> +. ${RAISIN_PATH}/core.sh
> +
> +# And do your own thing rather than running commands
> +# I suggest defining a "main" function of your own and running it like this.
> +cmdline "$@"
> -- 
> 1.9.1
> 

Re: [PATCH 4/8] Import raise.sh and unraise.sh into library

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> Make as few changes as possible to begin with, just to separate code
> motion from changes.
> 
> For now, remove raise.sh and unraise.sh from package creation, until
> we can figure out what to do instead.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Good idea, I like it.


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  raise.sh => lib/build.sh | 62 ++++++++++++++++--------------------------------
>  raise                    |  6 +++++
>  scripts/mkdeb            |  3 ++-
>  scripts/mkrpm            |  5 ++--
>  unraise.sh               | 17 -------------
>  5 files changed, 31 insertions(+), 62 deletions(-)
>  rename raise.sh => lib/build.sh (77%)
>  delete mode 100755 unraise.sh
> 
> diff --git a/raise.sh b/lib/build.sh
> similarity index 77%
> rename from raise.sh
> rename to lib/build.sh
> index 422fbe4..ab1e087 100755
> --- a/raise.sh
> +++ b/lib/build.sh
> @@ -2,10 +2,6 @@
>  
>  set -e
>  
> -source config
> -source lib/common-functions.sh
> -source lib/git-checkout.sh
> -
>  _help() {
>      echo "Usage: ./build.sh <options> <command>"
>      echo "where options are:"
> @@ -18,7 +14,9 @@ _help() {
>      echo "    configure            Configure the system  (requires sudo)"
>  }
>  
> -_build() {
> +build() {
> +    $arg_parse
> +
>      if [[ $YES != "y" ]]
>      then
>          echo "Do you want Raisin to automatically install build time dependencies for you? (y/n)"
> @@ -50,7 +48,20 @@ _build() {
>      build_package xen-system
>  }
>  
> -_install() {
> +unraise() {
> +    $arg_parse
> +
> +    for_each_component clean
> +
> +    uninstall_package xen-system
> +    for_each_component unconfigure
> +
> +    rm -rf "$INST_DIR"
> +}
> +
> +install() {
> +    $arg_parse
> +
>      # need single braces for filename matching expansion
>      if [ ! -f xen-sytem*rpm ] && [ ! -f xen-system*deb ]
>      then
> @@ -60,7 +71,9 @@ _install() {
>      install_package xen-system
>  }
>  
> -_configure() {
> +configure() {
> +    $arg_parse
> +
>      if [[ $YES != "y" ]]
>      then
>          echo "Proceeding we'll make changes to the running system,"
> @@ -82,38 +95,3 @@ _configure() {
>      for_each_component configure
>  }
>  
> -# start execution
> -common_init
> -
> -# parameters check
> -export VERBOSE=0
> -export YES="n"
> -export NO_DEPS=0
> -while [[ $# -gt 1 ]]
> -do
> -  if [[ "$1" = "-v" || "$1" = "--verbose" ]]
> -  then
> -    VERBOSE=1
> -    shift 1
> -  elif [[ "$1" = "-y" || "$1" = "--yes" ]]
> -  then
> -    YES="y"
> -    shift 1
> -  else
> -    _help
> -    exit 1
> -  fi
> -done
> -
> -case "$1" in
> -    "build" | "install" | "configure" )
> -        COMMAND=$1
> -        ;;
> -    *)
> -        _help
> -        exit 1
> -        ;;
> -esac

I would move the command line parsing to raise so that it can work
without core.sh.


> -_$COMMAND
> -
> diff --git a/raise b/raise
> index 7f3faae..142956d 100755
> --- a/raise
> +++ b/raise
> @@ -10,6 +10,12 @@ fi
>  
>  # Then as many as the sub-libraries as you need
>  . ${RAISIN_PATH}/core.sh
> +. ${RAISIN_PATH}/common-functions.sh
> +. ${RAISIN_PATH}/git-checkout.sh
> +. ${RAISIN_PATH}/build.sh
> +
> +# Set up basic functionality
> +common_init
>  
>  # And do your own thing rather than running commands
>  # I suggest defining a "main" function of your own and running it like this.
> diff --git a/scripts/mkdeb b/scripts/mkdeb
> index 46ade07..cb2a1b6 100755
> --- a/scripts/mkdeb
> +++ b/scripts/mkdeb
> @@ -35,7 +35,8 @@ mkdir -p deb/opt/raisin
>  cp -r data deb/opt/raisin
>  cp -r components deb/opt/raisin
>  cp -r scripts deb/opt/raisin
> -cp config raise.sh unraise.sh deb/opt/raisin
> +# FIXME
> +#cp config raise.sh unraise.sh deb/opt/raisin

This can be easily fixed by copying lib and raise.

>  
>  # Debian doesn't use /usr/lib64 for 64-bit libraries
> diff --git a/scripts/mkrpm b/scripts/mkrpm
> index c530466..90d9bdc 100755
> --- a/scripts/mkrpm
> +++ b/scripts/mkrpm
> @@ -48,8 +48,9 @@ cp -r "$BASEDIR"/data \$RPM_BUILD_ROOT/opt/raisin
>  cp -r "$BASEDIR"/components \$RPM_BUILD_ROOT/opt/raisin
>  cp -r "$BASEDIR"/scripts \$RPM_BUILD_ROOT/opt/raisin
>  cp "$BASEDIR"/config \$RPM_BUILD_ROOT/opt/raisin
> -cp "$BASEDIR"/raise.sh \$RPM_BUILD_ROOT/opt/raisin
> -cp "$BASEDIR"/unraise.sh \$RPM_BUILD_ROOT/opt/raisin
> +# FIXME
> +# cp "$BASEDIR"/raise.sh \$RPM_BUILD_ROOT/opt/raisin
> +# cp "$BASEDIR"/unraise.sh \$RPM_BUILD_ROOT/opt/raisin

same


>  %clean
>  
> diff --git a/unraise.sh b/unraise.sh
> deleted file mode 100755
> index 50ce310..0000000
> --- a/unraise.sh
> +++ /dev/null
> @@ -1,17 +0,0 @@
> -#!/usr/bin/env bash
> -
> -set -e
> -
> -source config
> -source lib/common-functions.sh
> -
> -
> -# start execution
> -common_init
> -
> -for_each_component clean
> -
> -uninstall_package xen-system
> -for_each_component unconfigure
> -
> -rm -rf "$INST_DIR"
> -- 
> 1.9.1
> 

Re: [PATCH 5/8] Allow the user's config to live outside of git

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> Move the default config to "defconfig" and add config to .gitignore.
> If config doesn't exist, then copy defconfig to it in common_init.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

but you also need to update the files copied by mkdeb and mkrpm


> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  .gitignore          |  1 +
>  config => defconfig |  0
>  raise               | 13 ++++++++-----
>  3 files changed, 9 insertions(+), 5 deletions(-)
>  rename config => defconfig (100%)
> 
> diff --git a/.gitignore b/.gitignore
> index 47e9874..35a46c6 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -28,6 +28,7 @@ cscope.in.out
>  cscope.out
>  cscope.po.out
>  .config
> +config
>  
>  xen-dir
>  xen-dir-remote
> diff --git a/config b/defconfig
> similarity index 100%
> rename from config
> rename to defconfig
> diff --git a/raise b/raise
> index 142956d..ae435d9 100755
> --- a/raise
> +++ b/raise
> @@ -1,10 +1,5 @@
>  #!/usr/bin/env bash
>  
> -# Include your defaults
> -if [[ -e "./config" ]] ; then
> -    . ./config
> -fi
> -
>  # To use this as a library, set RAISIN_PATH appropriately
>  [[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
>  
> @@ -14,6 +9,14 @@ fi
>  . ${RAISIN_PATH}/git-checkout.sh
>  . ${RAISIN_PATH}/build.sh
>  
> +# Include your defaults
> +if [[ ! -e "./config" ]] ; then
> +   echo "No config file found, copying default config"
> +   cp defconfig config
> +fi
> +
> +. ./config
> +
>  # Set up basic functionality
>  common_init
>  
> -- 
> 1.9.1
> 

Re: [PATCH 8/8] Refactor package dependency checking and installation

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> First, create a new global variable, PKGTYPE.  At the moment support "deb" and "rpm".
> 
> Define _check-package-$PKGTYPE which returns true if the package is
> installed, false otherwise, and _install-package-$PKGTYPE which will
> install a list of packages.
> 
> Define check-package(), which will take a list of packages, and check
> to see if they're installed.  Any missing packages will be added to an
> array called "missing".
> 
> Change _${COMPONENT}_install_dependencies to
> ${COMPONENT}_check_package.  Have these call check-package.
> 
> Don't call _${COMPONENT}_install_dependencies from ${COMPONENT}_build.
> 
> Define check-builddeps().  Define an empty "missing" array.  Call
> check-package for "raisin" dependincies (like git and rpmbuild).  Then
> call for_each_component check_package.
> 
> At this point we have an array with all missing packages.  If it's
> empty, be happy.  If it's non-empty, and deps=true, try to install the
> packages; otherwise print the missing packages and exit.
> 
> Add install-builddeps(), which is basically check-builddeps() with
> deps=true by default.
> 
> Call check-builddeps from build() to close the loop.

I think that this is a good idea. It is nice to be able to check whether
the deps are already installed before going ahead and installing them. I
also like the new command to install dependencies and do nothing else.

It needs to be changed a bit to work without core.sh but it is mostly
OK.


> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  components/grub         |  6 ++--
>  components/libvirt      |  6 ++--
>  components/xen          | 10 +++---
>  lib/build.sh            | 87 +++++++++++++++++++++++++++++++++++------------
>  lib/common-functions.sh | 89 +++++++++++++++++++++++++++++++++++++++----------
>  5 files changed, 148 insertions(+), 50 deletions(-)
> 
> diff --git a/components/grub b/components/grub
> index a5aa27d..839e001 100644
> --- a/components/grub
> +++ b/components/grub
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env bash
>  
> -function _grub_install_dependencies() {
> +function grub_check_package() {
>      local DEP_Debian_common="build-essential tar autoconf bison flex"
>      local DEP_Debian_x86_32="$DEP_Debian_common"
>      local DEP_Debian_x86_64="$DEP_Debian_common libc6-dev-i386"
> @@ -18,8 +18,8 @@ function _grub_install_dependencies() {
>          echo grub is only supported on x86_32 and x86_64
>          return
>      fi
> -    echo installing Grub dependencies
> -    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
> +    echo checking Grub dependencies
> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
>  }
>  

Need to change build not to call _grub_install_dependencies


> diff --git a/components/libvirt b/components/libvirt
> index 6602dcf..b106970 100644
> --- a/components/libvirt
> +++ b/components/libvirt
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env bash
>  
> -function _libvirt_install_dependencies() {
> +function libvirt_check_package() {
>      local DEP_Debian_common="build-essential libtool autoconf autopoint \
>                               xsltproc libxml2-utils pkg-config python-dev   \
>                               libxml-xpath-perl libyajl-dev libxml2-dev      \
> @@ -18,8 +18,8 @@ function _libvirt_install_dependencies() {
>      local DEP_Fedora_x86_32="$DEP_Fedora_common"
>      local DEP_Fedora_x86_64="$DEP_Fedora_common"
>  
> -    echo installing Libvirt dependencies
> -    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
> +    echo checking Libvirt dependencies
> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
>  }

Same here


>  function libvirt_build() {
> diff --git a/components/xen b/components/xen
> index 7a9f22d..ce46e3d 100644
> --- a/components/xen
> +++ b/components/xen
> @@ -1,6 +1,8 @@
>  #!/usr/bin/env bash
>  
> -function _xen_install_dependencies() {
> +function xen_check_package() {
> +    $requireargs DISTRO ARCH
> +
>      local DEP_Debian_common="build-essential python-dev gettext uuid-dev   \
>               libncurses5-dev libyajl-dev libaio-dev pkg-config libglib2.0-dev  \
>               libssl-dev libpixman-1-dev bridge-utils wget"
> @@ -15,13 +17,11 @@ function _xen_install_dependencies() {
>      local DEP_Fedora_x86_32="$DEP_Fedora_common dev86 acpica-tools texinfo"
>      local DEP_Fedora_x86_64="$DEP_Fedora_x86_32 glibc-devel.i686"
>  
> -    echo installing Xen dependencies
> -    eval install_dependencies \$DEP_"$DISTRO"_"$ARCH"
> +    echo Checking Xen dependencies
> +    eval check-package \$DEP_"$DISTRO"_"$ARCH"
>  }
>  
>  function xen_build() {
> -    _xen_install_dependencies
> -
>      cd "$BASEDIR"
>      git-checkout $XEN_UPSTREAM_URL $XEN_UPSTREAM_REVISION xen-dir
>      cd xen-dir
> diff --git a/lib/build.sh b/lib/build.sh
> index ab1e087..a453874 100755
> --- a/lib/build.sh
> +++ b/lib/build.sh
> @@ -2,32 +2,72 @@
>  
>  set -e
>  
> -_help() {
> -    echo "Usage: ./build.sh <options> <command>"
> +RAISIN_HELP+=("check-builddep   Check to make sure we have all dependencies installed")
> +function check-builddep() {
> +    local -a missing
> +
> +    $arg_parse
> +
> +    default deps "false" ; $default_post
> +    
> +    $requireargs PKGTYPE DISTRO
> +
> +    check-package git
> +
> +    if [[ $DISTRO = "Fedora" ]] ; then
> +        check-package rpm-build
> +    fi
> +
> +    for_each_component check_package
> +
> +    if [[ -n "${missing[@]}" ]] ; then
> +	echo "Missing packages: ${missing[@]}"
> +	if $deps ; then
> +	    echo "Installing..."
> +	    install-package "${missing[@]}"
> +	else
> +	    echo "Please install, or run ./raise install-builddep"
> +	    exit 1
> +	fi
> +    fi
> +}

indentation is wrong


> +RAISIN_HELP+=("install-builddep Install all packages required for build.  Uses sudo.")
> +function install-builddep() {
> +    local pkgtype
> +
> +    $arg_parse
> +
> +    check-builddep deps=true
> +}
> +
> +build-help() {
> +    echo "Usage: $0 build opt=val"
>      echo "where options are:"
> -    echo "    -n | --no-deps       Do no install build-time dependencies"
> -    echo "    -v | --verbose       Verbose"
> -    echo "    -y | --yes           Do not ask questions and continue"
> -    echo "where commands are:"
> -    echo "    build                Build the components enabled in config"
> -    echo "    install              Install binaries under /  (requires sudo)"
> -    echo "    configure            Configure the system  (requires sudo)"
> +    echo "    yes=(true|false)     Don't ask before doing anything dangerous.  Defaults to false."
> +    echo "    deps=(true|false)    Attempt to install dependencies.  Defaults to false."
> +    echo "    verbose=(true|false) Increased verbosity.  Defaults to false."
>  }

I would keep the old style help and options for simplicity. Removing the
deps option entirely is OK I think.


> +RAISIN_HELP+=("build           Clone and build components enabled in config")
>  build() {
>      $arg_parse
>  
> -    if [[ $YES != "y" ]]
> -    then
> -        echo "Do you want Raisin to automatically install build time dependencies for you? (y/n)"
> +    default deps "false" ; $default_post
> +    default verbose "false" ; $default_post
> +    default yes "false" ; $default_post
> +
> +    if $deps && ! $yes ; then
> +        echo "WARNING: You have chosen to automatically install software as root."
> +        echo -n "Are you sure that you want to continue? (y/n)"
>          while read answer
>          do
>              if [[ "$answer" = "n" ]]
>              then
> -                NO_DEPS=1
> -                break
> +                exit 0
>              elif [[ "$answer" = "y" ]]
>              then
> +		yes="true"
>                  break
>              else
>                  echo "Reply y or n"

I would keep the warning as it was before


> @@ -35,12 +75,10 @@ build() {
>          done
>      fi
>  
> +    check-builddep
> +    
>      mkdir -p "$INST_DIR" &>/dev/null
> -    install_dependencies git
> -    if [[ $DISTRO = "Fedora" ]]
> -    then
> -        install_dependencies rpm-build
> -    fi
> +
>      # build and install under $DESTDIR
>      for_each_component clean
>      for_each_component build
> @@ -59,6 +97,7 @@ unraise() {
>      rm -rf "$INST_DIR"
>  }
>  
> +RAISIN_HELP+=("install         Install binaries under /  (requires sudo)")
>  install() {
>      $arg_parse
>  
> @@ -71,12 +110,15 @@ install() {
>      install_package xen-system
>  }
>  
> +RAISIN_HELP+=("configure       Configure the system  (requires sudo)")
>  configure() {
>      $arg_parse
>  
> -    if [[ $YES != "y" ]]
> -    then
> -        echo "Proceeding we'll make changes to the running system,"
> +    default verbose "false" ; $default_post
> +    default yes "false" ; $default_post
> +
> +    if $deps && ! $yes ; then
> +        echo "WARNING: Proceeding we'll make changes to the running system,"
>          echo "are you sure that you want to continue? (y/n)"
>          while read answer
>          do
> @@ -85,6 +127,7 @@ configure() {
>                  exit 0
>              elif [[ "$answer" = "y" ]]
>              then
> +		yes="true"
>                  break
>              else
>                  echo "Reply y or n"
> diff --git a/lib/common-functions.sh b/lib/common-functions.sh
> index cfa0c7f..fd73f08 100644
> --- a/lib/common-functions.sh
> +++ b/lib/common-functions.sh
> @@ -97,15 +97,23 @@ function get_distro() {
>      case "$os_VENDOR" in
>          "Debian"* | "Ubuntu"* | "LinuxMint"* )
>              DISTRO="Debian"
> +	    PKGTYPE="deb"
> +            ;;
> +        "Fedora" )
> +            DISTRO="Fedora"
> +	    PKGTYPE="rpm"
>              ;;
>          "SUSE"* )
>              DISTRO="SUSE"
> +	    PKGTYPE="rpm"
>              ;;
>          "OpenSUSE"* | "openSUSE"* )
>              DISTRO="openSUSE"
> +	    PKGTYPE="rpm"
>              ;;
>          "Red"* | "CentOS"* )
>              DISTRO="CentOS"
> +	    PKGTYPE="rpm"
>              ;;
>          *)
>              DISTRO=$os_VENDOR
> @@ -114,6 +122,7 @@ function get_distro() {
>  
>      export os_VENDOR os_RELEASE os_UPDATE os_CODENAME
>      export DISTRO
> +    export PKGTYPE
>  }
>  
>  function get_arch() {
> @@ -122,24 +131,70 @@ function get_arch() {
>                  -e s/aarch64/arm64/`
>  }
>  
> -function install_dependencies() {
> -    if [[ "$NO_DEPS" && "$NO_DEPS" -eq 1 ]]
> -    then
> -        echo "Not installing any dependencies, as requested."
> -        echo "Depency list: $*"
> -        return 0
> +function _check-package-deb() {
> +    $arg_parse
> +
> +    $verbose && echo "Checking for package ${args[0]}"
> +
> +    if dpkg -s "${args[0]}" 2>/dev/null | grep -q "Status:.*installed" ; then
> +	return 0
> +    else
> +	return 1
>      fi
> -    case $DISTRO in
> -        "Debian" )
> -        $SUDO apt-get install -y $*
> -        ;;
> -        "Fedora" )
> -        $SUDO yum install -y $*
> -        ;;
> -        * )
> -        echo "I don't know how to install dependencies on $DISTRO"
> -        ;;
> -    esac
> +}
> +
> +function _install-package-deb() {
> +    $arg_parse
> +
> +    $requireargs SUDO
> +
> +    $SUDO apt-get install -y "${args[@]}"
> +}
> +
> +function _check-package-rpm() {
> +    local pstatus
> +
> +    $arg_parse
> +
> +    $verbose && echo "Checking for package ${args[0]}"
> +
> +    if rpm -q "${args[0]}" 2>&1 >/dev/null ; then
> +	return 0
> +    else
> +	return 1
> +    fi
> +}

Indentation and coding style


> +function _install-package-rpm() {
> +    $arg_parse
> +
> +    $requireargs SUDO
> +
> +    $SUDO yum install -y "${args[@]}"
> +}
> +
> +# Modifies inherited variable "missing"
> +function check-package() {
> +    local p
> +
> +    $arg_parse
> +
> +    $requireargs PKGTYPE
> +
> +    for p in "${args[@]}" ; do
> +	if ! _check-package-${PKGTYPE} $p ; then
> +	    missing+=("$p")
> +	fi
> +    done

same here

> +}
> +
> +function install-package() {
> +    $arg_parse
> +
> +    $requireargs PKGTYPE
> +
> +    _install-package-${PKGTYPE} "${args[@]}"
>  }
>  
>  function start_initscripts() {
> -- 
> 1.9.1
> 

Re: [PATCH 1/8] Add core.sh and wrapper function

[Stefano Stabellini]

On Thu, 9 Apr 2015, George Dunlap wrote:
> On Thu, Apr 9, 2015 at 8:29 PM, George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
> > Add core functionality and an executable to run it
> >
> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> > ---
> > CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> Hmm, not sure where the 0/8 for this patch went... anyway, you can
> also find this series here:
> 
> git://xenbits.xenproject.org/people/gdunlap/raisin.git out/librarify/v1
> 

I have applied as is patches #2 #3 #6 and #7.
I dropped patch #1 and applied the others to the existing code base. I
needed to make a few changes to do so, but they are mostly unmodified.


> > ---
> >  lib/core.sh | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  raise       |  16 +++++++
> >  2 files changed, 163 insertions(+)
> >  create mode 100644 lib/core.sh
> >  create mode 100755 raise
> >
> > diff --git a/lib/core.sh b/lib/core.sh
> > new file mode 100644
> > index 0000000..46b02e0
> > --- /dev/null
> > +++ b/lib/core.sh
> > @@ -0,0 +1,147 @@
> > +#!/usr/bin/env bash
> > +
> > +RAISIN_HELP=()
> > +
> > +# Core calling convention functionality
> > +#
> > +# $arg_parse
> > +#
> > +#   Parse $@.  Begin with every element that a '=' in it being treated
> > +#   as a variable assignment: declare the LHS a local variable and set
> > +#   it to the RHS.  Once you reach an element without a '=' in it, or
> > +#   a '--', put all further elements into the array args.
> > +#
> > +# $requireargs VARNAME1 [VARNAME2, ...]
> > +#
> > +#   Check to see that VARNAME1 is defined, and throw an error if not.
> > +#
> > +# default VARNAME "VALUE" ; $default_post
> > +#
> > +#   Check to see is VARNAME is defined; if not, declare it a local
> > +#   variable and set it to VALUE.
> > +#
> > +# Example usage:
> > +#
> > +#   function log()
> > +#   {
> > +#       local i;
> > +#
> > +#       $arg_parse
> > +#
> > +#       default tmpdir "/tmp"; $default_post
> > +#
> > +#       $requireargs filename
> > +#
> > +#       for $i in ${args[@]} ; do
> > +#           echo "$i" > $tmpdir/$filename
> > +#       done
> > +#
> > +#       [[ -n "$flower" ] && echo $flower
> > +#   }
> > +
> > +arg_parse_cmd=\
> > +"local -a args;
> > +local _a;
> > +local _vn;
> > +local _m;
> > +
> > +_m=true;
> > +
> > +for _a in \"\$@\" ; do
> > +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
> > +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
> > +        false && echo Parameter;
> > +        _vn=\${_a%%=*};
> > +        eval \"local \$_vn\";
> > +        eval \"\$_a\";
> > +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
> > +        false && echo Separator;
> > +        _m=false;
> > +    else
> > +        false && echo Argument;
> > +        _m=false;
> > +        args+=(\"\$_a\");
> > +    fi;
> > +done"
> > +
> > +arg_parse="eval $arg_parse_cmd"
> > +
> > +# Pass in either the current function name, or the name of the script
> > +requireargs="eval _func=\"\$FUNCNAME\" ; eval [[ -n \\\"\$_func\\\" ]] || _func=\$0 ; eval _require-args \$_func"
> > +
> > +function _require-args()
> > +{
> > +    local _arg
> > +    local _args
> > +
> > +    _args=($@)
> > +
> > +    for _arg in ${_args[@]:1} ; do
> > +       eval "[[ -n \"\${$_arg}\" ]] || fail \"${_args[0]}: Missing $_arg\""
> > +    done
> > +}
> > +
> > +function default()
> > +{
> > +    # l0: eval i="5"
> > +    # l1: default_post="eval $1=\"$2\""
> > +    # l3: eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval \$1=\\\"$2\\\"\" ; fi"
> > +    eval "if [[ -z \"\$$1\" ]] ; then default_post=\"eval local \$1=\\\"$2\\\"\" ; else unset default_post ; fi"
> > +}
> > +
> > +function fail()
> > +{
> > +   echo FATAL "$@" 1>&2
> > +   [[ -n "$fail_cleanup" ]] && $fail_cleanup
> > +   exit 1
> > +}
> > +
> > +function info()
> > +{
> > +   echo INFO "$@" 1>&2
> > +}
> > +
> > +function error()
> > +{
> > +   echo ERROR "$@" 1>&2
> > +}
> > +
> > +RAISIN_HELP+=("help            List available commands")
> > +function help()
> > +{
> > +    echo "Usage: $0 command [arguments...]"
> > +    echo "  Run '$0 help' for a list of commands, or '$0 command-help' for"
> > +    echo "  help on a particular command."
> > +    echo
> > +
> > +    for i in "${RAISIN_HELP[@]}" ; do
> > +       echo "$i"
> > +    done
> > +
> > +    echo
> > +
> > +    exit 0
> > +}
> > +
> > +function cmdline()
> > +{
> > +    local cmd;
> > +
> > +    [[ "$#" -eq "0" ]] && help
> > +
> > +    $arg_parse
> > +
> > +    # If the command is not defined, then print help and exit
> > +    if ! type "${args[0]}" 2>/dev/null | grep -q 'function' ; then
> > +       echo "Unknown function: ${args[0]}"
> > +       echo
> > +       help
> > +    fi
> > +
> > +    info Running "${args[0]}"
> > +    "${args[0]}" "${args[@]:1}" || exit 1
> > +
> > +    if ! [[ -z "$RET" ]] ; then
> > +       echo $RET
> > +    fi
> > +}
> > diff --git a/raise b/raise
> > new file mode 100755
> > index 0000000..7f3faae
> > --- /dev/null
> > +++ b/raise
> > @@ -0,0 +1,16 @@
> > +#!/usr/bin/env bash
> > +
> > +# Include your defaults
> > +if [[ -e "./config" ]] ; then
> > +    . ./config
> > +fi
> > +
> > +# To use this as a library, set RAISIN_PATH appropriately
> > +[[ -z "$RAISIN_PATH" ]] && RAISIN_PATH="$PWD/lib"
> > +
> > +# Then as many as the sub-libraries as you need
> > +. ${RAISIN_PATH}/core.sh
> > +
> > +# And do your own thing rather than running commands
> > +# I suggest defining a "main" function of your own and running it like this.
> > +cmdline "$@"
> > --
> > 1.9.1
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 1/8] Add core.sh and wrapper function

[George Dunlap]

On 04/10/2015 03:29 PM, Stefano Stabellini wrote:
>> diff --git a/raise b/raise
>> new file mode 100755
>> index 0000000..7f3faae
>> --- /dev/null
>> +++ b/raise
>> @@ -0,0 +1,16 @@
>> +#!/usr/bin/env bash
> 
> It is important to "set -e" immediately to catch all possible errors:
> 
>  "-e  Exit immediately if a command exits with a non-zero status."
> 
> Then you can drop set -e from raise.sh and unraise.sh when you turn them
> into libraries.

"set -e" gets tricky when you sometimes expect commands to fail;
particularly if you may want to do some sort of clean-up.  But let's
cross that bridge when we come to it. :-)

 -George

Re: [PATCH 1/8] Add core.sh and wrapper function

[George Dunlap]

On 04/10/2015 03:29 PM, Stefano Stabellini wrote:
>> +arg_parse_cmd=\
>> +"local -a args;
>> +local _a;
>> +local _vn;
>> +local _m;
>> +
>> +_m=true;
>> +
>> +for _a in \"\$@\" ; do
>> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
>> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
>> +        false && echo Parameter;
>> +        _vn=\${_a%%=*};
>> +        eval \"local \$_vn\";
>> +        eval \"\$_a\";
>> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
>> +        false && echo Separator;
>> +        _m=false;
>> +    else
>> +        false && echo Argument;
>> +        _m=false;
>> +        args+=(\"\$_a\");
>> +    fi;
>> +done"
> 
> I am sorry but I cannot bear myself to introduce so much complexity into
> the world.  raisin is supposed to be easy to read.  If we end up
> actually needing something so powerful and so complex in the future we
> can import it then, but I think that at the moment we can do nicely
> without it.

I can certainly understand the sentiment, but the idea behind this
calling convention is actually to make it *easier* both to read and
debug most of the source file, at the cost of two very complex, but
well-tested and generic macros ($arg_parse and $requireargs).

Consider some criteria:
- Difficulty of writing or reading code that calls a function
- Difficulty of writing a function which will be called
- Availability of sensible defaults
- Ability to make sure all necessary arguments have been specified
- Passing function arguments into sub-functions
- Global variables are nasty to debug

Consider alternate ways of passing arguments into functions:

1. Positional

i.e., "cp SRC DEST"

In this model, it's easy to make sure that all necessary arguments have
been specified; and there are no nasty global variables to work with.

However is really hard to program and read because you have to remember
what argument goes where.  You also have to specify all possible
arguments -- you can't have a default.  And if you need to take an
argument and pass it to another function you call, you have to repeat it
again.

2. Option

i.e., "-v --components='a b'"

This is easier to read if you use long options (or common ones like -v
or -y), and you can also have defaults.  Additionally, you can have a
certain amount of error checking -- if you get an argument you don't
recognize you can throw an error.

But you have to write a lot of boilerplate option processing at the top
of each function; and if you have anything a bit complex (line
"--components= above"), it makes it a lot harder to read.  And you have
to write new code every time, giving you more opportunity for bugs.

Consider the argument parsing code you have in raise at the moment -- it
assumes that you will run a command without any additional parameters.
What if you wanted to do something like this:

"raise -v build xen"

The current code would have to be refactored.

And you have to write a lot of custom code to check to make sure that
all the required arguments have been passed in.

And you have the same problem that if you need to pass an argument
further down the call stack, you have to repeat it again.

3. Global variables

i.e.,
"VAR=foo
 function"

This is a bit easier to read in the sense that you're using full
variable names for options.  It's easier to write, since you don't have
any parsing code.  Additionally, variables can be passed further down
the calling stack automatically without having to be explicitly copied
around.  In theory you should be able to have sensible local defaults.

But you have all the problems with global variables being really hard to
debug.

4. Prefix variable

i.e., "YES=y check-builddep"

This is a bit easier to read in the sense that you're using full
variable names for options.  It's easier to write, since you don't have
any parsing code.  Additionally, variables can be passed further down
the calling stack automatically without having to be explicitly copied
around.  In theory you should be able to have sensible local defaults.

I hadn't thought about this as a calling convention.  It certainly has
the advantage that it's build into bash and fairly well-understood by
experienced users.

But I do think it's a bit unnatural to have to prefix all the variables
to the function.

5. VAR=VAL automatic parsing with inheritance

This allows you to have a rich, consistent way to pass in arguments that
are descriptive.  It's easy to write code to parse the arguments and
check to make sure you have all the ones you need: Just add $arg_parse
and $requireargs at the top of your function.  They are automatically
declared local, so there's no worry about polluting the global
namespace.  They are automatically inherited, so you don't have to keep
passing information further down the stack.  And you can have sensible
defaults.

I think #5 is well worth the little bit of macro hackery confined to a
handful of well-tested macros, particularly if raise becomes a fairly
large and fully-functional script.  I've been using this calling
convention for several years now in my own scripts, so it's pretty well
tested code-wise, feature-wise, and it hasn't become a cancer that
spreads throughout the rest of the code. :-)

 -George

Re: [PATCH 1/8] Add core.sh and wrapper function

[Stefano Stabellini]

On Mon, 13 Apr 2015, George Dunlap wrote:
> On 04/10/2015 03:29 PM, Stefano Stabellini wrote:
> >> +arg_parse_cmd=\
> >> +"local -a args;
> >> +local _a;
> >> +local _vn;
> >> +local _m;
> >> +
> >> +_m=true;
> >> +
> >> +for _a in \"\$@\" ; do
> >> +    false && echo \"Evaluating \${_a} [[ \"\${_a/=}\" = \"\${_a}\" ]]\";
> >> +    if \$_m && [[ \"\${_a/=}\" != \"\${_a}\" ]] ; then
> >> +        false && echo Parameter;
> >> +        _vn=\${_a%%=*};
> >> +        eval \"local \$_vn\";
> >> +        eval \"\$_a\";
> >> +    elif \$_m && [[ \"\${_a}\" == \"--\" ]] ; then
> >> +        false && echo Separator;
> >> +        _m=false;
> >> +    else
> >> +        false && echo Argument;
> >> +        _m=false;
> >> +        args+=(\"\$_a\");
> >> +    fi;
> >> +done"
> > 
> > I am sorry but I cannot bear myself to introduce so much complexity into
> > the world.  raisin is supposed to be easy to read.  If we end up
> > actually needing something so powerful and so complex in the future we
> > can import it then, but I think that at the moment we can do nicely
> > without it.
> 
> I can certainly understand the sentiment, but the idea behind this
> calling convention is actually to make it *easier* both to read and
> debug most of the source file, at the cost of two very complex, but
> well-tested and generic macros ($arg_parse and $requireargs).
> 
> Consider some criteria:
> - Difficulty of writing or reading code that calls a function
> - Difficulty of writing a function which will be called
> - Availability of sensible defaults
> - Ability to make sure all necessary arguments have been specified
> - Passing function arguments into sub-functions
> - Global variables are nasty to debug
>
> Consider alternate ways of passing arguments into functions:
> 
> 1. Positional
> 
> i.e., "cp SRC DEST"
> 
> In this model, it's easy to make sure that all necessary arguments have
> been specified; and there are no nasty global variables to work with.
> 
> However is really hard to program and read because you have to remember
> what argument goes where.  You also have to specify all possible
> arguments -- you can't have a default.  And if you need to take an
> argument and pass it to another function you call, you have to repeat it
> again.
> 
> 2. Option
> 
> i.e., "-v --components='a b'"
> 
> This is easier to read if you use long options (or common ones like -v
> or -y), and you can also have defaults.  Additionally, you can have a
> certain amount of error checking -- if you get an argument you don't
> recognize you can throw an error.
> 
> But you have to write a lot of boilerplate option processing at the top
> of each function; and if you have anything a bit complex (line
> "--components= above"), it makes it a lot harder to read.  And you have
> to write new code every time, giving you more opportunity for bugs.
> 
> Consider the argument parsing code you have in raise at the moment -- it
> assumes that you will run a command without any additional parameters.
> What if you wanted to do something like this:
> 
> "raise -v build xen"
> 
> The current code would have to be refactored.
> 
> And you have to write a lot of custom code to check to make sure that
> all the required arguments have been passed in.
> 
> And you have the same problem that if you need to pass an argument
> further down the call stack, you have to repeat it again.
> 
> 3. Global variables
> 
> i.e.,
> "VAR=foo
>  function"
> 
> This is a bit easier to read in the sense that you're using full
> variable names for options.  It's easier to write, since you don't have
> any parsing code.  Additionally, variables can be passed further down
> the calling stack automatically without having to be explicitly copied
> around.  In theory you should be able to have sensible local defaults.
> 
> But you have all the problems with global variables being really hard to
> debug.
> 
> 4. Prefix variable
> 
> i.e., "YES=y check-builddep"
> 
> This is a bit easier to read in the sense that you're using full
> variable names for options.  It's easier to write, since you don't have
> any parsing code.  Additionally, variables can be passed further down
> the calling stack automatically without having to be explicitly copied
> around.  In theory you should be able to have sensible local defaults.
> 
> I hadn't thought about this as a calling convention.  It certainly has
> the advantage that it's build into bash and fairly well-understood by
> experienced users.
> 
> But I do think it's a bit unnatural to have to prefix all the variables
> to the function.
> 
> 5. VAR=VAL automatic parsing with inheritance
> 
> This allows you to have a rich, consistent way to pass in arguments that
> are descriptive.  It's easy to write code to parse the arguments and
> check to make sure you have all the ones you need: Just add $arg_parse
> and $requireargs at the top of your function.  They are automatically
> declared local, so there's no worry about polluting the global
> namespace.  They are automatically inherited, so you don't have to keep
> passing information further down the stack.  And you can have sensible
> defaults.
> 
> I think #5 is well worth the little bit of macro hackery confined to a
> handful of well-tested macros, particularly if raise becomes a fairly
> large and fully-functional script.  I've been using this calling
> convention for several years now in my own scripts, so it's pretty well
> tested code-wise, feature-wise, and it hasn't become a cancer that
> spreads throughout the rest of the code. :-)


We have two different use cases:

- command line parsing in the main script, raise

- argument parsing in the other functions

I think it might be OK to have something like 5) just for the command
line parsing in raise, for the sake of giving more flexibility to the
user. Also it would be confined to one specific call site. But I am
uncertain that the pros outweigh the cons in this case.

I would rather avoid it in all the other functions. The reason is that
not only is very complex, but it makes the code quite different from
most of the other bash code out there, including DevStack.  For the
principle of least astonishment, I would rather have something more
common and easier to read and guess the meaning of it.

I would be content with requiring a simple comment on top of each new
function that documents which are the arguments of the function.
Otherwise option 4) would also work.

Re: [PATCH 1/8] Add core.sh and wrapper function

[Ian Campbell]

On Mon, 2015-04-13 at 17:13 +0100, Stefano Stabellini wrote:
> I think it might be OK to have something like 5) just for the command
> line parsing in raise, for the sake of giving more flexibility to the
> user. Also it would be confined to one specific call site. But I am
> uncertain that the pros outweigh the cons in this case.

I've only glanced at it, but isn't this largely replicating getopt(1)?
Perhaps without requiring -- on the options, but having the -- is more
conventional I think.

Ian.

Re: [PATCH 1/8] Add core.sh and wrapper function

[George Dunlap]

On 04/14/2015 02:55 PM, Ian Campbell wrote:
> On Mon, 2015-04-13 at 17:13 +0100, Stefano Stabellini wrote:
>> I think it might be OK to have something like 5) just for the command
>> line parsing in raise, for the sake of giving more flexibility to the
>> user. Also it would be confined to one specific call site. But I am
>> uncertain that the pros outweigh the cons in this case.
> 
> I've only glanced at it, but isn't this largely replicating getopt(1)?
> Perhaps without requiring -- on the options, but having the -- is more
> conventional I think.

For the top-level command-line parsing, yeah, I wouldn't think
introducing that crazy macro would be worth it; doing getopt just the
one time is probably worth it.

For the internal calling convention, setting up getopt at the top fo
every single function would be a nightmare.  I have been using the
$arg_parse calling convention in my own script library for several years
now, and believe me, it is a *lot* nicer.

 -George

Re: [PATCH 1/8] Add core.sh and wrapper function

[Ian Campbell]

On Tue, 2015-04-14 at 15:44 +0100, George Dunlap wrote:
> On 04/14/2015 02:55 PM, Ian Campbell wrote:
> > On Mon, 2015-04-13 at 17:13 +0100, Stefano Stabellini wrote:
> >> I think it might be OK to have something like 5) just for the command
> >> line parsing in raise, for the sake of giving more flexibility to the
> >> user. Also it would be confined to one specific call site. But I am
> >> uncertain that the pros outweigh the cons in this case.
> > 
> > I've only glanced at it, but isn't this largely replicating getopt(1)?
> > Perhaps without requiring -- on the options, but having the -- is more
> > conventional I think.
> 
> For the top-level command-line parsing, yeah, I wouldn't think
> introducing that crazy macro would be worth it; doing getopt just the
> one time is probably worth it.
> 
> For the internal calling convention, setting up getopt at the top fo
> every single function would be a nightmare.  I have been using the
> $arg_parse calling convention in my own script library for several years
> now, and believe me, it is a *lot* nicer.

Right, that's why I trimmed all but the bit where Stefano talked about
the raise command line, sorry for not making it clearer why I'd done
that in my comment.

I've no particular opinion on the internal calling convention used
within the shell functions.

Ian.