ma27 commented

Member

Describe the bug

Given the following flake (kept very short for simplicity):

{
  inputs.vmtools.url = "git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools";
  outputs = { vmtools, ... }: {
    an-attribute = "${vmtools}";
  };
}

and a .envrc with

use flake

Also, run direnv allow.

When I run nix flake show, I'll get the following output now:

warning: updating lock file '/home/ma27/tmp/flake.lock':
• Updated input 'vmtools':
    'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27)
  → 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27)
path:/home/ma27/tmp?lastModified=1723828908&narHash=sha256-muAfMCv/cSR2yyrThDuDwv1fsuSQEyXCQGLoRCfdaco%3D
└───an-attribute: unknown
direnv: loading ~/tmp/.envrc                                                                                                                                                                                                                              
direnv: using flake
warning: updating lock file '/home/ma27/tmp/flake.lock':
• Updated input 'vmtools':
    'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27)
  → 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27)
error: flake 'path:/home/ma27/tmp' does not provide attribute 'devShells.x86_64-linux.default', 'devShell.x86_64-linux', 'packages.x86_64-linux.default' or 'defaultPackage.x86_64-linux'

The direnv error is expected, more interesting is that both direnv and nix invocations (using Lix) re-lock the vmtools input.

The cause for that is that Nix locks the URL to https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools in flake.lock whereas Lix locks it to https://git.pub.solar/pub-solar/infra-vintage (since ?dir is a flake-specific thing and has nothing to do with libfetchers).

Also, there's a dir attribute already in the lockfile-entry which is what flakes actually use. The URL parameter serves no additional purpose.

Potential fixes

Bump lockfile version

I think it's valid to change the semantics of flake.lock, it's versioned after all.
The actual issue here is that direnv uses a wrong Nix. A bumped lockfile version would make that clear (also, we should provide a nix-direnv version that uses Lix unless that already happened).

Just bumping it from 7 to 8 is not an option though: as soon as Nix does that (and the change is a different one), it will be pretty chaotic.

Doing "version": "lix-N" won't work since Nix expects the version to be an integer.
Another option might be to bump the lockfile version very very high.

Maintain compat with Nix lockfiles

We could accept dir in libfetchers again and make sure it's kept in the locked URL.

Opinions @jade @pennae @qyriad

Tested with nix (Lix, like Nix) 2.92.0-devpre20240813_b016eb0 for CLI and nix-direnv 3.0.5 using Nix 2.18.5.

# Describe the bug Given the following flake (kept very short for simplicity): ```nix { inputs.vmtools.url = "git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools"; outputs = { vmtools, ... }: { an-attribute = "${vmtools}"; }; } ``` and a `.envrc` with ``` use flake ``` Also, run `direnv allow`. When I run `nix flake show`, I'll get the following output now: ``` warning: updating lock file '/home/ma27/tmp/flake.lock': • Updated input 'vmtools': 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27) → 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27) path:/home/ma27/tmp?lastModified=1723828908&narHash=sha256-muAfMCv/cSR2yyrThDuDwv1fsuSQEyXCQGLoRCfdaco%3D └───an-attribute: unknown direnv: loading ~/tmp/.envrc direnv: using flake warning: updating lock file '/home/ma27/tmp/flake.lock': • Updated input 'vmtools': 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27) → 'git+https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools&ref=refs/heads/main&rev=0d039dcf06afb8cbddd7ac54bae4d0d185f3e88e' (2023-10-27) error: flake 'path:/home/ma27/tmp' does not provide attribute 'devShells.x86_64-linux.default', 'devShell.x86_64-linux', 'packages.x86_64-linux.default' or 'defaultPackage.x86_64-linux' ``` The direnv error is expected, more interesting is that both direnv and `nix` invocations (using Lix) re-lock the `vmtools` input. The cause for that is that Nix locks the URL to `https://git.pub.solar/pub-solar/infra-vintage?dir=vmtools` in flake.lock whereas Lix locks it to `https://git.pub.solar/pub-solar/infra-vintage` (since `?dir` is a flake-specific thing and has nothing to do with libfetchers). Also, there's a `dir` attribute already in the lockfile-entry which is what flakes actually use. The URL parameter serves no additional purpose. # Potential fixes ## Bump lockfile version I think it's valid to change the semantics of `flake.lock`, it's versioned after all. The actual issue here is that direnv uses a wrong Nix. A bumped lockfile version would make that clear (also, we should provide a nix-direnv version that uses Lix unless that already happened). Just bumping it from 7 to 8 is not an option though: as soon as Nix does that (and the change is a different one), it will be pretty chaotic. Doing `"version": "lix-N"` won't work since Nix expects the version to be an integer. Another option might be to bump the lockfile version very very high. ## Maintain compat with Nix lockfiles We could accept `dir` in libfetchers again and make sure it's kept in the locked URL. --- Opinions @jade @pennae @qyriad --- Tested with `nix (Lix, like Nix) 2.92.0-devpre20240813_b016eb0` for CLI and `nix-direnv 3.0.5` using Nix 2.18.5.

ma27 added the

bug

RFD

labels 2024-08-16 17:40:49 +00:00

qyriad added the

Area/flakes

label 2024-08-16 17:49:16 +00:00

jade commented

2024-08-16 22:00:32 +00:00

Owner

I think that we're stuck between a rock and a hard place by cppnix versioned interface semantics, yet again. And this interface is ridiculously fragile.

Either we implement newer versions of their semantics (e.g. nix profile manifest), not care and simply be incompatible, drag them forward to our own semantics by laboriously porting patches to cppnix (e.g. integer overflow) or freeze our interface in time at some convenient version (e.g. nix protocol).

Honestly, the file format compatibility between Lix and CppNix is going to probably be the first thing to cause divergence. Especially flake lock files, because we are unlikely to want to merge flake internals changes from them.

Taking a step back, actually the bug is that we are not just accepting the lock file without modifying it when it is semantically identical, right? And likewise they are doing the same on 2.18. Maybe we should stop doing that? It would at least stop this user facing behaviour surviving more than one cycle.

I think that we're stuck between a rock and a hard place by cppnix versioned interface semantics, yet again. And this interface is ridiculously fragile. Either we implement newer versions of their semantics (e.g. nix profile manifest), not care and simply be incompatible, drag them forward to our own semantics by laboriously porting patches to cppnix (e.g. integer overflow) or freeze our interface in time at some convenient version (e.g. nix protocol). Honestly, the file format compatibility between Lix and CppNix is going to probably be the first thing to cause divergence. Especially flake lock files, because we are unlikely to want to merge flake internals changes from them. Taking a step back, *actually* the bug is that we are not just accepting the lock file without modifying it when it is semantically identical, right? And likewise they are doing the same on 2.18. Maybe we should stop doing *that*? It would at least stop this user facing behaviour surviving more than one cycle.

ma27 commented

2024-08-17 13:55:48 +00:00

Author

Member

Taking a step back, actually the bug is that we are not just accepting the lock file without modifying it when it is semantically identical, right?

So there's one case I can think of where it makes a difference, but I'm not sure if it's that realistic: what if you have a git server that accepts query params in its clone URLs (dir to be precise).

I think the correct way then is to provide a URL with all the query params, but rely on attributes for the rest instead of additional query params:

inputs.foobar = {
  type = "git";
  url = "https://mygitrepo?dir=yes-this-needs-a-dir-query-param";
  dir = "vmtools";
}

However, I'm not sure if there's a reasonable way to differentiate between URLs requiring ?dir= for the clone and URLs having it for flake reasons without refetching the input based on what's in the lockfile which is probably not desirable.

In the example above the dir attribute is what makes the difference. So this seems at least theoretically possible.

If we consider this case negligible, then we can surely avoid refetches (having a hard time to estimate the amount of hackery for that though). I think this is the only semantic difference we currently have in flake.lock, correct?

Especially flake lock files, because we are unlikely to want to merge flake internals changes from them.

Agreed.

May I ask if you or anyfew else has a better idea than just bumping the version to a very high number (or actually, a negative number should also work since CppNix only accepts ranges of lockfile versions)? The problem I see right now is that we want CppNix to error out reasonably if it encounters a Lix lockfile, however if "version" is not an integer, you just something get something like this:

error: [json.exception.type_error.302] type must be number, but is string

EDIT: hmm, perhaps the "dir is a query param" thing isn't that unrealistic since e.g. tarball fetchers are also affected, I think?

> Taking a step back, actually the bug is that we are not just accepting the lock file without modifying it when it is semantically identical, right? So there's one case I can think of where it makes a difference, but I'm not sure if it's _that_ realistic: what if you have a git server that accepts query params in its clone URLs (`dir` to be precise). I think the correct way then is to provide a URL with all the query params, but rely on attributes for the rest instead of additional query params: ``` inputs.foobar = { type = "git"; url = "https://mygitrepo?dir=yes-this-needs-a-dir-query-param"; dir = "vmtools"; } ``` However, I'm not sure if there's a reasonable way to differentiate between URLs requiring `?dir=` for the clone and URLs having it for flake reasons without refetching the input based on what's in the lockfile which is probably not desirable. In the example above the `dir` attribute is what makes the difference. So this seems at least theoretically possible. If we consider this case negligible, then we can surely avoid refetches (having a hard time to estimate the amount of hackery for that though). I think this is the only semantic difference we currently have in flake.lock, correct? > Especially flake lock files, because we are unlikely to want to merge flake internals changes from them. Agreed. May I ask if you or anyfew else has a better idea than just bumping the version to a very high number (or actually, a negative number should also work since CppNix only accepts ranges of lockfile versions)? The problem I see right now is that we want CppNix to error out reasonably if it encounters a Lix lockfile, however if `"version"` is not an integer, you just something get something like this: ``` error: [json.exception.type_error.302] type must be number, but is string ``` ---- EDIT: hmm, perhaps the "`dir` is a query param" thing isn't that unrealistic since e.g. tarball fetchers are also affected, I think?

jade commented

2024-08-18 19:52:06 +00:00

Owner

I don't actually think that we want cppnix to error out when it gets a lix lock file. The thing about the lock format is it's intended to just get dumped into fetchTree, and if the generator was different sure maybe it can think the locked urls are wrong and want to redo them, but it should just dump it into fetchTree and work, in a reasonable world.

This is in a similar vein to npm lock files where different implementations will resolve them differently but when you have a lock file it's cross compatible.

However, is our fetchTree incompatible? I don't know.

Personally I do not consider it acceptable at all to break CppNix being able to read our lock files and vice versa, even as I would rather think about other things than cleaning up other people's flake tech debt.

The most responsible thing to do is to find all the broken shit in the URL currently and eliminate it and make a new, standard, lock file version that CppNix supports. Also ideally reduce the amount of shit in the lock file that's implementation details like the ones that led to this problem in the first place. And finally don't immediately start writing files in the new version and breaking older software.

I don't actually think that we want cppnix to error out when it gets a lix lock file. The thing about the lock format is it's *intended* to just get dumped into fetchTree, and if the generator was different sure maybe it can think the locked urls are wrong and want to redo them, but it *should* just dump it into fetchTree and work, in a reasonable world. This is in a similar vein to npm lock files where different implementations will resolve them differently but when you have a lock file it's cross compatible. However, is our fetchTree incompatible? I don't know. Personally I do not consider it acceptable at all to break CppNix being able to read our lock files and vice versa, even as I would rather think about other things than cleaning up other people's flake tech debt. The most responsible thing to do is to find all the broken shit in the URL currently and eliminate it and make a new, standard, lock file version that CppNix supports. Also ideally reduce the amount of shit in the lock file that's implementation details like the ones that led to this problem in the first place. And finally don't immediately start writing files in the new version and breaking older software.

ma27 commented

2024-09-01 20:31:49 +00:00

Author

Member

Apologies for the delayed response!

I agree that yes, if there's nothing to fetch, this isn't supposed to happen.

But I think it may be the desired behavior in one case, the tarball fetcher: there, flake-specific query params and the URL to fetch are mixed together quite badly. As a result, there's no reasonable way to check if the ?dir= part of the URL is there for flake reasons or because it's needed for the download. Hence, the current behavior would be correct in this one case I think.

Does that make (more) sense? :)

Apologies for the delayed response! I agree that yes, if there's nothing to fetch, this isn't supposed to happen. But I think it may be the desired behavior in one case, the tarball fetcher: there, flake-specific query params and the URL to fetch are mixed together quite badly. As a result, there's no reasonable way to check if the `?dir=` part of the URL is there for flake reasons or because it's needed for the download. Hence, the current behavior would be correct in this one case I think. Does that make (more) sense? :)

jade commented

2024-09-01 22:50:01 +00:00

Owner

Ugh. Yeah it's probably reasonable, though putting ?dir in there would just break your flake as well, if it was required for the URL. None of what was designed into there was a good idea to begin with and we are paying the price.

Ugh. Yeah it's probably reasonable, though putting ?dir in there would just break your flake as well, if it *was* required for the URL. None of what was designed into there was a good idea to begin with and we are paying the price.

ma27 commented

2024-09-09 13:44:01 +00:00

Author

Member

None of what was designed into there was a good idea to begin with and we are paying the price.

Agreed!
Only trying to make it a little less weird for everyone :)

Yeah it's probably reasonable, though putting ?dir in there would just break your flake as well, if it was required for the URL

Yes. But that's the case already since #174 with the suggested workaround to do { url = ...; type = "tarball"; } instead of the URL syntax.

I guess that's enough informatino to give it a try.
Might take me a bit, currently preparing for the last math exam on my schedule :)

> None of what was designed into there was a good idea to begin with and we are paying the price. Agreed! Only trying to make it a little less weird for everyone :) > Yeah it's probably reasonable, though putting ?dir in there would just break your flake as well, if it was required for the URL Yes. But that's the case already since https://git.lix.systems/lix-project/lix/issues/174 with the suggested workaround to do `{ url = ...; type = "tarball"; }` instead of the URL syntax. I guess that's enough informatino to give it a try. Might take me a bit, currently preparing for the last math exam on my schedule :)

❤️ 1

1

ma27 self-assigned this 2024-09-15 17:01:49 +00:00

ma27 referenced this issue

2024-09-15 21:09:33 +00:00

Lix and CppNix disagree on how `?ref=` in flake inputs should be represented in lockfiles #520

Lix lockfiles differ Nix lockfiles #472

Describe the bug

Potential fixes

Bump lockfile version

Maintain compat with Nix lockfiles