Use the fs accessor for readInvalidDerivation #4366
Conversation
Extend `FSAccessor::readFile` to allow not checking that the path is a valid one, and rewrite `readInvalidDerivation` using this extended `readFile`. Several places in the code use `readInvalidDerivation`, either because they need to read a derivation that has been written in the store but not registered yet, or more generally to prevent a deadlock because `readDerivation` tries to lock the state, so can't be called from a place where the lock is already held. However, `readInvalidDerivation` implicitely assumes that the store is a `LocalFSStore`, which isn't always the case. The concrete motivation for this is that it's required for `nix copy --from someBinaryCache` to work, which is tremendously useful for the tests.
| @@ -61,9 +61,9 @@ struct LocalStoreAccessor : public FSAccessor | |||
| return res; | |||
| } | |||
|
|
|||
| std::string readFile(const Path & path) override | |||
| std::string readFile(const Path & path, bool requireValidPath = true) override | |||
There was a problem hiding this comment.
I think the overrides shouldn't have the default args? Elsewhere in the codebase they at least are only part of the declarations, IIRC.
There was a problem hiding this comment.
That's a good question. Triggered me to read https://stackoverflow.com/questions/6464404/virtual-function-default-arguments-behaviour which is… interesting.
The gist of it seems to that if we only call accessor.readFile(path) where accessor is declared of type FSAccessor (which is probably the case) then we don't need the default arguments for the overrides. But that looks quite fragile. Maybe I could just split the interface as:
virtual bool isValidPath(const Path& path);
virtual std::string readFileUnchecked(const Path& path);
std::string readFile(const Path& path) {
if (!isValidPath(path)) throw InvalidPath(...);
return readFileUnchecked(path);
}There was a problem hiding this comment.
Hmm I read it the other way around, where up casting certainly gives you the outer default arg, but no upcasting would also give you the outer default arg if there is no inner default arg.
There was a problem hiding this comment.
Nah, just checked:
struct Foo {
virtual void foo(int n = 1) { }
};
struct Bar : Foo {
void foo(int n) override { }
};
int main() {
Foo x;
x.foo();
Bar y;
y.foo();
}x.foo() typechecks fine, but y.foo() doesn't
There was a problem hiding this comment.
Sorry to have misled you with that wrong interpretation then.
Extend
FSAccessor::readFileto allow not checking that the path is avalid one, and rewrite
readInvalidDerivationusing this extendedreadFile.Several places in the code use
readInvalidDerivation, either becausethey need to read a derivation that has been written in the store but
not registered yet, or more generally to prevent a deadlock because
readDerivationtries to lock the state, so can't be called from aplace where the lock is already held.
However,
readInvalidDerivationimplicitely assumes that the store is aLocalFSStore, which isn't always the case.The concrete motivation for this is that it's required for
nix copy --from someBinaryCacheto work, which is tremendously useful for thetests.