pkg/nixpath: move to pkg/storepath, make FromString receive non-absolute paths #113
Conversation
flokli
force-pushed
the
nixpath-relative
branch
from
cd9135b
to
da64b2e
Compare
The whole module layout was a bit messy - store paths had to be passed in primarily with the store dir prefix, even though it's not possible to have a different prefix, at least with the way it's currently structured. There were Absolute() helper functions which helped with concatenation of strings, but no actual validation, which also were not really used outside of tests. The comments were also wrong, as no cleaning of store paths actually did happen. Some library code (like pkg/derivation) even started implementing their own String() methods. Clean this up a bit - have NixPath's FromString() receive a non-absolute path, and have an additional FromAbsolutePath() method that can be used if you have an absolute path. Move from nixpath.Absolute() to a Absolute() function defined on a specific NixPath struct. This moves things closer to how they look in the corresponding `nix_compat::store_path::StorePath` rust codebase.
flokli
force-pushed
the
nixpath-relative
branch
from
da64b2e
to
7719bd6
Compare
pkg/nixpath/nixpath.go
Outdated
| // String returns a NixPath without StoreDir. | ||
| // It starts with a digest (20 bytes), nixbase32-encoded, | ||
| // followed by a `-`, and ends with the name. | ||
| func (n *NixPath) String() string { | ||
| return Absolute(nixbase32.EncodeToString(n.Digest) + "-" + n.Name) | ||
| return nixbase32.EncodeToString(n.Digest) + "-" + n.Name | ||
| } |
There was a problem hiding this comment.
This breaks back-compat; some users might assume that they will get an absolute path here.
Maybe the best is to rename or remove this method?
There was a problem hiding this comment.
Yes, it does break back-compat, but it'd be quite quick to notice, and matches the behaviour in the nix-compat crate (where impl Display also shows the non-absolute string). Similarly, String() is what gets called if you %s things in a Printf statement, for example.
The repo is marked with a "STATUS: experimental", so while it is, breaking compat is fine.
This is easier said than done - we'd need to carry a pointer to the store dir used for individual store paths, and verify compatibility on every consumer side, because the store path would need to be compatible with whatever that consumer is doing with it. For now, it's a build-time constant. |
This prevents silently changing behaviour of the NixPath.String() function, which might cause some debugging for users of the library otherwise, and aligns the package name with how it's called in the nix- compat crate too.
|
As @zimbatm mentioned, I renamed the package to It's not perfect, but better than silently returning different strings on a go mod update. |
flokli
changed the title
nixpath.FromString -> storepath.FromAbsolutePath. See nix-community/go-nix#113 for details. Closes: https://b.tvl.fyi/issues/314 Change-Id: I25277fb6006cbbb2a323ffb5809a1be500822a97 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9551 Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: Connor Brewster <cbrewster@hey.com>
The whole module layout was a bit messy - store paths had to be passed in primarily with the store dir prefix, even though it's not possible to have a different prefix, at least with the way it's currently structured.
There were Absolute() helper functions which helped with concatenation of strings, but no actual validation, which also were not really used outside of tests. The comments were also wrong, as no cleaning of store paths actually did happen.
Some library code (like pkg/derivation) even started implementing their own String() methods.
Clean this up a bit - have NixPath's FromString() receive a non-absolute path, and have an additional FromAbsolutePath() method that can be used if you have an absolute path.
Move from nixpath.Absolute() to a Absolute() function defined on a specific NixPath struct.
This moves things closer to how they look in the corresponding
nix_compat::store_path::StorePathrust codebase.