Backport SourcePath from the lazy-trees branch#8172
Conversation
This introduces the SourcePath type from lazy-trees as an abstraction for accessing files from inputs that may not be materialized in the real filesystem (e.g. Git repositories). Currently, however, it's just a wrapper around CanonPath, so it shouldn't change any behaviour. (On lazy-trees, SourcePath is a <InputAccessor, CanonPath> tuple.)
github-actions
bot
added
the
with-tests
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Partial review.
Mostly questions to be addressed by doc comments.
| /** | ||
| * Copy this `SourcePath` to the Nix store. | ||
| */ | ||
| StorePath fetchToStore( |
There was a problem hiding this comment.
I'd call this addToStore. Fetching only makes sense for some StorePaths, and currently none.
There was a problem hiding this comment.
addToStore is already pretty overloaded. fetchToStore makes clear that this involves libfetcher.
| std::string to_string() const | ||
| { return path.abs(); } |
There was a problem hiding this comment.
What is the purpose of the string?
What's the syntax of the returned string?
Does it need to be deterministic?
There was a problem hiding this comment.
On the lazy-trees branch, it represents a human-readable representation of the path, such as «github:NixOS/nixpkgs/bla:foo/bar.nix», intended in error messages etc. So it doesn't need to be deterministic, but it currently is.
|
|
||
| /** | ||
| * Return the location of this path in the "real" filesystem, if | ||
| * it has a physical location. |
There was a problem hiding this comment.
All source paths can be given a physical location.
What are valid reasons not to return one?
Should this guarantee accurate file contents at that path?
What are valid use cases for this method?
There was a problem hiding this comment.
Well, all source paths can be written to the filesystem, but for some fetchers this could be expensive. If the caller needs a physical path they can use fetchToStore().
This is currently used for nix-instantiate --find-file, nix edit and nix develop (where we can't chdir into a flake that doesn't exist in the filesystem).
github-actions
bot
added
fetching
Ericson2314
changed the title
SourcePath from the lazy-trees branch
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Co-authored-by: Robert Hensing <[email protected]>
|
This causes a regression where |
4 participants
Motivation
This introduces the
SourcePathtype from lazy-trees as an abstraction for accessing files from inputs that may not be materialized in the real filesystem (e.g. Git repositories). Currently, however, it's just a wrapper aroundCanonPath, so it shouldn't change any behaviour. (On lazy-trees,SourcePathis a<InputAccessor, CanonPath>tuple.)The reason for this PR is to make the
lazy-treesdiff smaller and make it easier to keep that branch in sync.Context
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/teststests/nixos/*Priorities
Add 👍 to pull requests you find important.