Make nix path-info --json return an object not array
#9242
Conversation
Ericson2314
requested review from
thufschmitt,
fricklerhandwerk and
edolstra
as code owners
github-actions
bot
added
documentation
with-tests
| @@ -125,4 +125,25 @@ std::string NarInfo::to_string(const Store & store) const | |||
| return res; | |||
| } | |||
|
|
|||
| nlohmann::json NarInfo::toJSON( | |||
There was a problem hiding this comment.
Is this method used anywhere?
There was a problem hiding this comment.
Ah right, I was going to write a round-trip unit test but didn't yet :)
There was a problem hiding this comment.
It's all nice to have it tested, but it's still dead code, isn't it? 🙃
There was a problem hiding this comment.
Yes, but I think it is still worth it. This is the one way to test that the JSON format covers all the fields --- and it's how I discovered the compression field was missing.
There was a problem hiding this comment.
(I have a few ways in mind I think we might eventually use this, but I would have that as part of the motivation. Future usability is indeed not a good enough reason to add dead code.)
There was a problem hiding this comment.
If we were to make a strict rule out of this, we'd have to move it into the test directory, but that just makes it harder to eventually use it. I think writing these is a good practice.
Ericson2314
force-pushed
the
path-info-map
branch
from
e85635e
to
887b433
Compare
|
@thufschmitt Thanks for the review. I think I did all the things you asked for, and I also split it into two commits so refactors come before behavior changes. |
Ericson2314
force-pushed
the
path-info-map
branch
5 times, most recently
from
2038166
to
b74cee4
Compare
We will need these for tests.
Ericson2314
force-pushed
the
path-info-map
branch
from
b74cee4
to
4ae1c91
Compare
`Store::pathInfoToJSON` was a rather baroque functions, being full of parameters to support both parsed derivations and `nix path-info`. The common core of each, a simple `dValidPathInfo::toJSON` function, is factored out, but the rest of the logic is just duplicated and then specialized to its use-case (at which point it is no longer that duplicated). This keeps the human oriented CLI logic (which is currently unstable) and the core domain logic (export reference graphs with structured attrs, which is stable), separate, which I think is better.
It was forgotten before.
Before it returned a list of JSON objects with store object information, including the path in each object. Now, it maps the paths to JSON objects with the metadata sans path. This matches how `nix derivation show` works. Quite hillariously, none of our existing functional tests caught this change to `path-info --json` though they did use it. So just new functional tests need to be added.
Ericson2314
force-pushed
the
path-info-map
branch
from
4ae1c91
to
cc46ea1
Compare
|
|
||
| jsonObject["closureSize"] = getStoreObjectsTotalSize(store, closure); | ||
|
|
||
| if (auto * narInfo = dynamic_cast<const NarInfo *>(&*info)) { |
There was a problem hiding this comment.
src/nix/path-info.cc:57:28: warning: unused variable ‘narInfo’ [-Wunused-variable]
57 | if (auto * narInfo = dynamic_cast<const NarInfo *>(&*info)) {
| ^~~~~~~
Was something lost along the way, or is narInfo truly redundant? Or is the whole conditional redundant?
There was a problem hiding this comment.
computeFSClosure is transitive-reflexive I think so we do not need narInfo (auto & p : closure below will get us what we need).
However we do still need the conditional because the invariant is "the transitive references are only expected to have NarInfo if the root has NarInfo".
There was a problem hiding this comment.
(The original code would just skip store objects without a NarInfo, making it possible to get a bogus closureDownloadSize silently if an internal invariant was broken.)
Motivation
Before it returned a list of JSON objects with store object information, including the path in each object. Now, it maps the paths to JSON objects with the metadata sans path.
This matches how
nix derivation showworks.Context
In the process of doing this, shuffle around the
ValidPathInfoJSON rendering logic into what I hope is a better state:Store::pathInfoToJSONwas a rather baroque functions, being full of parameters to support both parsed derivations andnix path-info. The common core of each, a simpleUnkeyedValidPathInfo::toJSONfunction, is factored out, but the rest of the logic is just duplicated and then specialized to its use-case (at which point it is no longer that duplicated).This keeps the human oriented CLI logic (which is currently stable) and the core domain logic (export reference graphs with structured attrs, which is stable), separate, which I think is better.
Quite hilariously, none of our existing tests caught this change to
path-info --jsonthough they did use it. So just new tests need to be added.Priorities
Add 👍 to pull requests you find important.