Fix `builtins.getFlake` to handle Git based flakes properly #28

New issue

Open

opened 2024-03-13 18:52:37 +00:00 by lunaphied · 5 comments

lunaphied commented

2024-03-13 18:52:37 +00:00

Owner

Currently (as far as I understand it), builtins.getFlake ignores if the active repository is a Git repository and always treats it as a plain source repository. This causes some notable issues:

Often incredibly slow as it copies far more than it needs and often copies things to the store when there are existing copies of the Git based flake in the repo. This makes working in the REPL unpleasant as :lf and starting with a loaded flake both pollute top-level scope which isn't always desirable, and the alternative is slow or not functional given below.
Sometimes literally breaks things because it may attempt to copy things that are not in the supported set of regular files, directories, or symlinks (such as Unix sockets).

In my opinion this is outright broken and should simply be fixed, hopefully nobody is relying on this in the wild.

Currently (as far as I understand it), `builtins.getFlake` ignores if the active repository is a Git repository and always treats it as a plain source repository. This causes some notable issues: - Often incredibly slow as it copies far more than it needs and often copies things to the store when there are existing copies of the Git based flake in the repo. This makes working in the REPL unpleasant as `:lf` and starting with a loaded flake both pollute top-level scope which isn't always desirable, and the alternative is slow or not functional given below. - Sometimes literally *breaks* things because it may attempt to copy things that are not in the supported set of regular files, directories, or symlinks (such as Unix sockets). In my opinion this is outright broken and should simply be fixed, hopefully nobody is relying on this in the wild.

lunaphied added the

performance

bug

labels 2024-03-13 18:52:37 +00:00

puck commented

2024-03-13 20:14:37 +00:00

Owner

I'm a bit unsure whether this is the behavior that should happen, or not; builtins.getFlake takes a standard flakeref as input. Could you try builtins.parseFlakeRef and see what it outputs?

I'm a bit unsure whether this is the behavior that should happen, or not; `builtins.getFlake` takes a standard flakeref as input. Could you try `builtins.parseFlakeRef` and see what it outputs?

lunaphied commented

2024-03-17 20:30:14 +00:00

Author

Owner

Could you try builtins.parseFlakeRef and see what it outputs?

nix-repl> builtins.parseFlakeRef (toString ./.)
{ path = "/home/lunaphied/.config"; type = "path"; }

nix-repl> builtins.parseFlakeRef (toString "/home/lunaphied/.config")
{ path = "/home/lunaphied/.config"; type = "path"; }

What has to happen instead is something like:

nix-repl> builtins.parseFlakeRef (toString "git+file:///" + (toString ./.))
{ type = "git"; url = "file:////home/lunaphied/.config"; }

nix-repl> builtins.getFlake (toString "git+file:///" + (toString ./.))
warning: Git tree '//home/lunaphied/.config' is dirty
{ _type = "flake"; dirtyRev = "51e56a3b624ed2682028bdfc9067b603475e2c75-dirty"; dirtyShortRev = "51e56a3-dirty"; inputs = { ... }; lastModified = 1709239955; lastModifiedDate = "20240229205235"; legacyPackages = { ... }; lib = { ... }; narHash = "sha256-VIc0fORja4BOmAhcKx+FZSetrO3nphbJg+uzKfTKt7Y="; nixosConfigurations = { ... }; outPath = "/nix/store/q13janwwxhd6shv1ja6l9mgr0n34fccv-source"; outputs = { ... }; packages = { ... }; sourceInfo = { ... }; submodules = false; templates = { ... }; }

I'd define this as somewhat of a footgun since the behavior of the command-line installables interface is that the current path is treated as a Git repository if possible, whereas using it this way sees it as a full path (since you can't use a "." reference in pure mode) which prevents it from being recognized the same way, leading to behavior that's almost never wanted.

> Could you try `builtins.parseFlakeRef` and see what it outputs? ``` nix-repl> builtins.parseFlakeRef (toString ./.) { path = "/home/lunaphied/.config"; type = "path"; } nix-repl> builtins.parseFlakeRef (toString "/home/lunaphied/.config") { path = "/home/lunaphied/.config"; type = "path"; } ``` What has to happen instead is something like: ``` nix-repl> builtins.parseFlakeRef (toString "git+file:///" + (toString ./.)) { type = "git"; url = "file:////home/lunaphied/.config"; } nix-repl> builtins.getFlake (toString "git+file:///" + (toString ./.)) warning: Git tree '//home/lunaphied/.config' is dirty { _type = "flake"; dirtyRev = "51e56a3b624ed2682028bdfc9067b603475e2c75-dirty"; dirtyShortRev = "51e56a3-dirty"; inputs = { ... }; lastModified = 1709239955; lastModifiedDate = "20240229205235"; legacyPackages = { ... }; lib = { ... }; narHash = "sha256-VIc0fORja4BOmAhcKx+FZSetrO3nphbJg+uzKfTKt7Y="; nixosConfigurations = { ... }; outPath = "/nix/store/q13janwwxhd6shv1ja6l9mgr0n34fccv-source"; outputs = { ... }; packages = { ... }; sourceInfo = { ... }; submodules = false; templates = { ... }; } ``` I'd define this as somewhat of a footgun since the behavior of the command-line installables interface is that the current path is treated as a Git repository if possible, whereas using it this way sees it as a full path (since you can't use a "." reference in pure mode) which prevents it from being recognized the same way, leading to behavior that's almost never wanted.

puck commented

2024-03-18 13:54:08 +00:00

Owner

Ah! That's a more actionable issue ("absolute paths are parsed as path flakes, rather than as git flakes"); and one that seems a lot more reasonable to fix (and we probably want to fix, as well.)

lunaphied commented

2024-03-18 14:43:22 +00:00

Author

Owner

Would detecting a .git subdirectory in an unqualified absolute path and assuming it to be a Git flake be a reasonable solution? I still want to be able to force path flake usage if desired but this seems like a reasonable way to handle unqualified?

Would detecting a `.git` subdirectory in an unqualified absolute path and assuming it to be a Git flake be a reasonable solution? I still want to be able to force path flake usage if desired but this seems like a reasonable way to handle unqualified?