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

Refactor flake.nix to remove flake-utils #177

Closed
wants to merge 2 commits into from

Conversation

toastal
Copy link
Contributor

@toastal toastal commented Apr 1, 2024

There is no reason to be sending downstream dependencies for a for loop. Includes missing devShells.

@toastal toastal force-pushed the remove-flake-utils branch from 1ec4bb0 to 3515b0a Compare April 1, 2024 08:10
@toastal toastal changed the title remove flake-utils Refactor flake.nix to remove flake-utils Apr 1, 2024
@toastal toastal force-pushed the remove-flake-utils branch from 3515b0a to 58004ac Compare April 1, 2024 11:36
@toastal
Copy link
Contributor Author

toastal commented Apr 1, 2024

There’s probably another, perhaps better way to achieve this, but there should be a goal to not be adding dependencies to users’ flake.lock files for a trivial function. I added this flake to a project to test the new rules out & was shocked to see dependencies needed.

@dasJ dasJ requested a review from infinisil April 2, 2024 06:39
There is no reason to be sending downstream dependencies for a for
loop.
@toastal toastal force-pushed the remove-flake-utils branch 2 times, most recently from 18ba70c to 9f648d8 Compare April 2, 2024 07:55
You will have trouble using this tho with direnv since `use nix` is
hard-coded into the project’s `.envrc`
@toastal toastal force-pushed the remove-flake-utils branch from 9f648d8 to 04520f6 Compare April 2, 2024 07:56
@toastal
Copy link
Contributor Author

toastal commented Apr 2, 2024

Split into 2 commits in case someone is opposed to adding the devShells.default since use nix is hard-coded into the .envrc.

Comment on lines -6 to +3
{ flake-utils, self }:
flake-utils.lib.eachDefaultSystem (
system:
let
result = import ./default.nix { inherit system; };
in
{
packages = result.packages // {
default = result;
};
{ self, nixpkgs }:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed flake-utils, but added nixpkgs (and that without an inputs entry, which then runs into NixOS/nix#7422). And if you don't commit the lockfile as here, we'll run into further non-reproducibility. Alternatively if you do commit the lockfile, we'll have two Nixpkgs versions that need to be fetched for the flake.

So, I don't think we should do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the current version using the builtins dependencies of flake-utils being more reproducible than using the lib from Nixpkgs? Seems most folks are used to passing in inputs.nixpkgs.follows thru their flakes. I suppose you could get lib from npins, but really after thinking about it, might it just be simpler to manually write out x86_64-linux, et al. for system.exposedFlakes and remove all dependencies then? It’s more repetitious “code”, but has zero dependencies. I would be willing to write it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, 32-bit Linux is one of the supported systems in Hydra but is missing from flake-utils’s defaultSystems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the current version using the builtins dependencies of flake-utils being more reproducible than using the lib from Nixpkgs?

Just because in the current PR at least, Nixpkgs isn't pinned, while flake-utils is. Without committing the lockfile to pin it, people will get different results at different points in time.

Seems most folks are used to passing in inputs.nixpkgs.follows thru their flakes

That doesn't actually work, the npins-pinned Nixpkgs can't be overridden like that (see #166 for what works instead)

might it just be simpler to manually write out x86_64-linux, et al. for system.exposedFlakes and remove all dependencies then? It’s more repetitious “code”, but has zero dependencies. I would be willing to write it out.

Actually one thing that flake-utils allows you is to use the https://github.com/nix-systems/nix-systems pattern, which allows consumers to work around the problem of Flakes requiring a hardcoded list of systems. This wouldn't be possible anymore if we hardcode the systems locally.

Copy link
Contributor Author

@toastal toastal Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Fair assessments as I misunderstand the environment. Thank you for explaining as I don’t see many places with a flake.nix & not using Nix to pin their Nixpkgs as well (which is why it feels like Nixpkgs is always a given).

Comment on lines +27 to +29
devShells = forAllSystems (system: {
default = result.${system}.shell;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using nix develop over nix-shell for its caching, or being able to add the shell to another dev environment if one really wanted. The information to make it exists, & it’s little to expose it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, sounds fine to have this :)

Personally I'm using https://github.com/nix-community/nix-direnv, which also caches the shell for speed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the flake-utils part is staying, I could just put in this commit & axe the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

@toastal toastal closed this Apr 2, 2024
@toastal toastal deleted the remove-flake-utils branch April 2, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants