volumio / Build

Buildscripts for Volumio System
GNU General Public License v2.0
113 stars 102 forks source link

Device check #435

Closed xipmix closed 3 years ago

gkkpch commented 3 years ago

This fix is appears to be problematic when one only builds a rootfs (e.g. using "-b armv7"), in this case $DEVICE has not been set yet.

ashthespy commented 3 years ago

IIRC that was the reason for it being down there.. Else we need to check if -b was passed in this exit..

xipmix commented 3 years ago

Doh! I'll take a look at how to improve the patch.

xipmix commented 3 years ago

Hm I tried to build with -b armv8 and got loud complaints about no device being selected.

# ./build.sh -b armv8  -v buster

[ .... ] Running Volumio Image Builder -  
[ error ] No configuration found for <> 
[ .. ] Build system currently supports 10 devices: [ motivo mp1 nanopineo2 odroidn2 orangepilite raspberry rockpis tinkerboard x86_amd64 x86_i386 ] 

@gkkpch Can you please post your build attempt and the log showing the problem?

xipmix commented 3 years ago

It may be enough to wrap that check in a check for $DEVICE being set

+if [[ -n "$DEVICE" ]]; then
   DEV_CONFIG="${SRC}/recipes/devices/${DEVICE}.sh"
   if [[ ! -f $DEV_CONFIG ]]; then
     log "No configuration found for <${DEVICE}>" "err"
     log "Build system currently supports ${#DEVICE_LIST[@]} devices:" "${DEVICE_LIST[*]}"
     exit 1
   fi
+fi
ashthespy commented 3 years ago

Nope, when you do a -b arm to prebuild a rootfs, device is not set.. you need to check both :-)

volumio commented 3 years ago

@xipmix @gkkpch @ashthespy

What about reverting it? And then adding later on when tested? I agree that while working on buster, let's wait to merge before carefully tested ;)

xipmix commented 3 years ago

sure, revert & I'll respin

ashthespy commented 3 years ago

@xipmix The idea behind this was to avoid a base -b <> build if the supplied device -d <> is not supported correct?

xipmix commented 3 years ago

Not really - my idea was to stop the build entirely because nonsensical arguments were supplied. I now understand that if it builds an image and then falls over due to an invalid device I can retry with -d and that just-built image, bit it seemed better to just abort early.

Ge has taken another swing at this but it's not quite right, see 7fc0cb0a34a594c32e5c633f756b6da934de4b0c

gkkpch commented 3 years ago

I think @xipmix idea is ok, especially considering the build server. There we always (re)build the rootfs and device in one go. So a typo means 15-20 minutes lost. I'll correct my too hasty fix and re-test.

ashthespy commented 3 years ago

Something like this -- I haven't had coffee yet though... ;-) EDIT: This might be a bit better..

diff --git a/build.sh b/build.sh
index 7fd2b9b..b33d0ba 100755
--- a/build.sh
+++ b/build.sh
@@ -217,6 +217,23 @@ function patch_multistrap_conf() {
   esac
 }

+function check_supported_device() {
+
+  if [[ -n ${DEVICE} ]]; then
+    DEV_CONFIG="${SRC}/recipes/devices/${DEVICE}.sh"
+    if [[ ! -f $DEV_CONFIG ]]; then
+      log "No configuration found for <${DEVICE}>" "err"
+      log "Build system currently supports ${#DEVICE_LIST[@]} devices:" "${DEVICE_LIST[*]}"
+      exit 1
+    fi
+  elif [[ -n ${BUILD} ]]; then
+    log "No device flag passed to builder, building only ${BASE}" "wrn"
+  else
+    log "No Base or Device flag found.." "wrn"
+    HELP
+  fi
+}
+
 #Check the number of arguments. If none are passed, print help and exit.
 NUMARGS=$#
 if [ "$NUMARGS" -eq 0 ]; then
@@ -249,19 +266,11 @@ while getopts b:v:d:p:t:h: FLAG; do
     ;;
   esac
 done
-
-if [ ${DEVICE:=none} != none ]; then
-  DEV_CONFIG="${SRC}/recipes/devices/${DEVICE}.sh"
-  if [[ ! -f $DEV_CONFIG ]]; then
-    log "No configuration found for <${DEVICE}>" "err"
-    log "Build system currently supports ${#DEVICE_LIST[@]} devices:" "${DEVICE_LIST[*]}"
-    exit 1
-  fi
-fi
-
 # move past our parsed args
 shift $((OPTIND - 1))

+check_supported_device
+
 log "Checking whether we are running as root"
 if [ "$(id -u)" -ne 0 ]; then
   log "Please run the build script as root" "err"