oddlama / nix-topology

🍁 Generate infrastructure and network diagrams directly from your NixOS configurations
https://oddlama.github.io/nix-topology
MIT License
576 stars 25 forks source link

flake: remove dependency on flake-utils and enhance enforcement of formatting #46

Open jaredmontoya opened 3 weeks ago

jaredmontoya commented 3 weeks ago

In my opinion nixfmt-rfc-style is better than alejandra because alejandra makes nix code look confusing, to switch to nixfmt just replace alejandra.enable with nixfmt.enable in treefmt.nix and execute nix fmt. You might also want to enable more formatters to format non-nix nix-topology code with nix fmt by adding prettier.enable or yamlfmt.enable to treefmt.nix for full list of supported formatters check treefmt-nix

oddlama commented 1 week ago

Thanks for your contribution, I think there are some nice things that I'd like to merge, but I'm not 100% certain on everything included:

* Removes dependency on flake-utils to reduce download size

While you have removed flake-utils you simultaneously added nix-systems/default, which basically just defines a list with 4 entries, but requires a whole repo?. I guess it would be better to just list the systems directly. Following that thought, I'd rather switch the project to utilize flake-parts, which this project is kind-of already made for.

* Removes pre-commit hooks because they do not guarantee style enforcement. reduces download size too.

Pre-commit-hooks don't guarantee anything of course, but they allow us to catch mistakes before pushing. Of course you can bypass it forcefully, but that's not the point.

* Adds treefmt-nix that unifies `nix fmt` and `nix flake check` and allows to use multiple formatters and linters with `nix fmt` at once.

I can get behind switching to treefmt, I tried this in some other projects alraedy and it looked good.

* Adds a github action that runs `nix flake check` on every pull request to ensure consistent styling, and perform all other checks defined in the flake should they be added in the future.

Sounds great, if this were a standalone PR I would already have merged it :)

In my opinion nixfmt-rfc-style is better than alejandra because alejandra makes nix code look confusing, to switch to nixfmt just replace alejandra.enable with nixfmt.enable in treefmt.nix and execute nix fmt. You might also want to enable more formatters to format non-nix nix-topology code with nix fmt by adding prettier.enable or yamlfmt.enable to treefmt.nix for full list of supported formatters check treefmt-nix

I find that both of them have weird edge-cases and shortcomings that make stuff look bad. But I'd probably switch to rfc-style simlpy because it is the new upstream default - and having defaults is usually good.


I think splitting this in to several PRs would be a good idea, so we can discuss the topics separately and they don't block each other. Let me know if you want to do that and are willing to invest the the time. Not sure how familiar you are with flake-parts, I can start with that conversion if you like. The separate topics I have in mind are:

jaredmontoya commented 1 week ago

systems are a dependency of flake-utils so 2dep - 1dep = 1dep. As you can see it is very small and has no git history so "whole repo" is an exaggaration. Also systems is in the flake-registry, if you remove

systems.url = "github:nix-systems/default";

flake.nix won't break as systems input will be resolved to this url by the nix cli. I chose to define it explicitly to show where systems come from.

In short, most people already have systems in their flake.lock even if they don't notice it and it's footprint is almost non existent compared to other flakes.

The most important thing however is that flake-utils does not have systems as a dependency for no reason. It allows consumers of the flake to override the systems input and customize the list of supported systems. If you set your own systems input(inline definition or reference with a url to your own repo) you can then add

inputs.systems.follows = "systems";

to other inputs that have systems as a dependency and synchronize supported systems list across all of them.

So if you switch to flake-parts(which is also in the flake-registry btw) it would be a good idea to just inherit systems; from the systems input to allow easy overrides of supported systems list. Which will keep systems as a dependency.


I didn't ever use flake-parts because I always tried to write my nix flakes with the smallest amount of dependencies possible. Most of my nix flakes only have nixpkgs systems and treefmt-nix as inputs because they all provide functionality that can't be achieved in other ways. flake-parts on the other hand, always looked to me like glue that adds nothing of inherent value to the nix flake from the consumer perspective. To the consumer nix-topology will perform it's topology generation function the same, with or without using flake-parts internally.

You can switch to flake-parts if you really want to but I won't be able to help with implementing that. (Also consider that I am not the only one who knows nothing about flake-parts and the barrier to code contributions will increase where it is used)


Considering that I added a github action that executes nix flake check and those checks include styling thanks to treefmt-nix, catching errors before pushing is not necessary because CI will catch it anyway.

Unless it is a direct push without a pull request that only people with write access to the repository can do, in this case pre commit hooks can indeed catch something.

and change pre-commit-hooks to call treefmt

If you want to keep pre-commit formatting together with the github action I think this is the best way to do it.


I think you should only hard code system in the simple example, not the complex one.

oddlama commented 1 week ago

systems are a dependency of flake-utils so 2dep - 1dep = 1dep.

Fair.

The most important thing however is that flake-utils does not have systems as a dependency for no reason. It allows consumers of the flake to override the systems input and customize the list of supported systems.

I understand the idea behind this input, although this only benefits projects which expose outputs that are actually dependent on the system - so mostly flakes that expose packages. On the other hand nix-topology only exposes modules and overlays, so the systems will already be those the user has chosen in their configuration. Everything that is in our forEachSystem block is only relevant for the development environment of nix-topology, so primarily the devshell and some related stuff.

So if you switch to flake-parts(which is also in the flake-registry btw) it would be a good idea to just inherit systems; from the systems input to allow easy overrides of supported systems list. Which will keep systems as a dependency.

flake-parts approaches this differently and does not need an input to provide overridable systems. Since everything is added as a module it will implicitly refer to the end-user's systems definition, and not that of its own flake. There are no system dependent outputs that need to be referred to, so there is no pre-selection of systems.

I didn't ever use flake-parts because I always tried to write my nix flakes with the smallest amount of dependencies possible. Most of my nix flakes only have nixpkgs systems and treefmt-nix as inputs because they all provide functionality that can't be achieved in other ways. flake-parts on the other hand, always looked to me like glue that adds nothing of inherent value to the nix flake from the consumer perspective. To the consumer nix-topology will perform it's topology generation function the same, with or without using flake-parts internally.

I was also skeptical at first for similar reasons. I guess it only adds value to the consumer if the consumer also uses flake-parts, and mostly by providing the usual benefits of having a module system. I like flake-parts nowadays adds value to me as a developer where I can more easily declare my development environment. Being able to reuse the work others already did is enough benefit, at least to me personally.

One of the main problems with reducing dependency count is that flakes just don't discern between inputs required for development and inputs required for consumption (among other design flaws). You always have to download all the unnecessary stuff as a consumer, or need to manually overwrite the inputs with null. But I guess we can't do much about this right now.

You can switch to flake-parts if you really want to but I won't be able to help with implementing that. (Also consider that I am not the only one who knows nothing about flake-parts and the barrier to code contributions will increase where it is used)

Since flake-parts only affects how the development stuff of nix-topology is defined, we are basically only talking about the devshell, pre-commit-hooks and treefmt. Nothing else will change, so this will only increase the barrier for contributions including changes to the flake's top-level.

Considering that I added a github action that executes nix flake check and those checks include styling thanks to treefmt-nix, catching errors before pushing is not necessary because CI will catch it anyway.

Unless it is a direct push without a pull request that only people with write access to the repository can do, in this case pre commit hooks can indeed catch something.

Of course it is not necessary, the important part to me is that it catches the common stuff early. It's great if the CI enforces it, but immediate feedback is even better than delayed feedback.

and change pre-commit-hooks to call treefmt

If you want to keep pre-commit formatting together with the github action I think this is the best way to do it.

I think you should only hard code system in the simple example, not the complex one.

Agreed, sounds like a good idea. Thanks for all your thoughts :)

jaredmontoya commented 1 week ago

nix-topology only exposes modules and overlays, so the systems will already be those the user has chosen in their configuration.

Fair.

flake-parts approaches this differently and does not need an input to provide overridable systems. Since everything is added as a module it will implicitly refer to the end-user's systems definition,

Sounds like this systems inheritance requires the consumer to be a flake-parts based flake and use of nix-topology as a flakeModule, but It is not a bad thing as long as nix-topology is also usable without flake-parts.

oddlama commented 1 week ago

Sounds like this systems inheritance requires the consumer to be a flake-parts based flake and use of nix-topology as a flakeModule, but It is not a bad thing as long as nix-topology is also usable without flake-parts.

Yes that's right. For general flake-parts projects you'd also need flake-parts downstream to benefit. In our case it will always stay usable without flake-parts as long as we expose normally expose the modules and overlays in the flake outputs.