Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilise nix hash and subcommands #8876

Closed
2 tasks done
fricklerhandwerk opened this issue Aug 28, 2023 · 17 comments
Closed
2 tasks done

Stabilise nix hash and subcommands #8876

fricklerhandwerk opened this issue Aug 28, 2023 · 17 comments
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command RFC Related to an accepted RFC

Comments

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Aug 28, 2023

This implements NixOS/rfcs#136. This issue is agreed-upon by the https://github.com/orgs/NixOS/teams/nix-team.

Only leave these two commands, collapsing the existing ones into them:

  • nix hash convert

    
    nix hash convert [--hash-algo (sha256|md5|...)] --from base16|...|sri --to (base64|...) <strings>...
    
    • the current implementation makes --from optional, but we are not quite sure how that works. If it doesn't seem robust, we can make it mandatory instead.

    • --algo is optional but will error if it can't infer from SRI or guess

  • nix hash path:

    nix hash path --hash-algo (sha256|md5|...) --to (base16|...|sri) --mode nar|flat|git <path>
    
    • algo is mandatory, any default would be arbitrary
    • collapse modes to nar (serialise the directory as a NAR and hash that), flat (single file, error on directory), git (directory, according to RFC 133)
@fricklerhandwerk fricklerhandwerk added idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. RFC Related to an accepted RFC labels Aug 28, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-08-28-nix-team-meeting-minutes-83/32290/1

@roberth roberth added the new-cli Relating to the "nix" command label Aug 30, 2023
@thufschmitt thufschmitt added this to the CLI Stabilisation milestone Sep 4, 2023
@roberth
Copy link
Member

roberth commented Sep 5, 2023

This should also align with a builtin. Such a builtin has been proposed, but does not have a satisfactory design as of yet (?)

@Ericson2314
Copy link
Member

A comment in #4354 suggested we rename our base32 to avoid confusion. I missed that before but now support it — docs aren't good enough.

The new CLI is the perfect time to introduce the name change, since the choice of base is really CLI thing (the language and various file and wire formats do not contain the names of base to my knowledge).

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-09-25-nix-team-meeting-minutes-89/33489/1

@kolloch
Copy link
Contributor

kolloch commented Nov 25, 2023

A comment in #4354 suggested we rename our base32 to avoid confusion. I missed that before but now support it — docs aren't good enough.

The new CLI is the perfect time to introduce the name change, since the choice of base is really CLI thing (the language and various file and wire formats do not contain the names of base to my knowledge).

I am not passionate either way except that we should not break existing code and continue accepting "base32" where we currently accept it, maybe with the addition of a deprecation notice.

Has this been decided on by https://github.com/orgs/NixOS/teams/nix-team?

@kolloch
Copy link
Contributor

kolloch commented Nov 25, 2023

Currently, this is how the internal hash conversion support looks like:

struct Hash { // ...
     static Hash parseAny(std::string_view s, std::optional<HashType> type);
     // ...
}

In your proposal, you suggest making --type optional but --from obligatory (even though you allow the possibility of auto-detecting).

The current code does the following:

  1. If the input is an SRI hash, the hash type is contained in the string and accordingly used.
  2. If the input is not an SRI hash, the hash type is required and the hash format is deduced from the length of the input.

I think that this is reasonable. It disallows auto-detection of different formats that result in the same length. But even if we introduce new formats resulting in the same length (e.g. alternative base32 encodings), we could still default to the current formats for compatibility.

Therefore, I'd allow skipping --type and --from for SRI input hashes.

For non-SRI hashes, I'd make --type obligatory (as in the internal code currently) and --from optional. If --from is not given, the default format based on the length is assumed (as now).

It would be nice to get formal approval by the https://github.com/orgs/NixOS/teams/nix-team for this as well.

@kolloch
Copy link
Contributor

kolloch commented Nov 25, 2023

I would also make --to optional and default to --to sri.

Some examples with my suggestions implemented for nix hash convert:

bash-5.2$ nix hash convert "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc=
bash-5.2$ nix hash convert --to base32 "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
09ymwqf5i9q7d4dm7x4pjjcqqj0qrcp5lnznbh42gfsci5hcbqqm
bash-5.2$ nix hash convert --from base32 --to base32 "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
error: input hash 'sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc=' does not have the expected format '--from base32'
bash-5.2$ nix hash convert --to base32 "sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="
09ymwqf5i9q7d4dm7x4pjjcqqj0qrcp5lnznbh42gfsci5hcbqqm
bash-5.2$ nix hash convert 09ymwqf5i9q7d4dm7x4pjjcqqj0qrcp5lnznbh42gfsci5hcbqqm
error: hash '09ymwqf5i9q7d4dm7x4pjjcqqj0qrcp5lnznbh42gfsci5hcbqqm' does not include a type, nor is the type otherwise known from context
bash-5.2$ nix hash convert --type sha256 09ymwqf5i9q7d4dm7x4pjjcqqj0qrcp5lnznbh42gfsci5hcbqqm
sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc=

Tell me what you think!

kolloch added a commit to kolloch/nix that referenced this issue Nov 25, 2023
This deviated from the proposal! See comments on the issue.

NixOS#8876
@kolloch kolloch mentioned this issue Nov 25, 2023
5 tasks
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-11-27-nix-team-meeting-minutes-107/36112/1

kolloch added a commit to kolloch/nix that referenced this issue Nov 27, 2023
This deviated from the proposal! See comments on the issue.

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
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
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
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
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
...and also adjusted parsing accordingly.

Also added CLI completion for HashFormats.

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
(But not yet nix-hash since `nix hash` is still hidden behind a feature flag.)

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
This deviated from the proposal! See comments on the issue.

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Nov 28, 2023
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
kolloch added a commit to kolloch/nix that referenced this issue Dec 6, 2023
...and also adjusted parsing accordingly.

Also added CLI completion for HashFormats.

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Dec 6, 2023
(But not yet nix-hash since `nix hash` is still hidden behind a feature flag.)

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Dec 6, 2023
kolloch added a commit to kolloch/nix that referenced this issue Dec 6, 2023
"warning: you don'\''t have Internet access; disabling some network-dependent features" ...

NixOS#8876
kolloch added a commit to kolloch/nix that referenced this issue Dec 6, 2023
@Ericson2314
Copy link
Member

Ericson2314 commented Jan 19, 2024

We now have nix hash convert, but nix hash path doesn't yet do the right thing.

@cole-h
Copy link
Member

cole-h commented Jan 19, 2024

We now have nix hash convert, but nix hash path doesn't yet do the right thing.

For the onlookers / those otherwise interested in helping out, could you say more about what nix hash path does wrong / doesn't yet do? (And then maybe edit the stabilisation issue to uncheck the box next to nix hash and this issue until this has been resolved?)

EDIT: I was under the assumption that nix hash path was "fixed", but that was just nix hash convert and nix hash path hasn't been attempted at all yet. Sorry for the noise! 😅

@Ericson2314
Copy link
Member

I think nix hash path is untouched and nothing like what it says above. nix hash convert is close but in another PR --algo was disliked and --hash-algo was preferred, so we should make that consistent.

@Ericson2314
Copy link
Member

the --mode flags implementations are also too ad-hoc and thus likely to drift out of sync.

@kolloch
Copy link
Contributor

kolloch commented Jan 19, 2024

You could edit the issue text to indicate that nix hash convert is done except for the—hash-algo rename.

@Ericson2314
Copy link
Member

On the flip side, on could argue that --algo is fine for a nix hash command, whereas --hash-algo is needed for other commands.

In any event, #9815 is a draft PR for the rest of this, whatever we decide.

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 15, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 15, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 15, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 16, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 20, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
@cole-h
Copy link
Member

cole-h commented Feb 22, 2024

Now that #9815 has merged, shall we close this, or do we want to wait on #10021?

@thufschmitt
Copy link
Member

#10021 is a backwards-compatible addition, let's not be blocked on it

@Ericson2314
Copy link
Member

Yes #10021 is extending builtins.derivation, not touching the CLI at all, it is just separate, inspired by this.

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 23, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 28, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Feb 29, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Apr 3, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Apr 5, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Apr 9, 2024
Now that we have a few things identifying content address methods by
name, we should be consistent about it.

Move up the `parseHashAlgoOpt` for tidiness too.

Discussed this change for consistency's sake as part of NixOS#8876

Co-authored-by: Eelco Dolstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command RFC Related to an accepted RFC
Projects
Status: Done
Archived in project
Development

No branches or pull requests

7 participants