From 13b743d51e2a7b35671e108f1f1c0138b3675fbd Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Fri, 28 Aug 2020 22:04:06 +0200 Subject: [PATCH 1/2] 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: ee5ec5afe5ea ("build: rewrite features.sh in Lua") --- .luacheckrc | 1 + docs/dev/packages.rst | 56 ++++++++++++++++---------------- package/features | 71 ++++++++++++++++------------------------- scripts/feature_lib.lua | 43 ++++++++++++++++--------- 4 files changed, 84 insertions(+), 87 deletions(-) diff --git a/.luacheckrc b/.luacheckrc index f532e1bf..b308748c 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -110,5 +110,6 @@ files["package/features"] = { read_globals = { "_", "feature", + "when", }, } diff --git a/docs/dev/packages.rst b/docs/dev/packages.rst index af047e21..b6032b2e 100644 --- a/docs/dev/packages.rst +++ b/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', }) diff --git a/package/features b/package/features index 665b1bd6..72887e3a 100644 --- a/package/features +++ b/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', }) diff --git a/scripts/feature_lib.lua b/scripts/feature_lib.lua index d4cfafe0..4c575dd4 100644 --- a/scripts/feature_lib.lua +++ b/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) - if feature_table[feature] then - return feature + local function add_pkgs(pkgs) + for _, pkg in ipairs(pkgs or {}) do + packages[pkg] = true end end - local nodefault = {} - local packages = {} - function funcs.feature(match, options) - if not match then - return + function funcs._(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 - if options.nodefault then - nodefault[match] = true - end - for _, package in ipairs(options.packages or {}) do - packages[package] = true + 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 @@ -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 From a9c2db939aac2c6123a6f3088fa2aaef42e80696 Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Fri, 28 Aug 2020 22:20:13 +0200 Subject: [PATCH 2/2] features: handle all feature files in a single pass of feature_lib.get_packages() All defined features need to be known at the same time, otherwise handling a feed-provided feature definition file would add gluon-web-advanced etc. to the package list when the corresponding feature flags appear in GLUON_FEATURES. Fixes: ee5ec5afe5ea ("build: rewrite features.sh in Lua") --- scripts/feature_lib.lua | 16 +++++++++------- scripts/target_config_lib.lua | 12 +++--------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/scripts/feature_lib.lua b/scripts/feature_lib.lua index 4c575dd4..a5197e0a 100644 --- a/scripts/feature_lib.lua +++ b/scripts/feature_lib.lua @@ -16,7 +16,7 @@ local function collect_keys(t) return ret end -function M.get_packages(file, features) +function M.get_packages(files, features) local enabled_features = to_keys(features) local handled_features = {} local packages = {} @@ -55,13 +55,15 @@ function M.get_packages(file, features) end end - -- Evaluate the feature definition file - local f, err = loadfile(file) - if not f then - error('Failed to parse feature definition: ' .. err) + -- 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 - setfenv(f, funcs) - f() -- Handle default packages for _, feature in ipairs(features) do diff --git a/scripts/target_config_lib.lua b/scripts/target_config_lib.lua index e91dfb0c..4b107e2b 100644 --- a/scripts/target_config_lib.lua +++ b/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