"path (...) is not valid" is a highly non-descriptive error and doesn't say anything about what is corrupted if anything

jade commented

2024-05-05 23:25:55 +00:00

Owner

Action items: audit where we throw a "is not valid" error in the Lix codebase, figure out the conditions they represent and give them at least unique messages, because there are also multiple of them.

Action items: audit where we throw a "is not valid" error in the Lix codebase, figure out the conditions they represent and give them at least unique messages, because there are *also* multiple of them.

jade added the

ux

label 2024-05-05 23:29:31 +00:00

ckie commented

2024-05-06 00:13:16 +00:00

Member

there's 10 usages and the bug I was having falls under "Store::isValidPath says its invalid", presumably lix thinks it's gonna copy the path but doesn't but expects to read it afterwards anyway.

suboptimal matrix ref (private beta)

there's 10 usages and the bug I was having falls under "Store::isValidPath says its invalid", presumably lix thinks it's gonna copy the path but doesn't but expects to read it afterwards anyway. [suboptimal matrix ref (private beta)](https://matrix.to/#/!UmoSKzItRVfhTVQHKz:matrix.org/$NVzxpa0Snj2Q_nMWSevGdayoTobFJ_jzJP9zp-S6K8o?via=lix.systems&via=matrix.org&via=fairydust.space)

ckie referenced this issue

2024-05-06 00:47:45 +00:00

`nix-instantiate --eval` ambigously fails to create -source storepath #271

qyriad added the

Area/store

label 2024-05-06 00:53:09 +00:00

midnightveil commented

2024-05-18 03:19:02 +00:00

Member

I'm taking at look at this FYI, seeing if I can knock some easy ones off.

This is my identification list so far, ~~(will be edited)~~, I can't find others. See https://gerrit.lix.systems/c/lix/+/1161

LocalFSStore::narFromPath, testable via nix path-info nixpkgs#bash (well, until #323 is (maybe)? fixed)
queryPathInfo (see: https://gerrit.lix.systems/c/lix/+/1159)
LocalStore::queryPathInfoInternal (bad hash error, says "invalid-path")
LocalStore::queryValidPathId().
RemoteFSAccessor::fetch
LocalStoreAccessor::toRealPath
BinaryCacheStore::addToStoreCommon
builtins.exec

Other notes:

this code appears to check for the existence of "is not valid" when communicating with a remote store....
LegacySSHStore::narFromPath does not appear to have error handling? same with RemoteFSStore

I'm taking at look at this FYI, seeing if I can knock some easy ones off. This is my identification list so far, ~~(will be edited)~~, I can't find others. See https://gerrit.lix.systems/c/lix/+/1161 - [x] `LocalFSStore::narFromPath`, testable via `nix path-info nixpkgs#bash` (well, until #323 is (maybe)? fixed) - [x] `queryPathInfo` (see: https://gerrit.lix.systems/c/lix/+/1159) - [x] `LocalStore::queryPathInfoInternal` (bad hash error, says "invalid-path") - [x] `LocalStore::queryValidPathId()`. - [x] `RemoteFSAccessor::fetch` - [x] `LocalStoreAccessor::toRealPath` - [x] `BinaryCacheStore::addToStoreCommon` - [x] `builtins.exec` Other notes: - [this code](https://git.lix.systems/lix-project/lix/src/commit/7a3745b07607d3fc85fb5a0a08832ab078080884/src/libstore/remote-store.cc#L313-L315) appears to check for the existence of `"is not valid"` when communicating with a remote store.... - [ ] `LegacySSHStore::narFromPath` does not appear to have error handling? same with RemoteFSStore

midnightveil referenced this issue

2024-05-18 03:22:01 +00:00

nix path-info fails as the 'path is not valid' after 'fetching' them #323

jade commented

2024-05-18 04:02:32 +00:00

Author

Owner

hmmmmmmmmmm. it does look like this is for compat with possibly ancient daemon versions, and i don't think we should seriously care about daemons older than 2.3. do we have a history of what the protocol version is?

jade commented

2024-05-18 04:11:02 +00:00

Author

Owner

Alright I have retrieved an answer: any version strictly less than 21, we do not care the slightest bit about, and we can and should just delete the code for it and fail at the very start of the connection.

https://github.com/NixOS/nix/blob/2.3/src/libstore/worker-protocol.hh#L9

Alright I have retrieved an answer: any version strictly less than 21, we do not care the slightest bit about, and we can and should just delete the code for it and fail at the very start of the connection. https://github.com/NixOS/nix/blob/2.3/src/libstore/worker-protocol.hh#L9

jade commented

2024-05-18 04:13:06 +00:00

Author

Owner

Raised an issue to do that: #325

Raised an issue to do that: https://git.lix.systems/lix-project/lix/issues/325

👍 1

midnightveil commented

2024-05-18 09:19:04 +00:00

Member

Whilst wrapping my head around the codebase, I was trying to understand precise meaning of e.g. the "InvalidPath" error, and "QueryValidPath" / "isValidPath".

From a user-level error message POV, I've been leaning towards "path blah does not exist in the store", because that's relatively unambiguous.

But within the code (and previously exposed), was the conception of "InvalidPath". As far as I can tell (with some exceptions (maybe many? I can't see others, see cl/1159) this means "the path does not exist in the store".

In contrast, the BadStorePath error means "this store path is not allowed"; sometimes this is just Error.

Personally, invalid is closer to illegal than non-existent, which makes this more confusing than necessary. (Same as "validStorePath" when querying (possibly-remote) stores; though admittedly it they will return false if it's an illegal store path, but that also just means it's not in the store).
i.e. all illegal store paths are a subset of all non-existent store paths.

Admittedly, this is probably leaning into the "very hard breaking API change", but having (and defninig somewhere!) terms for "path is in the store" (curr: validPath), "path is not in the store" (curr: invalidPath), and "path is not allowed to be in the store" (curr(roughly): badstorepath) in a consistent manner would be nice.

(queryValidPath isn't probably the least bad, it's somewhat intuitive especially if functions had doc comments. just queryPath would probably make more sense, though).

I can think of maybe, IllegalStorePath (I mean, BadStorePath is /fine/), and maybe NonResidentStorePath? (that's kinda long though). Missing? Unavailable? PathNotFound?

Whilst wrapping my head around the codebase, I was trying to understand precise meaning of e.g. the "InvalidPath" error, and "QueryValidPath" / "isValidPath". From a user-level error message POV, I've been leaning towards "path blah does not exist in the store", because that's relatively unambiguous. But within the code (and previously exposed), was the conception of "InvalidPath". As far as I can tell (with some exceptions (maybe many? I can't see others, see [cl/1159](https://gerrit.lix.systems/c/lix/+/1159)) this means "the path does not exist in the store". In contrast, the BadStorePath error means "this store path is not allowed"; sometimes this is just Error. Personally, invalid is closer to illegal than non-existent, which makes this more confusing than necessary. (Same as "validStorePath" when querying (possibly-remote) stores; though admittedly it they will return false if it's an illegal store path, but that also just means it's not in the store). i.e. all illegal store paths are a subset of all non-existent store paths. Admittedly, this is probably leaning into the "very hard breaking API change", but having (and defninig somewhere!) terms for "path is in the store" (curr: validPath), "path is not in the store" (curr: invalidPath), and "path is not allowed to be in the store" (curr(roughly): badstorepath) in a consistent manner would be nice. (queryValidPath isn't probably the least bad, it's somewhat intuitive especially if functions had *doc comments*. just queryPath would probably make more sense, though). I can think of maybe, IllegalStorePath (I mean, BadStorePath is /fine/), and maybe NonResidentStorePath? (that's kinda long though). Missing? Unavailable? PathNotFound?

midnightveil self-assigned this 2024-05-18 22:33:46 +00:00

lix-bot commented

2024-06-03 09:24:04 +00:00

Member

This issue was mentioned on Gerrit on the following CLs:

commit message in cl/1161 ("Change error messages about 'invalid paths' to 'path does not exist'.")
commit message in cl/1160 ("Add a clearer error message for InvalidPathError during evaluation")
commit message in cl/1159 ("Harmonise the Store::queryPathInfoUncached interface")

This issue was mentioned on Gerrit on the following CLs: * commit message in [cl/1161](https://gerrit.lix.systems/c/lix/+/1161) ("Change error messages about 'invalid paths' to 'path does not exist'.") * commit message in [cl/1160](https://gerrit.lix.systems/c/lix/+/1160) ("Add a clearer error message for InvalidPathError during evaluation") * commit message in [cl/1159](https://gerrit.lix.systems/c/lix/+/1159) ("Harmonise the Store::queryPathInfoUncached interface")

lix-project referenced this issue from a commit

2024-06-16 04:29:33 +00:00

Harmonise the Store::queryPathInfoUncached interface

lix-project referenced this issue from a commit

2024-06-16 04:29:33 +00:00

Add a clearer error message for InvalidPathError during evaluation

lix-project referenced this issue from a commit

2024-06-16 04:29:33 +00:00

Change error messages about 'invalid paths' to 'path does not exist'.

lix-project closed this issue

2024-06-16 04:29:33 +00:00

"path (...) is not valid" is a highly non-descriptive error and doesn't say anything about what is corrupted if anything #270