Rewrite features.sh in Lua (#2045)

* build: target_config_lib: introduce concat_list helper

* build: rewrite features.sh in Lua

The `features` file is converted to a Lua-based DSL.

A helper function `_` is used in the DSL; this will return the original
string for enabled features, and nil for disabled features. This allows
to use boolean operations on features without making the code too
verbose.

Besides having more readable and robust code, this also fixes the bug
that all files `packages/*/features` were evaluated instead of only
using the feature definitions of currently active feeds.

* build: add luacheck support for package/features

.luacheckrc

@@ -12,6 +12,7 @@ include_files = {
 	"**/*.lua",
 	"package/**/luasrc/**/*",
 	"targets/*",
+	"package/features",
 }
 
 exclude_files = {
@@ -104,3 +105,10 @@ files["targets/*"] = {
 		"try_config",
 	},
 }
+
+files["package/features"] = {
+	read_globals = {
+		"_",
+		"feature",
+	},
+}

docs/dev/packages.rst

@@ -71,44 +71,62 @@ Feature flags
 =============
 
 Feature flags provide a convenient way to define package selections without
-making it necessary to list each package explicitly.
+making it necessary to list each package explicitly. The list of features to
+enable for a Gluon build is set by the *GLUON_FEATURES* variable in *site.mk*.
 
 The main feature flag definition file is ``package/features``, but each package
 feed can provide additional definitions in a file called ``features`` at the root
 of the feed repository.
 
-Each flag *$flag* without any explicit definition will simply include the package
+Each flag *$flag* will include the package the name *gluon-$flag* by default.
-with the name *gluon-$flag* by default. The feature definition file can modify
+The feature definition file can modify the package selection by adding or removing
-the package selection in two ways:
+packages when certain combinations of flags are set.
 
-* The *nodefault* function suppresses default of including the *gluon-$flag*
+Feature definitions use Lua syntax. The function *feature* has two arguments:
-  package
+
-* The *packages* function adds a list of packages (or removes, when package
+* A logical expression composed of feature flag names (each prefixed with an underscore before the opening
-  names are prepended with minus signs) when a given logical expression
+  quotation mark), logical operators (*and*, *or*, *not*) and parantheses
-  is satisfied
+* A table with settings that are applied when the logical expression is
+  satisfied:
+
+  * Setting *nodefault* to *true* suppresses the default of including the *gluon-$flag* package.
+    This setting is only applicable when the logical expression is a single,
+    non-negated flag name.
+  * The *packages* field adds or removes packages to install. A package is
+    removed when the package name is prefixed with a ``-`` (after the opening
+    quotation mark).
 
 Example::
 
-    nodefault 'web-wizard'
+    feature(_'web-wizard', {
+      nodefault = true,
+      packages = {
+        'gluon-config-mode-hostname',
+        'gluon-config-mode-geo-location',
+        'gluon-config-mode-contact-info',
+        'gluon-config-mode-outdoor',
+      },
+    })
 
-    packages 'web-wizard' \
+    feature(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
-      'gluon-config-mode-hostname' \
+      packages = {
-      'gluon-config-mode-geo-location' \
+        'gluon-config-mode-mesh-vpn',
-      'gluon-config-mode-contact-info'
+      },
+    })
+
+    feature(_'no-radvd', {
+      nodefault = true,
+      packages = {
+        '-gluon-radvd',
+      },
+    })
 
-    packages 'web-wizard & (mesh-vpn-fastd | mesh-vpn-tunneldigger)' \
-      'gluon-config-mode-mesh-vpn'
 
 This will
 
-* disable the inclusion of a (non-existent) package called *gluon-web-wizard*
+* disable the inclusion of the (non-existent) packages *gluon-web-wizard* and *gluon-no-radvd* when their
-* enable three config mode packages when the *web-wizard* feature is enabled
+  corresponding feature flags appear in *GLUON_FEATURES*
+* enable four additional config mode packages when the *web-wizard* feature is enabled
 * enable *gluon-config-mode-mesh-vpn* when both *web-wizard* and one
   of *mesh-vpn-fastd* and *mesh-vpn-tunneldigger* are enabled
-
+* disable the *gluon-radvd* package when *gluon-no-radvd* is enabled
-Supported syntax elements of logical expressions are:
-
-* \& (and)
-* \| (or)
-* \! (not)
-* parentheses

package/features

@@ -1,37 +1,69 @@
-nodefault 'web-wizard'
+-- GLUON_FEATURES definition file
-
+--
-packages 'web-wizard' \
+-- See the page `dev/packages` (Developer Documentation / Package development)
-	'gluon-config-mode-hostname' \
+-- in the `docs` directory or on gluon.readthedocs.io for information on the
-	'gluon-config-mode-geo-location' \
+-- file format
-	'gluon-config-mode-contact-info' \
-	'gluon-config-mode-outdoor'
-
-packages 'web-wizard & autoupdater' \
-	'gluon-config-mode-autoupdater'
-
-packages 'web-wizard & (mesh-vpn-fastd | mesh-vpn-tunneldigger)' \
-	'gluon-config-mode-mesh-vpn'
 
 
-nodefault 'web-advanced'
+feature(_'web-wizard', {
+	nodefault = true,
+	packages = {
+		'gluon-config-mode-hostname',
+		'gluon-config-mode-geo-location',
+		'gluon-config-mode-contact-info',
+		'gluon-config-mode-outdoor',
+	},
+})
 
-packages 'web-advanced' \
+feature(_'web-wizard' and _'autoupdater', {
-	'gluon-web-admin' \
+	packages = {
-	'gluon-web-network' \
+		'gluon-config-mode-autoupdater',
-	'gluon-web-wifi-config'
+	},
+})
 
-packages 'web-advanced & autoupdater' \
+feature(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
-	'gluon-web-autoupdater'
+	packages = {
+		'gluon-config-mode-mesh-vpn',
+	},
+})
 
-packages 'status-page & mesh-batman-adv-15' \
-	'gluon-status-page-mesh-batman-adv'
 
-packages 'mesh-batman-adv-15' \
+feature(_'web-advanced', {
-	'gluon-ebtables-limit-arp' \
+	nodefault = true,
-	'gluon-radvd'
+	packages = {
+		'gluon-web-admin',
+		'gluon-web-network',
+		'gluon-web-wifi-config',
+	},
+})
 
-packages 'mesh-babel' \
+feature(_'web-advanced' and _'autoupdater', {
-	'gluon-radvd'
+	packages = {
+		'gluon-web-autoupdater',
+	},
+})
 
-packages '!wireless-encryption-wpa3' \
+feature(_'status-page' and _'mesh-batman-adv-15', {
-	'hostapd-mini'
+	packages = {
+		'gluon-status-page-mesh-batman-adv',
+	},
+})
+
+feature(_'mesh-batman-adv-15', {
+	packages = {
+		'gluon-ebtables-limit-arp',
+		'gluon-radvd',
+	},
+})
+
+feature(_'mesh-babel', {
+	packages = {
+		'gluon-radvd',
+	},
+})
+
+feature(not _'wireless-encryption-wpa3', {
+	packages = {
+		'hostapd-mini',
+	},
+})

scripts/feature_lib.lua

@@ -0,0 +1,60 @@
+local M = {}
+
+local function to_keys(t)
+	local ret = {}
+	for _, v in ipairs(t) do
+		ret[v] = true
+	end
+	return ret
+end
+
+local function collect_keys(t)
+	local ret = {}
+	for v in pairs(t) do
+		table.insert(ret, v)
+	end
+	return ret
+end
+
+function M.get_packages(file, features)
+	local feature_table = to_keys(features)
+
+	local funcs = {}
+
+	function funcs._(feature)
+		if feature_table[feature] then
+			return feature
+		end
+	end
+
+	local nodefault = {}
+	local packages = {}
+	function funcs.feature(match, options)
+		if not match then
+			return
+		end
+
+		if options.nodefault then
+			nodefault[match] = true
+		end
+		for _, package in ipairs(options.packages or {}) do
+			packages[package] = true
+		end
+	end
+
+	-- Evaluate the feature definition file
+	local f = loadfile(file)
+	setfenv(f, funcs)
+	f()
+
+	-- Handle default packages
+	for _, feature in ipairs(features) do
+		if not nodefault[feature] then
+			packages['gluon-' .. feature] = true
+		end
+	end
+
+	return collect_keys(packages)
+end
+
+return M

scripts/features.sh

@@ -1,77 +0,0 @@
-#!/bin/bash --norc
-
-set -e
-shopt -s nullglob
-
-
-nodefault() {
-	# We define a function instead of a variable, as variables could
-	# be predefined in the environment (in theory)
-	eval "gluon_feature_nodefault_$1() {
-		:
-	}"
-}
-
-packages() {
-	:
-}
-
-for f in package/features packages/*/features; do
-	. "$f"
-done
-
-
-# Shell variables can't contain minus signs, so we escape them
-# using underscores (and also escape underscores to avoid mapping
-# multiple inputs to the same output)
-sanitize() {
-	local v="$1"
-	v="${v//_/_1}"
-	v="${v//-/_2}"
-	echo -n "$v"
-}
-
-vars=()
-
-for feature in $1; do
-	if [ "$(type -t "gluon_feature_nodefault_${feature}")" != 'function' ]; then
-		echo "gluon-${feature}"
-	fi
-
-	vars+=("$(sanitize "$feature")=1")
-done
-
-
-nodefault() {
-	:
-}
-
-# shellcheck disable=SC2086
-packages() {
-	local cond="$(sanitize "$1")"
-	shift
-
-	# We only allow variable names, parentheses and the operators: & | !
-	if grep -q '[^A-Za-z0-9_()&|! ]' <<< "$cond"; then
-		exit 1
-	fi
-
-	# Let will return false when the result of the passed expression is 0,
-	# so we always add 1. This way false is only returned for syntax errors.
-	local ret="$(env -i "${vars[@]}" bash --norc -ec "let _result_='1+($cond)'; echo -n \"\$_result_\"" 2>/dev/null)"
-	case "$ret" in
-	2)
-		for pkg in "$@"; do
-			echo "$pkg"
-		done
-		;;
-	1)
-		;;
-	*)
-		exit 1
-	esac
-}
-
-for f in package/features packages/*/features; do
-	. "$f"
-done

scripts/target_config_lib.lua

@@ -1,4 +1,5 @@
 local lib = dofile('scripts/target_lib.lua')
+local feature_lib = dofile('scripts/feature_lib.lua')
 local env = lib.env
 
 local target = env.GLUON_TARGET
@@ -24,6 +25,8 @@ local function split(s)
 	return ret
 end
 
+local feeds = split(lib.exec_capture_raw('. scripts/modules.sh; echo "$FEEDS"'))
+
 -- Strip leading '-' character
 local function strip_neg(s)
 	if string.sub(s, 1, 1) == '-' then
@@ -49,14 +52,26 @@ local function append_to_list(list, item, keep_neg)
 	return ret
 end
 
-local function compact_list(list, keep_neg)
+local function concat_list(a, b, keep_neg)
-	local ret = {}
+	local ret = a
-	for _, el in ipairs(list) do
+	for _, el in ipairs(b) do
 		ret  = append_to_list(ret, el, keep_neg)
 	end
 	return ret
 end
 
+local function compact_list(list, keep_neg)
+	return concat_list({}, list, keep_neg)
+end
+
+local function file_exists(file)
+	local f = io.open(file)
+	if not f then
+		return false
+	end
+	f:close()
+	return true
+end
 
 local function site_vars(var)
 	return lib.exec_capture_raw(string.format(
@@ -75,17 +90,25 @@ local function site_packages(image)
 	return split(site_vars(string.format('$(GLUON_%s_SITE_PACKAGES)', image)))
 end
 
--- TODO: Rewrite features.sh in Lua
 local function feature_packages(features)
-	-- Ugly hack: Lua doesn't give us the return code of a popened
+	local pkgs = {}
-	-- command, so we match on a special __ERROR__ marker
+	local function handle_feature_file(file)
-	local pkgs = lib.exec_capture({'scripts/features.sh', features}, '|| echo __ERROR__')
+		pkgs = concat_list(pkgs, feature_lib.get_packages(file, features))
-	assert(string.find(pkgs, '__ERROR__') == nil, 'Error while evaluating features')
+	end
+
+	handle_feature_file('package/features')
+
+	for _, feed in ipairs(feeds) do
+		local path = string.format('packages/%s/features', feed)
+		if file_exists(path) then
+			handle_feature_file(path)
+		end
+	end
+
 	return pkgs
 end
 
--- This involves running lots of processes to evaluate site.mk, so we
+-- This involves running a few processes to evaluate site.mk, so we add a simple cache
--- add a simple cache
 local class_cache = {}
 local function class_packages(class)
 	if class_cache[class] then
@@ -93,12 +116,10 @@ local function class_packages(class)
 	end
 
 	local features = site_vars(string.format('$(GLUON_FEATURES) $(GLUON_FEATURES_%s)', class))
-	features = table.concat(compact_list(split(features), false), ' ')
+	features = compact_list(split(features), false)
 
 	local pkgs = feature_packages(features)
-	pkgs = pkgs .. ' ' .. site_vars(string.format('$(GLUON_SITE_PACKAGES) $(GLUON_SITE_PACKAGES_%s)', class))
+	pkgs = concat_list(pkgs, split(site_vars(string.format('$(GLUON_SITE_PACKAGES) $(GLUON_SITE_PACKAGES_%s)', class))))
-
-	pkgs = compact_list(split(pkgs))
 
 	class_cache[class] = pkgs
 	return pkgs
@@ -178,9 +199,7 @@ else
 	-- x86 fallback: no devices
 	local target_pkgs = {}
 	local function handle_pkgs(pkgs)
-		for _, pkg in ipairs(pkgs) do
+		target_pkgs = concat_list(target_pkgs, pkgs)
-			target_pkgs = append_to_list(target_pkgs, pkg)
-		end
 	end
 
 	-- Just hardcode the class for device-less targets to 'standard'