Add nix hash convert by kolloch · Pull Request #9452 · NixOS/nix

kolloch · 2023-11-25T16:42:24Z

Motivation

This adds the nix hash convert command as described in #8876.

This does slightly deviate from the proposal: --to defaults to sri.

Context

Open:

Better CLI help text with available options.
Update docs? Or which part is auto-generated which is not?
Clarify whether the base32 vs nix32 hash format name change should be part of this.
Verify that the approach to making "--from" AND "--type" AND "--to" optional depending on context is a good choice.
Verify that the "--from" parameter strategy is as desired. It is an assert rather than a format hint since it is redundant.

Priorities

Add 👍 to pull requests you find important.

fricklerhandwerk

Cool, thanks a lot for helping out! Would be great if you could add a release note. And ideally a deprecation warning on the existing command, but this can be done in a follow-up not to stall progress here if you're short on time.

How exactly does this deviate from the proposal though?

.gitignore

src/nix/hash.cc

tests/functional/hash.sh

tomberek · 2023-11-27T15:06:17Z

@kolloch , for your questions and from Nix team

Update docs? Or which part is auto-generated which is not?
- yes, and please add docs
Clarify whether the base32 vs nix32 hash format name change should be part of this.
- yes
Verify that the approach to making "--from" AND "--type" AND "--to" optional depending on context is a good choice.
- yes, if it can be unambiguous
- please consider changing "--type" into "--algo" (thoughts?), we updated the original issue
Verify that the "--from" parameter strategy is as desired. It is an assert rather than a format hint since it is redundant.
- yes
more tests are always wonderful to clarify expected behavior (eg: what was in the comment in the parent issue)

kolloch · 2023-11-28T18:06:38Z

I think I addressed all the comments.

In addition, I did rename HashType to HashAlgorithm so that the type matches the arguement names of the CLI and the builtin.

I did NOT yet remove the need to specify the nix-command experiment feature.

It would be nice to get this merged in the current form plus eventually needed bug fixes, of course. Meaning: it might still be worth it to add some more docs somewhere but I'd like to do future works in separate merge requests so this doesn't grow unboundedly.

Thank you!

kolloch · 2023-11-28T18:21:27Z

How exactly does this deviate from the proposal though?

The --to format parameter is optional and defaults to SRI.

kolloch · 2023-11-28T18:33:42Z

Ah, I noticed that my IDE reindented some lines close to some renames. I hope that's fine. It would be quite some pain to undo.

src/libexpr/primops.cc

kolloch · 2023-12-02T15:44:34Z

I rebase my changes on the latest master :)

kolloch · 2023-12-03T09:37:57Z

And some further fixes in code that wasn’t executed in make installcheck

kolloch · 2023-12-04T16:31:08Z

@tomberek anything else?

tomberek

A few binaries or unintended things snuck in. The vast majority is changes I presume the compiler is better at checking than I. Otherwise we're looking good.

src/libexpr/tests/libnixexpr-tests

doc/manual/src/release-notes/rl-next.md

kolloch · 2023-12-06T09:36:43Z

Thank you for checking and finding the added files, @tomberek!

I interactively rebased to make sure that the binary doesn't pollute the history.

Not sure what this is about and if I can do something about it:

https://github.com/NixOS/nix/actions/runs/7112656418/job/19363023090?pr=9452

This deviated from the proposal! See comments on the issue. NixOS#8876

NixOS#8876

To be consistent with CLI, nix API and many other references. As part of this, we also converted it to a scoped enum. NixOS#8876

NixOS#8876

...and also adjusted parsing accordingly. Also added CLI completion for HashFormats. NixOS#8876

(But not yet nix-hash since `nix hash` is still hidden behind a feature flag.) NixOS#8876

NixOS#8876

Co-authored-by: Théophane Hufschmitt <[email protected]>

NixOS#8876

"warning: you don'\''t have Internet access; disabling some network-dependent features" ... NixOS#8876

NixOS#8876

kolloch · 2023-12-06T22:45:09Z

rebased on latest master

kolloch · 2023-12-06T22:46:40Z

Somehow it still shows I need to address some review comments but I don't see which...

Ericson2314 · 2023-12-07T02:10:44Z

Sorry I dropped the ball on this. @tomberek thank you for picking up after me.

kolloch · 2023-12-07T11:45:36Z

Hurray! Thanks @tomberek

kolloch requested a review from edolstra as a code owner November 25, 2023 16:42

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

fricklerhandwerk reviewed Nov 27, 2023

View reviewed changes

.gitignore Show resolved Hide resolved

src/nix/hash.cc Outdated Show resolved Hide resolved

src/nix/hash.cc Outdated Show resolved Hide resolved

tests/functional/hash.sh Outdated Show resolved Hide resolved

kolloch force-pushed the feature/nix-hash-convert branch from da8928a to 1616f0e Compare November 27, 2023 18:38

kolloch requested review from roberth and thufschmitt as code owners November 28, 2023 13:20

github-actions bot added store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Nov 28, 2023

kolloch force-pushed the feature/nix-hash-convert branch from fc7c6c0 to e009c33 Compare November 28, 2023 13:27

kolloch changed the title ~~WIP: Add nix hash convert~~ Add nix hash convert Nov 28, 2023

kolloch force-pushed the feature/nix-hash-convert branch from b13d967 to 8935e7a Compare November 28, 2023 18:18

thufschmitt assigned Ericson2314 Dec 1, 2023

thufschmitt reviewed Dec 1, 2023

View reviewed changes

src/libexpr/primops.cc Show resolved Hide resolved

kolloch force-pushed the feature/nix-hash-convert branch from 068f565 to 754f20f Compare December 2, 2023 15:38

tomberek requested changes Dec 4, 2023

View reviewed changes

src/libexpr/tests/libnixexpr-tests Outdated Show resolved Hide resolved

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

kolloch force-pushed the feature/nix-hash-convert branch from f343d26 to bde30ed Compare December 6, 2023 09:31

kolloch requested a review from tomberek December 6, 2023 09:56

kolloch force-pushed the feature/nix-hash-convert branch from bde30ed to 9c6c4ca Compare December 6, 2023 10:00

kolloch added 3 commits December 6, 2023 23:41

hash.cc/hash.h: Minor C++ improvements

e7abf60

CmdHashBase: doc comment

156ea78

nix hash convert: added

6bbd900

This deviated from the proposal! See comments on the issue. NixOS#8876

kolloch and others added 10 commits December 6, 2023 23:43

nix hash convert: s/--type/--algo/ + more functional tests

0c2d5f7

NixOS#8876

HashType: Rename to HashAlgorithm

5334c9c

To be consistent with CLI, nix API and many other references. As part of this, we also converted it to a scoped enum. NixOS#8876

Further HashType renaming + using mkHashAlgoOptFlag for new conversion

837b889

NixOS#8876

Renamed HashFormat::Base32 to HashFormat::Nix32

fc6f290

...and also adjusted parsing accordingly. Also added CLI completion for HashFormats. NixOS#8876

Add deprecation notice for old nix hash conversion subcommands.

7ff876b

(But not yet nix-hash since `nix hash` is still hidden behind a feature flag.) NixOS#8876

Add docs/rl-notes for nix hash convert / builtins.convertHash

8afeaf0

NixOS#8876

Update src/libexpr/primops.cc

d38ec12

Co-authored-by: Théophane Hufschmitt <[email protected]>

Refactor concurrently added tests to use HashAlgorithm.

bbba205

NixOS#8876

hash.sh: Make failure tests more tolerant of additional output

e9a5365

"warning: you don'\''t have Internet access; disabling some network-dependent features" ... NixOS#8876

Store.xs: fix references to HashFormat::Nix32

9a1a3c4

NixOS#8876

kolloch force-pushed the feature/nix-hash-convert branch from 9c6c4ca to 9a1a3c4 Compare December 6, 2023 22:43

tomberek approved these changes Dec 7, 2023

View reviewed changes

tomberek merged commit 82449a4 into NixOS:master Dec 7, 2023

Ericson2314 mentioned this pull request Dec 7, 2023

Merge master into lazy trees edolstra/nix#8

Merged

tomberek mentioned this pull request Dec 18, 2023

CLI stabilization effort #7701

Open

86 tasks

thufschmitt added this to the CLI Stabilisation milestone Dec 20, 2023

ShamrockLee mentioned this pull request Feb 26, 2024

treewide: hash type -> hash algorithm #10086

Merged

Frontear mentioned this pull request Aug 24, 2025

treewide: migrate usages of nix hash to nix-hash NixOS/nixpkgs#436570

Draft

13 tasks

Uh oh!

Conversation

kolloch commented Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Priorities

Uh oh!

fricklerhandwerk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomberek commented Nov 27, 2023

Uh oh!

kolloch commented Nov 28, 2023

Uh oh!

kolloch commented Nov 28, 2023

Uh oh!

kolloch commented Nov 28, 2023

Uh oh!

Uh oh!

kolloch commented Dec 2, 2023

Uh oh!

kolloch commented Dec 3, 2023

Uh oh!

kolloch commented Dec 4, 2023

Uh oh!

tomberek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolloch commented Dec 6, 2023

Uh oh!

kolloch commented Dec 6, 2023

Uh oh!

kolloch commented Dec 6, 2023

Uh oh!

Ericson2314 commented Dec 7, 2023

Uh oh!

kolloch commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolloch commented Nov 25, 2023 •

edited

Loading

fricklerhandwerk left a comment •

edited

Loading