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

Make outputHashAlgo accept "nar", stay in sync #10021

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 15, 2024

Motivation

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.

Context

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

Priorities and Process

Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 15, 2024
@Ericson2314 Ericson2314 force-pushed the ca-type-names branch 2 times, most recently from c21f04d to 9d246ff Compare February 15, 2024 21:05
Comment on lines +1123 to +1144
if (s == "recursive") {
// back compat, new name is "nar"
ingestionMethod = FileIngestionMethod::Recursive;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we instead want to accept `recursive as a file ingestion method for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean, I think change ContentAddressMethod::parse so this one is accepted everywhere (e.g. the new CLI too)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget whether we discussed this before, but I do want to over time steer people to the new name (which is clearer) in the new CLI.

Copy link
Member Author

@Ericson2314 Ericson2314 Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also with git support, which I hope to merge very soon, NAR isn't the only "recursive" format anyways.)

@Ericson2314 Ericson2314 force-pushed the ca-type-names branch 2 times, most recently from b84755a to 1eb4c55 Compare February 20, 2024 18:37
@cole-h cole-h mentioned this pull request Feb 22, 2024
2 tasks
@thufschmitt thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Feb 28, 2024
@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting on 2024-02-28.
Approved, modulo the test failure and the merge conflicts

  • New aliases are annoying because cause backwards-compat issues
    • Acceptable trade of

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-02-28-nix-team-meeting-129/40499/1

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Apr 5, 2024
This was part of approved PR NixOS#10021. Unfortunately that one is stalled
on a peculiar Linux test timeout, so trying to get bits of it merged
first to bisect failure.
Ericson2314 added a commit that referenced this pull request Apr 5, 2024
This was part of approved PR #10021. Unfortunately that one is stalled
on a peculiar Linux test timeout, so trying to get bits of it merged
first to bisect failure.
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 <edolstra@gmail.com>
@edolstra edolstra merged commit 3fd8dfe into NixOS:master Apr 9, 2024
9 checks passed
@Ericson2314 Ericson2314 deleted the ca-type-names branch April 9, 2024 21:06
@roberth
Copy link
Member

roberth commented Apr 10, 2024

This would be a case where a release note becomes more valuable over time.
@Ericson2314 could you add one? (with a recommendation not to use it yet)

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. with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants