Skip to content

Conversation

@illdefined
Copy link
Contributor

@illdefined illdefined commented Mar 5, 2025

This permits usage of content‐addressed derivations and has the added benefit of checking normalised paths.

We could as an alternative introduce a function to check if a string is a store path prefix that works directly on strings, without producing a temporary path.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Mar 5, 2025
@illdefined illdefined requested a review from roberth March 5, 2025 14:14
@illdefined illdefined force-pushed the types-path-with branch 4 times, most recently from a1fb950 to 153cc45 Compare March 13, 2025 12:39
@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 13, 2025
@illdefined illdefined closed this Mar 13, 2025
@illdefined illdefined reopened this Mar 13, 2025
@illdefined illdefined marked this pull request as ready for review March 13, 2025 16:47
@nix-owners nix-owners bot requested review from hsjobeki and infinisil March 13, 2025 16:48
@pillowtrucker
Copy link
Contributor

pillowtrucker commented Mar 21, 2025

the builds on contentAddressedByDefault are still broken despite the other two PRs being merged

@illdefined
Copy link
Contributor Author

the builds on contentAddressedByDefault are still broken despite the other two PRs being merged

It is still blocked by at least this PR getting merged. So far I have opened an individual PR for every subsystem I touch, but perhaps it would be better to collect them all in a single PR.

@pillowtrucker
Copy link
Contributor

I'm merging into my own fork to try to update my system so I'll let you know if it worked at least

@pillowtrucker
Copy link
Contributor

it appears so, but it's going to take a while for it to churn all the packages and I have to leave my computer for a while

@pillowtrucker
Copy link
Contributor

yeah this removes the error on my machine. At this point I don't think it would be very useful to refactor the PRs, since two have already been in master for weeks 🤷
It would be nice if contentAddressedByDefault was actually in the test suite.. but that's not the place to discuss that I guess

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@pillowtrucker
Copy link
Contributor

@illdefined I'm sorry but I don't know how to resolve a conflict in someone else's pr branch, maybe I don't have the permissions, I've done a quick fix on my branch tracking master with this fix in it https://github.com/pillowtrucker/nixpkgs/tree/fix-ca-dervs

This permits usage of content‐addressed derivations and has the added
benefit of checking normalised paths.
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@pillowtrucker
Copy link
Contributor

thanks

@illdefined
Copy link
Contributor Author

Is it acceptable to check the path with hasStorePathPrefix, which involves converting the option value to a path, or should I try to find an alternative solution that only operates on strings?

@pillowtrucker
Copy link
Contributor

@robeth @infinisil @hsjobeki unless there are issues with the style, could someone please OK this? I've built several generations of a full desktop system with this and it seems fine.

Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

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

Is it acceptable to check the path with hasStorePathPrefix, which involves converting the option value to a path, or should I try to find an alternative solution that only operates on strings?

Hm, this could be a small regression. Using discardStringContext now would discard the context if it's a store-path. Which then means you can no longer nix-build it.
Where this was possible before. I'd like to keep isString unless some concrete example can be shown why we need to discard the context.

@illdefined
Copy link
Contributor Author

Is it acceptable to check the path with hasStorePathPrefix, which involves converting the option value to a path, or should I try to find an alternative solution that only operates on strings?

Hm, this could be a small regression. Using discardStringContext now would discard the context if it's a store-path. Which then means you can no longer nix-build it. Where this was possible before. I'd like to keep isString unless some concrete example can be shown why we need to discard the context.

The context is only discarded for the purpose of checking. The option value retains any context.

@hsjobeki
Copy link
Contributor

carded for the purpose of checking. The option value retains any context.

Ah yeah, my mistake right. 👍

@hsjobeki hsjobeki merged commit 013beed into NixOS:master Apr 21, 2025
29 checks passed
@illdefined illdefined deleted the types-path-with branch April 21, 2025 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants