Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: implement flexible site-selection system #3054

Merged
merged 15 commits into from
Dec 26, 2023

Conversation

blocktrron
Copy link
Member

This is currently lacking documentation.
Opening as a draft to obtain feedback whether or not this is moving in the right direction.


Implement a flexible system for handling site-defined features as well as packages.

This system is inspired by the existing feature-system and allows for a more flexible approach for selecting specific packages for devices.

Features are now defined in a features file in the site-root. The same goes for packages.

These files are sequentially evaluated and the device-package list is evaluated for each device independently.

@github-actions github-actions bot added 3. topic: docs Topic: Documentation 3. topic: build This is about the build system labels Nov 5, 2023
@blocktrron blocktrron force-pushed the flexible-feature-packages branch from 0fe6fd4 to 92ac0bb Compare November 6, 2023 01:27
@blocktrron blocktrron marked this pull request as ready for review November 11, 2023 13:38
@neocturne
Copy link
Member

I like the idea of using a Lua DSL here a lot, but I think it would be nicer if features and packages could be configured in the same file, as they are often related. That would also allow to set related conditional features and packages under a single when.

@blocktrron
Copy link
Member Author

Great idea! I'll see how this can be done.

@blocktrron blocktrron force-pushed the flexible-feature-packages branch from 20992d1 to 87721cc Compare November 25, 2023 20:30
@blocktrron
Copy link
Member Author

@neocturne I've adressed your request, does this look better?

@blocktrron
Copy link
Member Author

Added a first draft of documentation. Example and CI sites still need to be reworked.

@blocktrron blocktrron force-pushed the flexible-feature-packages branch from dd90095 to ee11ddf Compare November 29, 2023 03:35
@blocktrron
Copy link
Member Author

@neocturne Please see the updated PR, is this more to your liking?

Also yes, negation is still supported, as this is done in handle_pkgs which is not modified.

@blocktrron blocktrron force-pushed the flexible-feature-packages branch from ee11ddf to a434925 Compare December 9, 2023 20:02
@blocktrron blocktrron force-pushed the flexible-feature-packages branch 2 times, most recently from 3b855c8 to 7935eee Compare December 9, 2023 23:49
@neocturne
Copy link
Member

Looking quite nice now!

Two remarks (obviously bikeshedding - I'm fine with either solution, just stating my preference):

  • Is there a reason you kept the () in addition to the {} for the Packages and Features arguments in the example?
  • The capitalization of Packages and Features looks a bit weird IMO

@blocktrron blocktrron force-pushed the flexible-feature-packages branch from 1799467 to 1e16177 Compare December 10, 2023 01:33
@blocktrron
Copy link
Member Author

@neocturne

No special reason for keeping the (). I can remove them at wish

For the capitalization - My intention was to keep modifying functions with a leading Uppercase while function for the logic evaluation have lowercase first letters.

@blocktrron blocktrron force-pushed the flexible-feature-packages branch 2 times, most recently from a5ae531 to 0ca701f Compare December 11, 2023 04:54
scripts/target_lib.lua Outdated Show resolved Hide resolved
@neocturne
Copy link
Member

@neocturne

No special reason for keeping the (). I can remove them at wish

For the capitalization - My intention was to keep modifying functions with a leading Uppercase while function for the logic evaluation have lowercase first letters.

Using lowercase for everything would be consistent with our target definitions, so if we decide to go with the uppercase names, I would propose to change those as well. Using a consistent function name case still feels more common as a coding style, but I accept that it may make sense to apply different rules to such DSLs.

@blocktrron
Copy link
Member Author

@neocturne I'll opt with all lowercase and maybe revising the docs. Anything from the implementation side i should revise?

scripts/image_customization_lib.lua Outdated Show resolved Hide resolved
scripts/image_customization_lib.lua Outdated Show resolved Hide resolved
scripts/image_customization_lib.lua Outdated Show resolved Hide resolved
scripts/image_customization_lib.lua Outdated Show resolved Hide resolved
scripts/target_config_lib.lua Outdated Show resolved Hide resolved
@neocturne
Copy link
Member

One more idea: How about allowing to specify negated features and packages as -'name' instead of '-name'? I kinda like the separation of the operator from the name.

A very simple implementation might look like this (using the - in the strings would be an implementation detail that we could change in the future):

getmetatable('').__unm = function(s)
	return '-' .. s
end

@blocktrron blocktrron force-pushed the flexible-feature-packages branch 2 times, most recently from 291fc8d to 06516b0 Compare December 13, 2023 02:57
Now we use the new Image-customization framework in Gluon, we need to
also update the docs so examples and descriptions are acurate again.

Signed-off-by: David Bauer <mail@david-bauer.net>
Print a warning in case users attempt to build Gluon using a site.mk
which containes deprecated GLUON_FEATURES or GLUON_SITE_PACKAGES.

Signed-off-by: David Bauer <mail@david-bauer.net>
If we align the table keys of the selection table to match the return
object of get_selections, we can omit creating a new return object.

Signed-off-by: David Bauer <mail@david-bauer.net>
@blocktrron blocktrron force-pushed the flexible-feature-packages branch from 875ff0b to c3474a8 Compare December 19, 2023 17:23
Include the file-extension with the image-customization.lua file. This
will ease work with editors providing syntax highlighting, as they now
properly detect the file-type.

Signed-off-by: David Bauer <mail@david-bauer.net>
Signed-off-by: David Bauer <mail@david-bauer.net>
@blocktrron blocktrron force-pushed the flexible-feature-packages branch 2 times, most recently from 7ceeab7 to 3bf54a7 Compare December 19, 2023 17:37
@blocktrron
Copy link
Member Author

Updated the PR. I have not build-tested the current state yet.

@blocktrron blocktrron force-pushed the flexible-feature-packages branch 4 times, most recently from 461b822 to d9bb9bc Compare December 20, 2023 12:04
Create an init routine which loads the environment only once instead
every evaluation.

Signed-off-by: David Bauer <mail@david-bauer.net>
@blocktrron blocktrron force-pushed the flexible-feature-packages branch 2 times, most recently from e5e61a4 to d65573d Compare December 20, 2023 12:13
Load the site image-customization.lua file only on init. Previously, the
file was loaded on every device invocation.

Signed-off-by: David Bauer <mail@david-bauer.net>
@blocktrron blocktrron force-pushed the flexible-feature-packages branch from d65573d to dd5c503 Compare December 20, 2023 12:17
@maurerle
Copy link
Member

It would be quite handy to be able to call:
is_cellular_device and is_outdoor_device from platform.lua in the image-customization.lua

@blocktrron
Copy link
Member Author

@maurerle I would like to hold this back for further improvement. I see this as possible, but we would ahve to build this meta-information from the same source. Possibly the same way as to handling flash-sizes as well as RAM.

@maurerle
Copy link
Member

Sure, I am fine with that.
For now I helped myself here - but I don't like the idea of maintaining that list myself:
https://github.com/ffac/site/blob/eb6f98e8ec05957163c3d24115d337cb21ecb97e/image-customization.lua#L25-L36

I just build and tested Firmware from this PR - looks good. I am happy with merging it the way it is.

Signed-off-by: David Bauer <mail@david-bauer.net>
@blocktrron blocktrron merged commit db0e7bd into freifunk-gluon:master Dec 26, 2023
40 checks passed
@blocktrron blocktrron deleted the flexible-feature-packages branch December 26, 2023 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants