-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Factor out MemorySourceAccessor
, implement missing features
#9283
Conversation
a2507d7
to
414927b
Compare
|
||
namespace nix { | ||
|
||
struct MemoryInputAccessorImpl : MemoryInputAccessor | ||
{ | ||
std::map<CanonPath, std::string> files; | ||
MemorySourceAccessor a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edolstra I we'll need to make the base SourceAccessors
virtual but then yes we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK it's done!
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 <edolstra@gmail.com>
c84aee5
to
9b880e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I actually think this is right and its PosixSourceAccessor
that needs to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#9306 partially fixes PosixSourceAccesor
and documents this in the methods.
Triaged in Nix team meeting:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: https://discourse.nixos.org/t/2023-11-06-nix-team-meeting-minutes-101/35247/1 |
Motivation
The new
MemorySourceAccessor
rather 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.