Skip to content

More store object info json cleanup#14502

Merged
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:more-store-object-info-json-cleanup
Nov 10, 2025
Merged

More store object info json cleanup#14502
Ericson2314 merged 2 commits intoNixOS:masterfrom
obsidiansystems:more-store-object-info-json-cleanup

Conversation

@Ericson2314
Copy link
Member

Motivation

See each commit for details.

Context

JSON cleanup #13570


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

It turns out this code path is only used for unit tests (to ensure our
JSON formats are possible to parse by other code, elsewhere). No
user-facing functionality consumes this format.

Therefore, let's drop the old version parsing support.
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner November 7, 2025 00:32
@Ericson2314 Ericson2314 force-pushed the more-store-object-info-json-cleanup branch from 1ba0fa2 to 04ce7b3 Compare November 7, 2025 00:43
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Nov 7, 2025
Since we haven't released v2 yet (2.32 has v1) we can just update this
in-place and avoid version churn.

Note that as a nice side effect of using the standard `Hash` JSON impl,
we don't neeed this `hashFormat` parameter anymore.
@Ericson2314 Ericson2314 force-pushed the more-store-object-info-json-cleanup branch from 04ce7b3 to 4f1c8f6 Compare November 7, 2025 04:29
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 7, 2025
@edolstra
Copy link
Member

edolstra commented Nov 7, 2025

FWIW, Determinate Nix has adopted the following format for serializing hashes in a structured way:

{"algo": "sha256", "base16": "abcdef..."}

https://github.com/DeterminateSystems/nix-src/blob/d2142024ca3688e0c62c5b7d567eca14b482318c/src/libutil/hash.cc#L495-L501

It makes implementations simpler if we have only one hash encoding format (i.e. base-16) in JSON.

@Ericson2314
Copy link
Member Author

We can perhaps change our format. (Though do note that that was in a different PR much before this one, this one is just using the pre-existing format.)

It may help to know that my intent was not to support multiple hash formats --- mine always outputs base-64 unconditionally --- but make sure that we only support one hash format at a time.

For example, yours will parse

{
  "algo": "sha256",
  "base16": "abcdef...",
}

and might someday parse

{
  "algo": "sha256",
  "base64": "FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
}

but if given the union of things it parses at that point:

{
  "algo": "sha256",
  "base16": "abcdef...",
  "base64": "FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
}

but what does it do in that situation? Parse both and make sure they match?

mine will parse both

{
  "algorithm": "sha256",
  "format": "base64",
  "hash": "FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
}

and

{
  "algorithm": "sha256",
  "format": "base16",
  "hash": "abcdef..."
}

(though I wouldn't mind doing some anti-"Postel's law" and just refusing to parse format != baseWhateverWeChoose.)

but even if it does parse both, there is no union: the schema makes very clear that one can only pass one hash at a time.

@edolstra
Copy link
Member

The field name base16 is not intended to denote that there will ever be another supported encoding. It could also be named hash. But calling it base16 makes it self-documenting.

@Ericson2314
Copy link
Member Author

(Note that the hash instance was added in #14307)

@Ericson2314
Copy link
Member Author

Ericson2314 commented Nov 10, 2025

Per #14532 I think this PR is unblocked, because this PR is about using the hash format for JSON, not making it. After the issue is fixed (before next release) all this JSON will be regenerated accordingly, but not of the C++ this PR touches will need to be touched.

@Ericson2314 Ericson2314 added this pull request to the merge queue Nov 10, 2025
Merged via the queue into NixOS:master with commit 68a5110 Nov 10, 2025
16 checks passed
@Ericson2314 Ericson2314 deleted the more-store-object-info-json-cleanup branch November 10, 2025 21:14
@edolstra edolstra mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants