`nix hash path`, text hashing for `nix store add`, and preparatory refactors #9815

Ericson2314 · 2024-01-20T02:22:48Z

Motivation

Finish #8876

Context

nix store add supports text hashing

With functional test ensuring it matches builtins.toFile.
Factored-out flags for both
Move all common reusable flags to libcmd
- They are not part of the definition of the CLI infra, just a usage
  of it.
- The new libstore flag couldn't go in args.hh in libutil anyways,
  would be awkward for it to live alone separate from the others
Shuffle around Cmd* hierarchy so flags for deprecated commands don't end up on the new ones

Priorities and Process

Add 👍 to pull requests you find important.

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

doc/manual/rl-next/nix-store-add.md

@cole-h

Thanks @cole-h for finding in NixOS#9815 (comment)

src/libcmd/misc-store-flags.cc

src/libcmd/misc-store-flags.hh

src/libstore/content-address.cc

src/libstore/content-address.hh

src/libstore/derivations.cc

src/libutil/args.cc

src/libutil/args.hh

thufschmitt · 2024-01-26T09:03:39Z

Discussed during the Nix maintainers meeting on 2024-01-22.
Approved, provided it's made explicit that it's an advanced thing.

- @edolstra: Is exposing `text` really good? - @Ericson2314: If it exists, I think we should exposed it even if it has some warnings/deprecations/etc. - @roberth: Document that it is not needed for users. Recommend that they just use `flat`. It needs to be kept around for the Nix language.

nixos-discourse · 2024-01-26T09:06:18Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-22-nix-team-meeting-minutes-117/38838/1

Ericson2314 · 2024-01-27T03:11:55Z

I updated the docs to make more clear that text is advanced users only. If would be nice if someone could approve this! :)

nixos-discourse · 2024-02-02T11:32:47Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-30-nix-team-meeting-minutes-119/39185/1

thufschmitt · 2024-02-13T11:53:45Z

I'm afraid this is way too many unrelated things for me to review at once. Please split that so that it's possible to make sense of the various parts

Ericson2314 · 2024-02-13T19:43:53Z

Sigh, I was hoping the comments I left would ameliorate that. I have no split off prep PRs and gotten them merged, so I hope this is small enough.

src/libcmd/misc-store-flags.cc

thufschmitt

This is a mixed bag of several things, trying to split them up here:

I don't really get the point of moving the hash arguments to libcmd
The addition of text hashing is good, although it's weird to have both fileIngestionMethod and contentAddressMethod (at least that would require some explanatory comment if there's really a reason to keep them both)
The proper split of nix hash path and nix hash file is good too, with the caveat that it should be done more strictly (can be a follow-up) like giving them a proper documentation, and restricting nix hash file to only files

src/libcmd/misc-store-flags.cc

src/libcmd/misc-store-flags.hh

thufschmitt · 2024-02-20T10:08:08Z

src/nix/hash.cc

+        addFlag({
+            .longName = "sri",
+            .description = "Print the hash in SRI format.",
+            .handler = {&hashFormat, HashFormat::SRI},
+        });
+
+        addFlag({
+            .longName = "base64",
+            .description = "Print the hash in base-64 format.",
+            .handler = {&hashFormat, HashFormat::Base64},
+        });
+
+        addFlag({
+            .longName = "base32",
+            .description = "Print the hash in base-32 (Nix-specific) format.",
+            .handler = {&hashFormat, HashFormat::Nix32},
+        });
+
+        addFlag({
+            .longName = "base16",
+            .description = "Print the hash in base-16 format.",
+            .handler = {&hashFormat, HashFormat::Base16},
+        });
+
+        addFlag(flag::hashAlgo("type", &hashAlgo));


If they are deprecated, I think they should be marked as such

src/nix/hash.cc

thufschmitt · 2024-02-20T10:14:49Z

src/nix/hash.cc

+        addFlag({
+            .longName = "sri",
+            .description = "Print the hash in SRI format.",
+            .handler = {&hashFormat, HashFormat::SRI},
+        });
+
+        addFlag({
+            .longName = "base64",
+            .description = "Print the hash in base-64 format.",
+            .handler = {&hashFormat, HashFormat::Base64},
+        });
+
+        addFlag({
+            .longName = "base32",
+            .description = "Print the hash in base-32 (Nix-specific) format.",
+            .handler = {&hashFormat, HashFormat::Nix32},
+        });
+
+        addFlag({
+            .longName = "base16",
+            .description = "Print the hash in base-16 format.",
+            .handler = {&hashFormat, HashFormat::Base16},
+        });
+
+        addFlag(flag::hashAlgo("type", &hashAlgo));


I'm not clear why these are kept for nix hash file and not nix hash path. Both commands accept them similarly right now, so we should either drop them for both, or (preferably) keep them for both. But keeping them for only one seems weird and inconsistent. Or am I missing something?

Ericson2314 · 2024-02-20T14:20:43Z

I don't really get the point of moving the hash arguments to libcmd

Just checking, did you read what I wrote in the PR description? I am happy to elaborate if it still doesn't make sense.

- `nix store add` supports text hashing With functional test ensuring it matches `builtins.toFile`. - Factored-out flags for both commands - Move all common reusable flags to `libcmd` - They are not part of the *definition* of the CLI infra, just a usag of it. - The `libstore` flag couldn't go in `args.hh` in libutil anyways, would be awkward for it to live alone - Shuffle around `Cmd*` hierarchy so flags for deprecated commands don't end up on the new ones

thufschmitt · 2024-02-21T17:34:03Z

Just checking, did you read what I wrote in the PR description? I am happy to elaborate if it still doesn't make sense.

It makes sense, it just feels like a very heavy hammer for the small change

Ericson2314 · 2024-02-22T15:04:33Z

@thufschmitt is #9815 (comment) satisfactory? Are there any other outstanding issues?

thufschmitt · 2024-02-22T16:15:31Z

@thufschmitt is #9815 (comment) satisfactory? Are there any other outstanding issues?

Nah, looks good. Let's merge!

Ericson2314 · 2024-02-22T16:50:14Z

Thank you!!

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Jan 20, 2024

Ericson2314 force-pushed the nix-hash-path branch from b7507ec to 31bdead Compare January 20, 2024 04:13

Ericson2314 mentioned this pull request Jan 20, 2024

Stabilise nix hash and subcommands #8876

Closed

2 tasks

cole-h reviewed Jan 20, 2024

View reviewed changes

doc/manual/rl-next/nix-store-add.md Outdated Show resolved Hide resolved

Ericson2314 force-pushed the nix-hash-path branch from 31bdead to ed22314 Compare January 20, 2024 22:06

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jan 20, 2024

Fix typo in upcomming release notes

65294fe

Thanks @cole-h for finding in NixOS#9815 (comment)

Ericson2314 mentioned this pull request Jan 20, 2024

Fix typo in upcomming release notes #9821

Merged

Ericson2314 force-pushed the nix-hash-path branch from ed22314 to ddb5764 Compare January 21, 2024 03:05

github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 21, 2024

Ericson2314 marked this pull request as ready for review January 21, 2024 03:05

Ericson2314 requested review from thufschmitt and edolstra as code owners January 21, 2024 03:05

Ericson2314 changed the title ~~nix hash path~~ nix hash path, and preperatory refactors Jan 21, 2024

Ericson2314 changed the title ~~nix hash path, and preperatory refactors~~ nix hash path, text hasing for nix store add, and preperatory refactors Jan 21, 2024

Ericson2314 mentioned this pull request Jan 21, 2024

Git object hashing in libstore #8918

Merged

8 tasks

Ericson2314 force-pushed the nix-hash-path branch 2 times, most recently from 19095bd to 61e7690 Compare January 22, 2024 15:17