Add a new Cmd type working on RealisedPaths #4372
Conversation
| /** | ||
| * A store path with all the history of how it went into the store | ||
| */ | ||
| struct RealisedPath { |
There was a problem hiding this comment.
Is all this C++ worth it? In derivation.hh we just wrapped a variant and called it a day. Other places we typedefed the variant. It would be good to be consistent whatever route we choose, I suppose.
There was a problem hiding this comment.
Yeah I eventually removed because it was a lot of boilerplate for nearly nothing
| /** | ||
| * A store path with all the history of how it went into the store | ||
| */ | ||
| struct RealisedPath { |
There was a problem hiding this comment.
On a separate note, I think it would be good to make a plan for reducing into fewer types this, StorePathWithOutputs, Buildable, and perhaps even the separation of inputsDrv vs inputSrcs. We've had too many similar data types for too long, but only recently have we been bringing them closer together.
thufschmitt
force-pushed
the
ca/drvoutputs-commands
branch
2 times, most recently
from
3e1d847
to
9fe4bf8
Compare
Where a `RealisedPath` is a store path with its history, meaning either an opaque path for stuff that has been directly added to the store, or a `Realisation` for stuff that has been built by a derivation This is a low-level refactoring that doesn't bring anything by itself (except a few dozen extra lines of code :/ ), but raising the abstraction level a bit is important on a number of levels: - Commands like `nix build` have to query for the realisations after the build is finished which is fragile (see 27905f1 for example). Having them oprate directly at the realisation level would avoid that - Others like `nix copy` currently operate directly on (built) store paths, but need a bit more information as they will need to register the realisations on the remote side
thufschmitt
force-pushed
the
ca/drvoutputs-commands
branch
from
9fe4bf8
to
9355ecd
Compare
src/libstore/realisation.hh
Outdated
| * } | ||
| * ``` | ||
| */ | ||
| #define GENERATE_ONE_CMP(COMPARATOR, MY_TYPE, FIELDS...) \ |
There was a problem hiding this comment.
Maybe move this into a separate file in libutil?
There was a problem hiding this comment.
OK, I guess that can be useful elsewhere.
Done in 3436874
src/libstore/realisation.hh
Outdated
|
|
||
| /** | ||
| * Syntactic sugar to run `std::visit` on the raw value: | ||
| * path.visit(blah) == std::visit(blah, path.raw) |
There was a problem hiding this comment.
TBH I wouldn't mind getting rid of the use of std::visit, which often seems unnecessarily verbose/complicated because of the use of closures compared to
if (auto realisation = variant.get_if<Realisation>)
do something
else if (auto realisation = variant.get_if<OpaquePath>)
do something else
There was a problem hiding this comment.
I don't know in the general case (it's indeed very verbose and easily hard to read, but at the same time the exhaustiveness check is a killer feature…). But in this precise case, there was only one single call to visit, so it's definitely not worth it indeed.
Fixed in 4f84b2c
There was a problem hiding this comment.
@regnat For some reason that commit isn't showing up in this PR, did you push it?
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Despite being an ugly hack, it can probably be useful in a couple extra places
In addition to being some ugly template trickery, it was also totally useless as it was used in only one place where I could replace it by just a few extra characters
Where a
RealisedPathis a store path with its history, meaning eitheran opaque path for stuff that has been directly added to the store, or a
Realisationfor stuff that has been built by a derivationThis is a low-level refactoring that doesn't bring anything by itself
(except a few dozen extra lines of code :/ ), but raising the
abstraction level a bit is important on a number of levels:
nix buildhave to query for the realisations after thebuild is finished which is fragile (see
27905f1 for example). Having them
oprate directly at the realisation level would avoid that
nix copycurrently operate directly on (built) storepaths, but need a bit more information as they will need to register
the realisations on the remote side