Merge pull request #2111 from freifunk-gluon/features-fix

Fix two feature handling bugs

.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
-  quotation mark), logical operators (*and*, *or*, *not*) and parentheses
-* A table with settings that are applied when the logical expression is
-  satisfied:
+* *feature(name, pkgs)*: Defines a new feature. *feature()* expects a feature
+  (flag) name and a list of packages to add or remove when the feature is
+  enabled.
 
-  * 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).
+  * Defining a feature using *feature* replaces the default definition of
+    just including *gluon-$flag*.
+  * A package is 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', {
-      nodefault = true,
-      packages = {
-        'gluon-config-mode-hostname',
-        'gluon-config-mode-geo-location',
-        'gluon-config-mode-contact-info',
-        'gluon-config-mode-outdoor',
-      },
+    feature('web-wizard', {
+      'gluon-config-mode-hostname',
+      'gluon-config-mode-geo-location',
+      'gluon-config-mode-contact-info',
+      'gluon-config-mode-outdoor',
     })
 
-    feature(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
-      packages = {
-        'gluon-config-mode-mesh-vpn',
-      },
+    when(_'web-wizard' and (_'mesh-vpn-fastd' or _'mesh-vpn-tunneldigger'), {
+      'gluon-config-mode-mesh-vpn',
     })
 
-    feature(_'no-radvd', {
-      nodefault = true,
-      packages = {
-        '-gluon-radvd',
-      },
+    feature('no-radvd', {
+      '-gluon-radvd',
     })
 
 

package/features

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

scripts/feature_lib.lua

@@ -16,43 +16,58 @@ local function collect_keys(t)
 	return ret
 end
 
-function M.get_packages(file, features)
-	local feature_table = to_keys(features)
+function M.get_packages(files, features)
+	local enabled_features = to_keys(features)
+	local handled_features = {}
+	local packages = {}
 
 	local funcs = {}
 
+	local function add_pkgs(pkgs)
+		for _, pkg in ipairs(pkgs or {}) do
+			packages[pkg] = true
+		end
+	end
+
 	function funcs._(feature)
-		if feature_table[feature] then
-			return feature
+		return enabled_features[feature] ~= nil
+	end
+
+	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
+
+	end
+
+	function funcs.when(cond, pkgs)
+		assert(
+			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
 
-	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
+	-- Evaluate the feature definition files
+	for _, file in ipairs(files) do
+		local f, err = loadfile(file)
+		if not f then
+			error('Failed to parse feature definition: ' .. err)
 		end
+		setfenv(f, funcs)
+		f()
 	end
 
-	-- Evaluate the feature definition file
-	local f, err = loadfile(file)
-	if not f then
-		error('Failed to parse feature definition: ' .. err)
-	end
-	setfenv(f, funcs)
-	f()
-
 	-- 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

scripts/target_config_lib.lua

@@ -91,21 +91,15 @@ local function site_packages(image)
 end
 
 local function feature_packages(features)
-	local pkgs = {}
-	local function handle_feature_file(file)
-		pkgs = concat_list(pkgs, feature_lib.get_packages(file, features))
-	end
-
-	handle_feature_file('package/features')
-
+	local files = {'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)
+			table.insert(files, path)
 		end
 	end
 
-	return pkgs
+	return feature_lib.get_packages(files, features)
 end
 
 -- This involves running a few processes to evaluate site.mk, so we add a simple cache