Rework configuration parsing (again) #2847

DaanDeMeyer · 2024-07-07T15:22:55Z

As explained in #2846, there are multiple issues with the current
implementation of mkosi.images. Let's take what we learned from the
default initrd and the default tools tree and apply it mkosi.images.

Specifically, all issues arise from the fact that we apply every option
from the global configuration (including CLI arguments) to the images
from mkosi.images/. To avoid the issues that arise from this (e.g --package
abc installing abc in all images), we made configuration values override
CLI arguments again so that we could override faulty CLI arguments again
in subimages so that they would only apply to the main image (e.g. set
Format= explicitly for each subimage so that --format on the command line
only applies to the main image).

Because we still wanted to allow configurable settings that can be modified
via the command line, we introduced the default specifier '@' which can be
prefixed to a setting to set a default value instead of overriding the value.
The '@' specifier is generally used in the global image independent
configuration to specify default values that can be overridden from the command
line. This specifier has led to a lot of confusion, along with the behavior that
the CLI does not override the configuration.

From the default tools tree and default initrd, we learned that what works
very well is to only have specific settings from the main image configuration
apply to the default tools tree and default initrd. For example, the distribution,
release, mirror and architecture should be the same for the main image and the
initrd, but the packages from the main image should not all be installed in the
initrd.

We can apply this idea to the images from mkosi.images/ as well, if we
introduce the assumption that all images defined in mkosi.images are
subimages intended to be included in some way or form in the main image.
This assumption allows us to divide all settings into either image specific
settings or "universal" settings that should apply to the main image and all
its subimages. The universal settings are passed on to each subimage. The image
specific settings are not.

This idea also allows us to define the "main" image outside of mkosi.images
again. Since only "universal" settings are passed on, we can safely define
an output format and such again in the global configuration, as we know this
won't be passed on to subimages.

It also allows us to make CLI arguments override configuration again. Since
there is no need anymore for subimages to override the CLI configuration as
inappropriate CLI configuration such as extra packages will only apply to the
main image and not any subimages from mkosi.images/. Because CLI configuration
overrides file configuration again, we also don't need the '@' specifier anymore,
as default values can simply be set without '@', since the CLI will override
the configuration file values by default.

We also lose the need for --append, because the sole use for --append was again
to override file based configuration.

This commit implements all of what's mentioned above, specifically:

CLI configuration now always trumps file based configuration.
The '@' specifier is dropped automatically during parsing
The main image is now always added from global configuration, even
if there are images in mkosi.images/. The main image is always built
last, and cannot be used as a dependency in the Dependencies= setting
for images defined in mkosi.images/.
The Dependencies= setting for the main image now is used to specify
which subimages from mkosi.images/ to build. By default all subimages
are built.
A universal tag is introduced for settings and appropriate settings
are marked as universal. Universal settings are passed on from the
main image configuration to subimage configuration.
The Images= setting is removed, as it's role is replaced by
Dependencies=.
The old name mkosi.presets and the Preset section name are removed
as they have been deprecated for a long time now.
The config parsing tests are extended to cover more cases.
All builtin configuration is adapted to stop using the '@' specifier.
The documentation is updated in accordance with the changes.

Fits what the function does better.

We don't need to keep track of the current amount of includes since those includes are already tracked in parsed_includes and will be ignored. Slightly less efficient but this shouldn't matter here. We also store the inode in parsed_includes before we parse the config to make sure we don't try to parse it more than once.

Doesn't change behavior, just slightly easier to read.

DaanDeMeyer · 2024-07-07T20:55:01Z

This breaks backwards compat in the following ways:

@ is removed, fix is to just remove the @. We can do this in mkosi itself and log a warning
Images is removed, which is only applicable when building multiple images with mkosi.images/, which is rare
Main image is now always included even when mkosi.images is used. Fix is to move the main image configuration from mkosi.images to the global configuration.
--append is removed, which is only used by the systemd integration tests I think
mkosi.images can only be used to build subimages now, with more restrictions put on them (same distribution, release, mirror, ...). Not all CLI settings apply to mkosi.images/ anymore (which is a good thing).

Anyone building single images without using mkosi.images/ will not be affected if we automatically remove @ from settings within mkosi. This makes me think that breaking compat here is worth it given the substantial simplification of config parsing we get from it.

NekkoDroid · 2024-07-07T21:45:03Z

Just went to test it out (adjusting my config for this) and noticed that %i was being ignored in subimages. ImageId and ImageVersion probably should also have the universal tag applied.

Is there any reason to keep the symlinks for subimages around now that they aren't used for qemu/shell/...? They can't really be used as base trees for anything since the mounting depends on the extension.
Is there some way to apply a global RemoveFiles= list to the image & subimages? I was using it globally to clean up directories & files that are shipped as part of Packages= and are needed for builds to work, but aren't needed any output image.

DaanDeMeyer · 2024-07-08T07:30:10Z

Just went to test it out (adjusting my config for this) and noticed that %i was being ignored in subimages. ImageId and ImageVersion probably should also have the universal tag applied.

Added both as universal settings.

Is there any reason to keep the symlinks for subimages around now that they aren't used for qemu/shell/...? They can't really be used as base trees for anything since the mounting depends on the extension.

I've fixed that now by making sure we resolve symlinks in mount_base_trees()

Is there some way to apply a global RemoveFiles= list to the image & subimages? I was using it globally to clean up directories & files that are shipped as part of Packages= and are needed for builds to work, but aren't needed any output image.

Have a shared snippet with the RemoveFiles= and Include= it in every subimage.

DaanDeMeyer · 2024-07-08T07:38:09Z

Also changed the behavior to automatically drop the '@' specifier in mkosi itself to reduce the breakage caused by this PR

We don't read configuration for the genkey verb anymore so this can be dropped.

We want to check the extension on the resolved path, not on any symlinks, so let's make sure we resolve the path first.

Instead of sharing the build directory between all images, let's use a subdirectory of the build directory for subimages. This requires us to unshare the user namespace in run_build() before we create the directories so that we always have permissions to create any nested build directories.

mkosi/resources/mkosi.md

mkosi/config.py

DaanDeMeyer · 2024-07-08T09:58:13Z

Added one more fix to make sure that mkosi.local.conf still overrides configuration that is parsed later.

Let's also chown the directory to be user owned if located in /tmp or /var/tmp.

In systemd/mkosi#2847, the '@' specifier is removed, CLI arguments take priority over configuration files again and the "main" image is defined at the top level instead of in mkosi.images/. Additionally, not every setting from the top level configuration is inherited by the images in mkosi.images/ anymore, only settings which make sense to be inherited are inherited. This commit gets rid of all the usages of '@', moves the "main" image configuration from mkosi.images/system to the top level and gets rid of various hacks we had in place to deal with quirks of the old configuration parsing logic. We also remove usages of Images= and --append as these options are removed by the mentioned PR.

Parse functions should return None to pick the default value. Also, we don't get any values at all if unescape=True and the empty string is passed so make sure we handle that case as well.

As explained in systemd#2846, there are multiple issues with the current implementation of mkosi.images. Let's take what we learned from the default initrd and the default tools tree and apply it mkosi.images. Specifically, all issues arise from the fact that we apply every option from the global configuration (including CLI arguments) to the images from mkosi.images/. To avoid the issues that arise from this (e.g --package abc installing abc in all images), we made configuration values override CLI arguments again so that we could override faulty CLI arguments again in subimages so that they would only apply to the main image (e.g. set Format= explicitly for each subimage so that --format on the command line only applies to the main image). Because we still wanted to allow configurable settings that can be modified via the command line, we introduced the default specifier '@' which can be prefixed to a setting to set a default value instead of overriding the value. The '@' specifier is generally used in the global image independent configuration to specify default values that can be overridden from the command line. This specifier has led to a lot of confusion, along with the behavior that the CLI does not override the configuration. From the default tools tree and default initrd, we learned that what works very well is to only have specific settings from the main image configuration apply to the default tools tree and default initrd. For example, the distribution, release, mirror and architecture should be the same for the main image and the initrd, but the packages from the main image should not all be installed in the initrd. We can apply this idea to the images from mkosi.images/ as well, if we introduce the assumption that all images defined in mkosi.images are subimages intended to be included in some way or form in the main image. This assumption allows us to divide all settings into either image specific settings or "universal" settings that should apply to the main image and all its subimages. The universal settings are passed on to each subimage. The image specific settings are not. This idea also allows us to define the "main" image outside of mkosi.images again. Since only "universal" settings are passed on, we can safely define an output format and such again in the global configuration, as we know this won't be passed on to subimages. It also allows us to make CLI arguments override configuration again. Since there is no need anymore for subimages to override the CLI configuration as inappropriate CLI configuration such as extra packages will only apply to the main image and not any subimages from mkosi.images/. Because CLI configuration overrides file configuration again, we also don't need the '@' specifier anymore, as default values can simply be set without '@', since the CLI will override the configuration file values by default. We also lose the need for --append, because the sole use for --append was again to override file based configuration. Note that configuration from mkosi.local.conf is special in that it should override settings from other configuration files, but not settings that are specified on the CLI. This commit implements all of what's mentioned above, specifically: - CLI configuration now always trumps file based configuration. - The '@' specifier is dropped automatically during parsing - The main image is now always added from global configuration, even if there are images in mkosi.images/. The main image is always built last, and cannot be used as a dependency in the Dependencies= setting for images defined in mkosi.images/. - The Dependencies= setting for the main image now is used to specify which subimages from mkosi.images/ to build. By default all subimages are built. - A universal tag is introduced for settings and appropriate settings are marked as universal. Universal settings are passed on from the main image configuration to subimage configuration. - The Images= setting is removed, as it's role is replaced by Dependencies=. - The old name mkosi.presets and the Preset section name are removed as they have been deprecated for a long time now. - The config parsing tests are extended to cover more cases. - All builtin configuration is adapted to stop using the '@' specifier. - The documentation is updated in accordance with the changes.

In systemd/mkosi#2847, the '@' specifier is removed, CLI arguments take priority over configuration files again and the "main" image is defined at the top level instead of in mkosi.images/. Additionally, not every setting from the top level configuration is inherited by the images in mkosi.images/ anymore, only settings which make sense to be inherited are inherited. This commit gets rid of all the usages of '@', moves the "main" image configuration from mkosi.images/system to the top level and gets rid of various hacks we had in place to deal with quirks of the old configuration parsing logic. We also remove usages of Images= and --append as these options are removed by the mentioned PR.

In systemd/mkosi#2847, the '@' specifier is removed, CLI arguments take priority over configuration files again and the "main" image is defined at the top level instead of in mkosi.images/. Additionally, not every setting from the top level configuration is inherited by the images in mkosi.images/ anymore, only settings which make sense to be inherited are inherited. This commit gets rid of all the usages of '@', moves the "main" image configuration from mkosi.images/system to the top level and gets rid of various hacks we had in place to deal with quirks of the old configuration parsing logic. We also remove usages of Images= and --append as these options are removed by the mentioned PR. (cherry picked from commit 20345a8)

septatrix · 2024-07-16T23:42:42Z

Main image is now always included even when mkosi.images is used. Fix is to move the main image configuration from mkosi.images to the global configuration.

mkosi.images can only be used to build subimages now, with more restrictions put on them (same distribution, release, mirror, ...). Not all CLI settings apply to mkosi.images/ anymore (which is a good thing).

I have two images which are strongly related but neither is a subimage or dependency of the other. What is the best way to have such a configuration with the new config mechanism?

More specifically I have a system image and a separate installer. If I move the former to the main image I will not be able to build the latter separate if I understand correctly (or at least I have no clue what to pass on the command line to only build the installer).

DaanDeMeyer · 2024-07-17T08:45:35Z

Main image is now always included even when mkosi.images is used. Fix is to move the main image configuration from mkosi.images to the global configuration.

mkosi.images can only be used to build subimages now, with more restrictions put on them (same distribution, release, mirror, ...). Not all CLI settings apply to mkosi.images/ anymore (which is a good thing).

I have two images which are strongly related but neither is a subimage or dependency of the other. What is the best way to have such a configuration with the new config mechanism?

More specifically I have a system image and a separate installer. If I move the former to the main image I will not be able to build the latter separate if I understand correctly (or at least I have no clue what to pass on the command line to only build the installer).

@septatrix My suggestion here is to have two different configuration which you build separate using mkosi --directory <...>. Shared configuration between the two images can be handled with Include=.

septatrix · 2024-07-17T12:55:02Z

@septatrix My suggestion here is to have two different configuration which you build separate using mkosi --directory <...>. Shared configuration between the two images can be handled with Include=.

I came up with a different solution in the meantime. I set Format=none for the main image (which should completely ignore it?) and use a profile to swap out the Dependencies= as well as a [Match] in the system image which checks for the profile and adjusts Format=, SplitArtifacts= accordingly. However, I am not sure if this is intended or bound to be broken at some later point.

The directory approach seems a bit annoying because in the installer I need to reference the split artifacts from the system image

septatrix · 2024-07-17T13:57:42Z

Also: Will mkosi qemu/boot now always start the main image? Using mkosi.images but not being able to start nothing but the main image seems a bit of a misnomer if that limitation really exists...

DaanDeMeyer · 2024-07-17T14:09:11Z

@septatrix Yes we'll always start the main image now. We had to introduce limitations to the feature to make it usable. We're not aiming for mkosi to be a general purpose build system. If mkosi.images doesn't fit your needs, you'll need to add something on top to orchestrate multiple image builds.

septatrix · 2024-07-17T14:25:47Z

Okay, in that case I will need to resort to -C, understood. It just feels like mkosi.images is now a misnomer. Similarly mkosi summary still talk about "default" images but if there is no way to start different images that might be confusing. The way this directory is now handled is more akin to mkosi.components or something like it because the sub"images" are intended to be used standalone but instead are more like glorified build scripts

septatrix · 2024-07-17T14:54:14Z

It also seems like I need a separate mkosi.version, mkosi.crt, and mkosi.key files for each of them then... :/

DaanDeMeyer · 2024-07-17T14:55:34Z

It also seems like I need a separate mkosi.version, mkosi.crt, and mkosi.key files for each of them then... :/

Include= is your friend, you can have a directory containing all of those and simply Include= it wherever needed.

septatrix · 2024-07-17T15:25:42Z

It also seems like I need a separate mkosi.version, mkosi.crt, and mkosi.key files for each of them then... :/

Include= is your friend, you can have a directory containing all of those and simply Include= it wherever needed.

This works only to a certain degree. mkosi bump does not work and mkosi genkey still places it in the individual directories (even when settings SshCertificate=). It is better than nothing but its fair to say that my use case and current workflow has definitely been hit by this change (though I can understand why it was done)

DaanDeMeyer · 2024-07-17T15:55:55Z

It also seems like I need a separate mkosi.version, mkosi.crt, and mkosi.key files for each of them then... :/

Include= is your friend, you can have a directory containing all of those and simply Include= it wherever needed.

This works only to a certain degree. mkosi bump does not work and mkosi genkey still places it in the individual directories (even when settings SshCertificate=). It is better than nothing but its fair to say that my use case and current workflow has definitely been hit by this change (though I can understand why it was done)

Yeah I'm sorry about that, but we had to consider all the users not using mkosi.images as well

DaanDeMeyer added 3 commits July 7, 2024 17:23

Rename finalize_default() to finalize_value()

efb6a60

Fits what the function does better.

Use return value of finalize_value() in one more place

8de94b7

Doesn't change behavior, just slightly easier to read.

DaanDeMeyer force-pushed the config branch 4 times, most recently from 05fb034 to 239ca7c Compare July 7, 2024 20:41

DaanDeMeyer changed the title ~~WIP~~ Rework configuration parsing (again) Jul 7, 2024

DaanDeMeyer marked this pull request as ready for review July 7, 2024 20:49

DaanDeMeyer force-pushed the config branch 2 times, most recently from c77e18a to 491c5e6 Compare July 8, 2024 07:29

DaanDeMeyer force-pushed the config branch from 491c5e6 to 811138c Compare July 8, 2024 07:33

DaanDeMeyer force-pushed the config branch 2 times, most recently from e070b95 to 9249677 Compare July 8, 2024 08:16

DaanDeMeyer added 3 commits July 8, 2024 10:20

Drop dead code

2459740

We don't read configuration for the genkey verb anymore so this can be dropped.

Resolve source path in mount_base_trees()

e895107

We want to check the extension on the resolved path, not on any symlinks, so let's make sure we resolve the path first.

DaanDeMeyer force-pushed the config branch from 9249677 to 340fca5 Compare July 8, 2024 08:21

behrmann reviewed Jul 8, 2024

View reviewed changes

mkosi/resources/mkosi.md Outdated Show resolved Hide resolved

mkosi/config.py Outdated Show resolved Hide resolved

mkosi/config.py Outdated Show resolved Hide resolved

mkosi/config.py Outdated Show resolved Hide resolved

DaanDeMeyer force-pushed the config branch 3 times, most recently from 55e6cb7 to b1efe11 Compare July 8, 2024 09:51

DaanDeMeyer force-pushed the config branch from b1efe11 to 5207abe Compare July 8, 2024 10:14

Make INVOKING_USER.chown() take /tmp and /var/tmp into account

3395162

Let's also chown the directory to be user owned if located in /tmp or /var/tmp.

DaanDeMeyer force-pushed the config branch from 5207abe to cd66d90 Compare July 8, 2024 10:48

DaanDeMeyer added the dont-merge label Jul 8, 2024

DaanDeMeyer force-pushed the config branch from 1ea1559 to e5706bd Compare July 8, 2024 17:52

DaanDeMeyer removed the dont-merge label Jul 8, 2024

DaanDeMeyer mentioned this pull request Jul 8, 2024

mkosi: Adapt configuration to take into account configuration rework systemd/systemd#33659

Merged

DaanDeMeyer force-pushed the config branch 2 times, most recently from 4830f1d to 513d638 Compare July 8, 2024 18:46

DaanDeMeyer force-pushed the config branch from 513d638 to 33af17d Compare July 8, 2024 19:57

DaanDeMeyer added 2 commits July 8, 2024 21:59

Fix empty strings for lists and dicts

8aefea6

Parse functions should return None to pick the default value. Also, we don't get any values at all if unescape=True and the empty string is passed so make sure we handle that case as well.

DaanDeMeyer force-pushed the config branch from 33af17d to 1b8f7f2 Compare July 8, 2024 19:59

behrmann approved these changes Jul 8, 2024

View reviewed changes

DaanDeMeyer merged commit 6b66aa9 into systemd:main Jul 9, 2024
30 of 32 checks passed

DaanDeMeyer deleted the config branch July 9, 2024 06:06

DaanDeMeyer mentioned this pull request Jul 9, 2024

Configuration parser issues and possible improvements #2846

Closed

DaanDeMeyer mentioned this pull request Jul 22, 2024

Environment in mkosi.configure script #2894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework configuration parsing (again) #2847

Rework configuration parsing (again) #2847

DaanDeMeyer commented Jul 7, 2024 •

edited

Loading

DaanDeMeyer commented Jul 7, 2024

NekkoDroid commented Jul 7, 2024 •

edited

Loading

DaanDeMeyer commented Jul 8, 2024

DaanDeMeyer commented Jul 8, 2024

DaanDeMeyer commented Jul 8, 2024

septatrix commented Jul 16, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

Rework configuration parsing (again) #2847

Rework configuration parsing (again) #2847

Conversation

DaanDeMeyer commented Jul 7, 2024 • edited Loading

DaanDeMeyer commented Jul 7, 2024

NekkoDroid commented Jul 7, 2024 • edited Loading

DaanDeMeyer commented Jul 8, 2024

DaanDeMeyer commented Jul 8, 2024

DaanDeMeyer commented Jul 8, 2024

septatrix commented Jul 16, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

septatrix commented Jul 17, 2024

DaanDeMeyer commented Jul 17, 2024

DaanDeMeyer commented Jul 7, 2024 •

edited

Loading

NekkoDroid commented Jul 7, 2024 •

edited

Loading