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

nix(flake): avoid importing nixpkgs #25

Merged
merged 1 commit into from
Jul 7, 2024
Merged

nix(flake): avoid importing nixpkgs #25

merged 1 commit into from
Jul 7, 2024

Conversation

diniamo
Copy link
Contributor

@diniamo diniamo commented Jul 7, 2024

https://zimbatm.com/notes/1000-instances-of-nixpkgs

I'm sorry, but the rest of the flake is also a horror to look at.

@Beastwick18
Copy link
Owner

Changes LGTM, I'll go ahead and merge. Also, in what way is it a horror? I'm new to Nix and this is my first time creating a flake so suggestions would be appreciated.

@Beastwick18 Beastwick18 merged commit f09e0ac into Beastwick18:main Jul 7, 2024
4 checks passed
@diniamo
Copy link
Contributor Author

diniamo commented Jul 7, 2024

  1. Why are tabs used for indentation? I recommend alejandra for formatting
  2. Don't use flake-utils, very ugly, and abstractions like this are generally bad. The only acceptable one is flake-parts
  3. I personally use the buildRustPackage function from nixpkgs, never really seen a reason to use crane or naersk, but if there is one, then enlighten me
  4. Declaring every option from the config file is a bad idea for countless reasons (the main ones are that it makes it impossible to maintain, and in your case, you had to do that funny stuff with making every option nullable, and filtering null options - all of which can be avoided), if you take a look at some home manager modules, they just serialize an attribute set directly to the config file

@Beastwick18
Copy link
Owner

Why are tabs used for indentation? I recommend alejandra for formatting

No real reason. I should be using a formatter though so I'll try that one out.

Don't use flake-utils, very ugly, and abstractions like this are generally bad. The only acceptable one is flake-parts

I don't think I'm using flake-utils, unless its an indirect dependency from naersk?

I personally use the buildRustPackage function from nixpkgs, never really seen a reason to use crane or naersk, but if there is one, then enlighten me

From what I understand, naersk will improve build times for Rust projects by reading the Cargo.toml and caching dependencies. Basically installing each dependency as its own package in the nix store so they don't have to be rebuilt. So when updating the package only new dependencies and the resulting binary have to be built.

This may be undesired, though. I just thought it would be a good idea to improve build times.

Declaring every option from the config file is a bad idea for countless reasons (the main ones are that it makes it impossible to maintain, and in your case, you had to do that funny stuff with making every option nullable, and filtering null options - all of which can be avoided), if you take a look at some home manager modules, they just serialize an attribute set directly to the config file

I did that so there would be explicit types associated with each option. So if the config expects a list of strings then that can be type-checked when building the resulting config. From what I understand this is not that uncommon. For example, the ssh module for home-manager lists out all options with types, filters them, and then writes them to disk. It does have an option for extraConfig, which isn't checked at all, but this config is well-defined so that's not necessary here.

The reason I have to filter out null values is because TOML does not support it, so it will throw an error while building if any values are null. The only options that should be nullable are the ones which are not required, anyways.

It's also not a big deal for me to manually add each option, since I don't frequently add many new config options. The bulk of the work has already been done and only 1-2 new options are typically added each update.

@diniamo
Copy link
Contributor Author

diniamo commented Jul 7, 2024

I don't think I'm using flake-utils, unless its an indirect dependency from naersk?

Oops my bad, what you are doing is fine there.

From what I understand, naersk will improve build times for Rust projects by [...] caching dependencies.

Hm, that's fine I guess, never really cared about that.

[...]

You are greatly missing the point here, but have it your way, doesn't really matter from the user's perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants