features: fix handling of logical expressions

The rewrite of the feature handling introduced multiple major bugs. One
of them was caused by the way Lua's logical operators work:

An expression of the form

    _'autoupdater' and _'web-advanced'

would return 'web-advanced' rather than the boolean true when _ returned
both strings unchanged (because the features are enabled).

As entries with more than a single feature name in their expressions did
not set no_default, Gluon would then attempt to add gluon-web-advanced to
the package selection, as web-advanced is a "pure" feature.

To fix this, and get rid of the annoying nodefault, separate handling of
"pure" feature and handling of logical expressions into two separate
functions, called feature() and when(). To simplify the feature
definitions, the package list is now passed directly to these functions
rather than in a table with a single field 'packages'.

Fixes: ee5ec5afe5 ("build: rewrite features.sh in Lua")

.luacheckrc

@@ -110,5 +110,6 @@ files["package/features"] = {
 	read_globals = {
 		"_",
 		"feature",
+		"when",
 	},
 }

docs/dev/packages.rst

@@ -82,43 +82,43 @@ Each flag *$flag* will include the package the name *gluon-$flag* by default.
 The feature definition file can modify the package selection by adding or removing
 packages when certain combinations of flags are set.
 
-Feature definitions use Lua syntax. The function *feature* has two arguments:
+Feature definitions use Lua syntax. Two basic functions are defined:
 
-* A logical expression composed of feature flag names (each prefixed with an underscore before the opening
+* *feature(name, pkgs)*: Defines a new feature. *feature()* expects a feature
-  quotation mark), logical operators (*and*, *or*, *not*) and parentheses
+  (flag) name and a list of packages to add or remove when the feature is
-* A table with settings that are applied when the logical expression is
+  enabled.
-  satisfied:
 
-  * Setting *nodefault* to *true* suppresses the default of including the *gluon-$flag* package.
+  * Defining a feature using *feature* replaces the default definition of
-    This setting is only applicable when the logical expression is a single,
+    just including *gluon-$flag*.
-    non-negated flag name.
+  * A package is removed when the package name is prefixed with a ``-`` (after
-  * The *packages* field adds or removes packages to install. A package is
+    the opening quotation mark).
-    removed when the package name is prefixed with a ``-`` (after the opening
+
-    quotation mark).
+* *when(expr, pkgs)*: Adds or removes packages when a given logical expression
+  of feature flags is satisfied.
+
+  * *expr* is a logical expression composed of feature flag names (each prefixed
+    with an underscore before the opening quotation mark), logical operators
+    (*and*, *or*, *not*) and parentheses.
+  * Referencing a feature flag in *expr* has no effect on the default handling
+    of the flag. When no *feature()* entry for a flag exists, it will still
+    add *gluon-$flag* by default.
+  * *pkgs* is handled as for *feature()*.
 
 Example::
 
-    feature(_'web-wizard', {
+    feature('web-wizard', {
-      nodefault = true,
+      'gluon-config-mode-hostname',
-      packages = {
+      'gluon-config-mode-geo-location',
-        'gluon-config-mode-hostname',
+      'gluon-config-mode-contact-info',
-        'gluon-config-mode-geo-location',
+      'gluon-config-mode-outdoor',
-        'gluon-config-mode-contact-info',
-        'gluon-config-mode-outdoor',
-      },
     })
 
-    feature(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
+    when(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
-      packages = {
+      'gluon-config-mode-mesh-vpn',
-        'gluon-config-mode-mesh-vpn',
-      },
     })
 
-    feature(_'no-radvd', {
+    feature('no-radvd', {
-      nodefault = true,
+      '-gluon-radvd',
-      packages = {
-        '-gluon-radvd',
-      },
     })
 
 

package/features

@@ -5,65 +5,48 @@
 -- file format
 
 
-feature(_'web-wizard', {
+feature('web-wizard', {
-	nodefault = true,
+	'gluon-config-mode-hostname',
-	packages = {
+	'gluon-config-mode-geo-location',
-		'gluon-config-mode-hostname',
+	'gluon-config-mode-contact-info',
-		'gluon-config-mode-geo-location',
+	'gluon-config-mode-outdoor',
-		'gluon-config-mode-contact-info',
-		'gluon-config-mode-outdoor',
-	},
 })
 
-feature(_'web-wizard' and _'autoupdater', {
+when(_'web-wizard' and _'autoupdater', {
-	packages = {
+	'gluon-config-mode-autoupdater',
-		'gluon-config-mode-autoupdater',
-	},
 })
 
-feature(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
+when(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
-	packages = {
+	'gluon-config-mode-mesh-vpn',
-		'gluon-config-mode-mesh-vpn',
-	},
 })
 
 
-feature(_'web-advanced', {
+feature('web-advanced', {
-	nodefault = true,
+	'gluon-web-admin',
-	packages = {
+	'gluon-web-network',
-		'gluon-web-admin',
+	'gluon-web-wifi-config',
-		'gluon-web-network',
-		'gluon-web-wifi-config',
-	},
 })
 
-feature(_'web-advanced' and _'autoupdater', {
+when(_'web-advanced' and _'autoupdater', {
-	packages = {
+	'gluon-web-autoupdater',
-		'gluon-web-autoupdater',
-	},
 })
 
-feature(_'status-page' and _'mesh-batman-adv-15', {
+
-	packages = {
+when(_'mesh-batman-adv-15', {
-		'gluon-status-page-mesh-batman-adv',
+	'gluon-ebtables-limit-arp',
-	},
+	'gluon-radvd',
 })
 
-feature(_'mesh-batman-adv-15', {
+when(_'status-page' and _'mesh-batman-adv-15', {
-	packages = {
+	'gluon-status-page-mesh-batman-adv',
-		'gluon-ebtables-limit-arp',
-		'gluon-radvd',
-	},
 })
 
-feature(_'mesh-babel', {
+
-	packages = {
+when(_'mesh-babel', {
-		'gluon-radvd',
+	'gluon-radvd',
-	},
 })
 
-feature(not _'wireless-encryption-wpa3', {
+
-	packages = {
+when(not _'wireless-encryption-wpa3', {
-		'hostapd-mini',
+	'hostapd-mini',
-	},
 })

scripts/feature_lib.lua

@@ -17,28 +17,41 @@ local function collect_keys(t)
 end
 
 function M.get_packages(file, features)
-	local feature_table = to_keys(features)
+	local enabled_features = to_keys(features)
+	local handled_features = {}
+	local packages = {}
 
 	local funcs = {}
 
-	function funcs._(feature)
+	local function add_pkgs(pkgs)
-		if feature_table[feature] then
+		for _, pkg in ipairs(pkgs or {}) do
-			return feature
+			packages[pkg] = true
 		end
 	end
 
-	local nodefault = {}
+	function funcs._(feature)
-	local packages = {}
+		return enabled_features[feature] ~= nil
-	function funcs.feature(match, options)
+	end
-		if not match then
+
-			return
+	function funcs.feature(feature, pkgs)
+		assert(
+			type(feature) == 'string',
+			'Incorrect use of feature(): pass a feature name without _ as first argument')
+
+		if enabled_features[feature] then
+			handled_features[feature] = true
+			add_pkgs(pkgs)
 		end
 
-		if options.nodefault then
+	end
-			nodefault[match] = true
+
-		end
+	function funcs.when(cond, pkgs)
-		for _, package in ipairs(options.packages or {}) do
+		assert(
-			packages[package] = true
+			type(cond) == 'boolean',
+			'Incorrect use of when(): pass a locical expression of _-prefixed strings as first argument')
+
+		if cond then
+			add_pkgs(pkgs)
 		end
 	end
 
@@ -52,7 +65,7 @@ function M.get_packages(file, features)
 
 	-- Handle default packages
 	for _, feature in ipairs(features) do
-		if not nodefault[feature] then
+		if not handled_features[feature] then
 			packages['gluon-' .. feature] = true
 		end
 	end