Add toml utility module that wraps every toml Python module ever created #1851
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achilleas-k
force-pushed
the
tomlunacy
branch
3 times, most recently
from
c00e5dd
to
96f2bf9
Compare
mvo5
reviewed
Aug 16, 2024
mvo5
approved these changes
Aug 16, 2024
achilleas-k
commented
Aug 16, 2024
kingsleyzissou
approved these changes
Aug 16, 2024
mvo5
added a commit
to mvo5/images
that referenced
this pull request
Aug 16, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
bcl
approved these changes
Aug 16, 2024
|
FWIW, all test configs built successfully with this PR in osbuild/images: osbuild/images#863 |
I'm looking into the rpm build failures now. |
mvo5
added a commit
to mvo5/images
that referenced
this pull request
Aug 20, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
mvo5
added a commit
to mvo5/images
that referenced
this pull request
Aug 20, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
The toml module situation in Python is a bit of a mess. Different distro versions have different modules packaged or built-in, sometimes with different capabilities (no writing). Since we need to support reading and writing toml files both on the host (osbuild internals, sources, inputs) and in the build root (stages), let's centralise the import decision making in an internal utility module that covers all cases. Two of the modules we might import (tomli and tomllib) don't support writing, so we need to either import a separate module (tomli_w) or raise an exception when dump() is called without a write-capable module. The tomli and tomllib modules require files be opened in binary mode (not text) while the others require text mode. So we can't wrap the toml.load() and toml.dump() functions directly; the caller doesn't know which module it will be using. Let's keep track of the mode based on which import succeeded and have our functions open the files as needed. The wrapper functions are named load_from_file() and dump_to_file() to avoid confusion with the load() and dump() functions that take a file object. See also osbuild#1847
The containers.storage.conf stage writes a header explaining what the configuration is doing and its origin. It also supports adding extra comments via stage options, which we need to support. Add support for writing comments at the top of the file in the toml.dump_to_file() function.
Add a new util module called host which is used for functions that are meant for interactions with the host. These functions should not be used in stages. The containers.get_host_storage() function is renamed to host.get_container_storage() for clarity, since it is no longer namespaced under containers.
Different toml libraries write arrays differently, so we can't know exactly what the file contents will look like. Some will write an array in a single line (toml) while others will break it into one element per line (tomli_w). Parse the file that's written by the stage so we can compare the objects instead of the text contents directly.
- Specify utf-8 encoding when opening files in text mode. - Add type hints. - Prefix all the top-level names with _.
Add two unit tests for our toml util module. - Write an object with util.toml, read it with util.toml, and compare written and read objects. - Write an object directly as a string, read it with util.toml, comparing with an expected object. A test that writes with util.toml, reads as string, and verifies the read string is difficult to do in a general way, because each toml module we support writes files in a slightly different way.
Add four toml test environments, testing that osbuild.util.toml works for reading and writing with all our supported toml module combinations.
|
Branch rebased on top of rpm build failure fix |
mvo5
added a commit
to mvo5/images
that referenced
this pull request
Sep 19, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
bcl
pushed a commit
to mvo5/images
that referenced
this pull request
Sep 23, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
github-merge-queue bot
pushed a commit
to osbuild/images
that referenced
this pull request
Oct 8, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
github-merge-queue bot
pushed a commit
to osbuild/images
that referenced
this pull request
Oct 8, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
github-merge-queue bot
pushed a commit
to osbuild/images
that referenced
this pull request
Oct 9, 2024
The current code will install python3-toml on fedora and also on rhel11 (once it comes out) because that is the default for unhandled distros. This commit makes the various toml libs for each distro more explicit, see also osbuild/osbuild#1851
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The toml module story in osbuild is difficult. Different distro versions have different modules packaged or built-in, sometimes with different capabilities (no writing). Since we need to support reading and writing toml files both on the host (osbuild internals, sources, inputs) and in the build root (stages), this PR centralises the import decision making in an internal utility module that covers all cases.
The wrapper is quite ugly and there's a few reasons for that. Two of the modules we might import (
tomliandtomllib) don't support writing, so we need to either import a separate module (tomli_w) or raise an exception whendump()is called without a write-capable module. Thetomliandtomllibmodules require files be opened in binary mode (not text) while the others require text mode. So we can't wrap thetoml.load()andtoml.dump()functions directly; the caller doesn't know which module it will be using. We keep track of the mode based on which import succeeded and have our functions open the files as needed.The wrapper functions are named
load_from_file()anddump_to_file()to avoid confusion with theload()anddump()functions that take a file object.The PR also adds simple read/write tests and tox configs for testing every supported toml module.
Feel free to suggest anything that might make the module less of a mess.
Fixes #1847