Rewrite disko in Python #902
Conversation
This will be essential for type safety later on.
Error handling could be done better, but I'm pretty happy with it already. The important part is aggregating all errors instead of failing on the first issue we encounter.
This requires a lot of restructuring. The .nix files have to be bundled together with the python files, so they need to follow python's module system structure. I ran `nix-fast-build --no-link -j 2 --eval-workers 3 --flake .#checks` and it succeeded, so I'm reasonably confident I changed everything as required.
Other people might use other tools, but having a known-good configuration is useful.
An oversight from 389235b
Using untyped dicts was not a safe way to pass arguments to error message rendering. Also, having one giant match statement wouldn't scale well. Using function pointers and kwargs, mypy can now properly check whether all required arguments are passed to DiskoMessage(). As a bonus, it's now easy to use the "Go to definition" LSP command for error codes, which wasn't possible before.
It combines multiple tools and is much faster.
This allows to create plans only based on the things that changed in the configuration.
This makes the tests more readable and easier to browse. Making sure the tests always run fine, no matter from which directory they're started is a little detail we should either test for or document.
This is required to be able to generate sgdisk invocations that are equivalent to what's currently generated in nix.
This will be very useful for generating documentation and python type definitions.
The default `strict = true` is too permissive for my liking, especially how it allows using `Any` in many places and doesn't warn about them at all.
This will be very useful for generating documentation and python type definitions.
There are still two issues: The type of "topology" in zpool is not created, and gpt_partitions_options_hybrid_options for some reason contains a `_create: "_create"` entry. This is an issue with the nix evaluation, though, not with the code generator. I'm fixing these issues manually to have some state I can start working from.
Belongs to d382a3c, but I authored that on another machine. Making sure vscode uses mypy from the environment is very important now, because some of these errors get triggered in different ways depending on the version.
There was a problem hiding this comment.
Just a few comments from a pythonista. 🐍
Re development tooling, I'd suggest to embrace uv, which is about to become the de-facto installer and packaging tool, and ruff as a universal linter and code formatter. See painless-software/cicd/app/cli (GitLab) for an example CLI setup with Tox that wraps Ruff for formatting and linting.
Some of your code mentions that a Nix test should be run in CI. I use the docker.io/nixos/nix container images to run nix flake check. See painless-software/nixos-config (GitLab). If you want this in GHA in this repo I could contribute that.
| match type_str: | ||
| case "str": | ||
| return "str" | ||
| case "absolute-pathname": | ||
| return "str" | ||
| case "bool": | ||
| return "bool" | ||
| case "int": | ||
| return "int" | ||
| case "anything": | ||
| return "Any" | ||
| # Set up discriminated unions to reduce error messages when validation fails | ||
| case "deviceType": | ||
| return '"deviceType" = Field(..., discriminator="type")' | ||
| case "partitionType": | ||
| return '"partitionType" = Field(..., discriminator="type")' | ||
| case _: | ||
| # Probably a type alias, needs to be quoted in case the type is defined later | ||
| return f'"{type_str}"' |
There was a problem hiding this comment.
With traditional Python you would write it like that:
| match type_str: | |
| case "str": | |
| return "str" | |
| case "absolute-pathname": | |
| return "str" | |
| case "bool": | |
| return "bool" | |
| case "int": | |
| return "int" | |
| case "anything": | |
| return "Any" | |
| # Set up discriminated unions to reduce error messages when validation fails | |
| case "deviceType": | |
| return '"deviceType" = Field(..., discriminator="type")' | |
| case "partitionType": | |
| return '"partitionType" = Field(..., discriminator="type")' | |
| case _: | |
| # Probably a type alias, needs to be quoted in case the type is defined later | |
| return f'"{type_str}"' | |
| types_map = { | |
| "str": "str", | |
| "absolute-pathname": "str", | |
| "bool": "bool", | |
| "int": "int", | |
| "anything": "Any", | |
| # Set up discriminated unions to reduce error messages when validation fails | |
| "deviceType": '"deviceType" = Field(..., discriminator="type")', | |
| "partitionType": '"partitionType" = Field(..., discriminator="type")', | |
| } | |
| try: | |
| return types_map[type_str] | |
| except KeyError: | |
| # Probably a type alias, needs to be quoted in case the type is defined later | |
| return f'"{type_str}"' |
This is (still) the pythonic way. I understand that match/switch/case is known in other languages.
There was a problem hiding this comment.
True, it's more terse anyway. I used match here initially because I called a function in one of the branches, but when all the results are static, that's not needed anymore.
| buffer = io.StringIO() | ||
|
|
||
| buffer.write( | ||
| """# File generated by scripts/generate_python_types.py |
There was a problem hiding this comment.
You could write it like that, which helps to keep the comment lines visually aligned.
| """# File generated by scripts/generate_python_types.py | |
| """\ | |
| # File generated by scripts/generate_python_types.py |
|
|
||
| DEFAULT_CONFIG_FILE = "disko-config.nix" | ||
|
|
||
| HEADER_COMMENT = """ |
There was a problem hiding this comment.
Add a backslash if you don't want an empty first line.
| HEADER_COMMENT = """ | |
| HEADER_COMMENT = """\ |
|
|
||
| """ | ||
|
|
||
| PARTIAL_FAILURE_COMMENT = """ |
There was a problem hiding this comment.
Add a backslash if you don't want an empty first line.
| PARTIAL_FAILURE_COMMENT = """ | |
| PARTIAL_FAILURE_COMMENT = """\ |
There was a problem hiding this comment.
Ah right, totally forgot about this.
| nix_code = re.sub(r"^\{ disko = \{ devices", "{ disko.devices", config_as_nix.value) | ||
| nix_code = re.sub(r"\}; \}$", "}", nix_code) | ||
|
|
||
| with open(DEFAULT_CONFIG_FILE, "w") as f: |
There was a problem hiding this comment.
You might want to consider using pathlib.Path instead, e.g.
| with open(DEFAULT_CONFIG_FILE, "w") as f: | |
| with Path(DEFAULT_CONFIG_FILE).open("w") as f: |
There was a problem hiding this comment.
I know pathlib is useful for path operations and type safety, but does it have any advantage here?
See #789 for rationale.
This is very barebones, but I'm at a point where I'm confident enough in the basic structure to get feedback from the community.
How to test
You can use
nix developto get the same shell I've been developing in.After that, you can just run
./disko2to try out the CLI, orpytestto run the test suite.Right now, only the
disk,gptandfilesystemtypes are implemented.The most interesting commands to try are
./disko2 generate, which generates adisko-config.nixat the project root, and./disko2 format,mount --dry-run disko-config.nix, which will show you what commands disko would run to align your current system with the target configuration.Right now, even without
--dry-run, no commands are executed.Thoughts on the rewrite
In general, it was quite a bit of effort to get a disko config into python and use it in a type-safe manner. Now that that's possible, working with the object feels quite nice.
Error handling and communicating issue to the user properly is significantly improved, which is a huge bonus, and being able to generate configs from real world systems and test the plan generation against that should help us test disko a lot more thoroughly.
You will notice that there is a lot more code, though. While you can template complete scripts in python as well, I wanted to actively avoid that to obviate the need for interpreting commands with a shell in the first place. The idea is that all commands will be executed with
subprocess.run, which means the binaries are called directly. This will solve escaping, but might make some things (like conditional execution) more complicated to implement.To Do
After that, I also have to think about how many of the currently nix-only functionality can be implemented. How would something like
config.system.build.formatbe generated, for example?