Factor out MemorySourceAccessor, implement missing features#9283
Factor out MemorySourceAccessor, implement missing features#9283roberth merged 1 commit intoNixOS:masterfrom
MemorySourceAccessor, implement missing features#9283Conversation
github-actions
bot
added
the
fetching
Ericson2314
force-pushed
the
mememory-source-accessor
branch
from
a2507d7 to
414927b
Compare
| struct MemoryInputAccessorImpl : MemoryInputAccessor | ||
| { | ||
| std::map<CanonPath, std::string> files; | ||
| MemorySourceAccessor a; |
There was a problem hiding this comment.
Can't we just make MemoryInputAccessorImpl a subclass of MemorySourceAccessor and InputAccessor? Then we don't need all these forwarding methods.
There was a problem hiding this comment.
@edolstra I we'll need to make the base SourceAccessors virtual but then yes we can.
The new `MemorySourceAccessor` rather than being a slightly lossy flat map is a complete in-memory model of file system objects. Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314
force-pushed
the
mememory-source-accessor
branch
from
c84aee5 to
9b880e3
Compare
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Needs symlink support for this to be a proper accessor. If that's not a priority, the flaw can be documented while this is moved into tests where you want to use it.
| return r->contents; | ||
| else | ||
| throw Error("file '%s' is not a regular file", path); | ||
| } |
There was a problem hiding this comment.
No symlink support.
Maybe the source accessor virtual methods should not even be responsible for that behavior. Or at least, symlink resolution should be implemented once, as a mix-in or an adapter.
(Also applies to other methods)
There was a problem hiding this comment.
Yeah I actually think this is right and its PosixSourceAccessor that needs to be fixed.
There was a problem hiding this comment.
#9306 partially fixes PosixSourceAccesor and documents this in the methods.
|
Triaged in Nix team meeting:
|
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
A suggestion; otherwise lgtm.
| * `MemorySourceAccessor`, this has a side benefit of nicely | ||
| * defining what a "file system object" is in Nix. | ||
| */ | ||
| struct File { |
There was a problem hiding this comment.
Perhaps:
| struct File { | |
| struct FileSystemObject { |
I don't like the idea that a directory would be a file.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
5 participants
Motivation
The new
MemorySourceAccessorrather than being a slightly lossy flat map is a complete in-memory model of file system objects.Context
I need this to write more unit tests for #8918
Priorities
Add 👍 to pull requests you find important.