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 profile element names stable #9656

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

edolstra
Copy link
Member

Motivation

The profile manifest is now an object keyed on the name returned by getNameFromURL() at installation time, instead of an array. This ensures that the names of profile elements don't change when other elements are added/removed.

It also removes profile element indices entirely.

Followup to #8678.

Context

Priorities and Process

Add 👍 to pull requests you find important.

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

The profile manifest is now an object keyed on the name returned by
getNameFromURL() at installation time, instead of an array. This
ensures that the names of profile elements don't change when other
elements are added/removed.
@edolstra edolstra added the new-cli Relating to the "nix" command label Dec 22, 2023
@github-actions github-actions bot added documentation with-tests Issues related to testing. PRs with tests have some priority labels Dec 22, 2023
AFAIK, we've never emitted this attribute.
@bobvanderlinden
Copy link
Member

I still need give this a try, but from what I've seen so far it looks good.

This closes #7967 by introducing stable identifiers.
This closes #9171 by removing indexes.

Removing indexes is a backwards incompatible change for CLI.

Copy link
Member

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

I tried it. Upgrades the manifest.json upon making changes quite nicely.

One issue I ran into was the URL that was used for getNameFromURL. Instead of github:nixos/nixpkgs/nixpkgs-unstable#hello it now passes github:nixos/nixpkgs/nixpkgs-unstable#legacyPackages.x86_64-linux.hello. Unfortunately getNameFromURL doesn't support legacyPackages and there are no tests for this as well. The resulting entry is named legacyPackages.x86_64-linux.hello. Unrelated to this PR, but it did expose the problem for me. I've created edolstra#9 to support legacyPackages.

Apart from that, I didn't run into trouble with my profile/manifest. I do wonder whether the migration from manifest version 2 to 3 should have a test?

Btw, I'm on vacation for the next week, so expect my feedback after that.

@roberth
Copy link
Member

roberth commented Jan 5, 2024

This should have a release note indicating that it changes the profile format, and previous versions will not be able to parse it anymore.
Ideally the note includes the error message from the old nix and a manual solution.

@edolstra
Copy link
Member Author

Updated the release notes and added a migration test.

@edolstra edolstra merged commit 52f949b into NixOS:master Jan 12, 2024
8 checks passed
@edolstra edolstra deleted the nix-profile-stable-names branch January 27, 2024 16:19
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
Based off of commit 3187bc9

Upstream-PR: NixOS/nix#9656
Co-authored-by: Eelco Dolstra <[email protected]>
Change-Id: I8ac4a33314cd1cf9de95404c20f58e883460acc7
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
Based off of commit 6268a45

Upstream-PR: NixOS/nix#9656
Co-authored-by: Eelco Dolstra <[email protected]>
Change-Id: I0fcf069a8537c61ad6fc4eee1f3c193a708ea1c4
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
(cherry picked from commit 72560f7)

Upstream-PR: NixOS/nix#9656
Change-Id: I405e5848e2627a76940220fb6aebadfb8f094afb
lf- pushed a commit to lix-project/lix that referenced this pull request May 2, 2024
It doesn't seem to have ever been used.

Based off of commit a748e88

Upstream-PR: NixOS/nix#9656
Change-Id: Idcf250a645fa43f2ef11fb15b503b070a62a917e
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
Based off of commit 3187bc9

Upstream-PR: NixOS/nix#9656
Co-authored-by: Eelco Dolstra <[email protected]>
Change-Id: I8ac4a33314cd1cf9de95404c20f58e883460acc7
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
Based off of commit 6268a45

Upstream-PR: NixOS/nix#9656
Co-authored-by: Eelco Dolstra <[email protected]>
Change-Id: I0fcf069a8537c61ad6fc4eee1f3c193a708ea1c4
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
(cherry picked from commit 72560f7)

Upstream-PR: NixOS/nix#9656
Change-Id: I405e5848e2627a76940220fb6aebadfb8f094afb
lf- pushed a commit to lix-project/lix that referenced this pull request May 4, 2024
It doesn't seem to have ever been used.

Based off of commit a748e88

Upstream-PR: NixOS/nix#9656
Change-Id: Idcf250a645fa43f2ef11fb15b503b070a62a917e
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.

4 participants