Factor out `StoreDirConfig` by Ericson2314 · Pull Request #6236 · NixOS/nix

Ericson2314 · 2022-03-11T23:04:29Z

Motivation

A lot of code doesn't need a bonafide store, but just needs to work with store paths. That code doesn't need the store interface, but just one setting, the store directory.

For sake of purity / restricting ambient authority --- and all the attendant benefits regarding security, code clarity, and code maintainability --- I think it is good to move that one setting and associated methods into a new super-class: StoreDirConfig.

Context

The first commit just does the bare minimum change. The second shows what we can do with the first, deprivilaging a bunch of code from taking const Store & to just const StoreDirConfig &.

I didn't bother moving the implementation of many store methods to new a store-dir-config.cc because:

They were already split in two locations (path.cc and store-api.cc), so their locations are hardly less "wrong" than before
I suspect after yet more shuffling of content addressing, we'll be able to get rid of many of these methods because they will become overlapping/redundant. At that point, there will be less stuff to move.

More progress on #5729.

~~depends on #6258~~
~~depends on #6237~~

edolstra · 2022-03-14T13:21:40Z

I'm not really in favor of splitting things just because we can... Is there a specific use case for splitting off StoreDirConfig? (E.g. is there a plausible scenario where we have a StoreDirConfig that isn't a Store?)

Ericson2314 · 2022-03-14T14:59:35Z

Yes there is, #6237 which depends on this. I could pass a whole store there, but that that is somewhat sketchy from a caching perspective. If I just pass StoreDirConfig, it is more clear that the cache key does have something to do with the store dir, but no other store information.

Ericson2314 · 2022-03-14T15:00:49Z

This was working in Clang, but not GCC, due to that bug @thufschmitt ran into when first making StoreConfig. Hopefully I can fix soon.

Ericson2314 · 2022-03-14T17:43:30Z

Actually it's a double free instead. #6258 came from my attempts debugging this over the weekend. I rebase this on top, but still no dice.

Ericson2314 · 2022-03-14T19:45:02Z

I built with clang and the double free went away. Ugh!

edolstra · 2022-03-18T14:47:01Z

Yes there is, #6237 which depends on this.

That's a bit of a circular motivation, because I'm not sure what the reason for #6237 is...

Ericson2314 · 2022-03-18T15:07:52Z

Err the motivation of #6237 is simply to use StorePath where possible to demonstrate the invariant that the item in question is in fact a store path, as we have done many times before, e.g. your recent #6198.

edolstra · 2022-03-18T16:21:32Z

Ah thanks, yeah that makes sense!

Ericson2314 · 2022-03-18T16:59:59Z

@edolstra Yay! I was hoping the core change just got lost in there. I flipped the dependencies around so now this depends on that:

In Decode string context straight to using StorePaths #6237 We share Store with the eval cache a bit more, which is sketchy re purity lest we accidentally use the store for something other than printing and parsing, but I suppose fine for now as eval contexts already have a store.
This PR will limit the use of Store in the eval cache, in addition to making StoreDirConfig. I assume there are many more instances where we could similarly do the same. (We could even make EvalState just have a StoreDirConfig which is downcasted for things like IFD, a la what we did with LogStore and GcStore.)

If you are not yet convinced by this, but #6237 now sounds good, hopefully this new approach is an improvement.

Ericson2314 · 2022-03-25T04:01:52Z

Still not sure how to fix this, but in the most recent commit found a bunch of locations that can be made StoreDirConfig instead of Store.

dpulls · 2023-10-31T16:48:30Z

🎉 All dependencies have been resolved !

More progress on NixOS#5729.

- part of eval cache - part of derivations - derived path - store path with outputs - serializers

nixos-discourse · 2023-11-13T08:59:06Z

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

https://discourse.nixos.org/t/2023-11-10-nix-team-meeting-minutes-102/35379/1

nixos-discourse · 2023-11-27T16:08:46Z

This pull request 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

roberth

Just good application of the interface segregation principle. Thanks!

github-actions · 2023-12-07T21:16:44Z

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-6236-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-6236-to-2.18-maintenance
git switch --create backport-6236-to-2.18-maintenance
git cherry-pick -x e97ac09abeab44fa3d10eb539f0b3d51f8575798 dde1d863388617b3a63db808c125f274c86a3222

github-actions · 2023-12-07T21:16:47Z

Successfully created backport PR for 2.19-maintenance:

[Backport 2.19-maintenance] Factor out StoreDirConfig #9561

github-actions · 2023-12-07T21:16:48Z

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-6236-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-6236-to-2.18-maintenance
git switch --create backport-6236-to-2.18-maintenance
git cherry-pick -x e97ac09abeab44fa3d10eb539f0b3d51f8575798 dde1d863388617b3a63db808c125f274c86a3222

github-actions · 2023-12-07T21:16:50Z

Git push to origin failed for 2.19-maintenance with exitcode 1

We can gut out some gratuitous inhertence as follows: - `MixStoreDirMethods` -> `StoreDir` - `StoreDirConfig` deleted because no longer needed. It is just folded into `StoreConfig`. - `StoreDirConfigBase` -> `StoreConfigBase` same trick still needed, but now is for `StoreConfig` not `StoreDirConfig` Here's how we got here: 1. I once factored out `StoreDirConfig` in NixOS#6236. 2. I factored out `MixStoreDirMethods` in NixOS#13154. But, I didn't realize at point (2) that we didn't need `StoreDirConfig` anymore, all uses of `StoreDirConfig` could instead be uses of `MixStoreDirMethods`. Now I am doing that, and renaming `MixStoreDirMethods` to just `StoreDir` because `MixStoreDirMethods` is an unnecessarily weird name.

We can cut out some gratuitous inhertence as follows: - `MixStoreDirMethods` -> `StoreDir` - `StoreDirConfig` deleted because no longer needed. It is just folded into `StoreConfig`. - `StoreDirConfigBase` -> `StoreConfigBase` same trick still needed, but now is for `StoreConfig` not `StoreDirConfig` Here's how we got here: 1. I once factored out `StoreDirConfig` in NixOS#6236. 2. I factored out `MixStoreDirMethods` in NixOS#13154. But, I didn't realize at point (2) that we didn't need `StoreDirConfig` anymore, all uses of `StoreDirConfig` could instead be uses of `MixStoreDirMethods`. Now I am doing that, and renaming `MixStoreDirMethods` to just `StoreDir` because `MixStoreDirMethods` is an unnecessarily weird name.

We can cut out some gratuitous inhertence as follows: - `MixStoreDirMethods` -> `StoreDirConfig` - `StoreDirConfig` deleted because no longer needed. It is just folded into `StoreConfig`. - `StoreDirConfigBase` -> `StoreConfigBase` same trick still needed, but now is for `StoreConfig` not `StoreDirConfig` Here's how we got here: 1. I once factored out `StoreDirConfig` in NixOS#6236. 2. I factored out `MixStoreDirMethods` in NixOS#13154. But, I didn't realize at point (2) that we didn't need `StoreDirConfig` anymore, all uses of `StoreDirConfig` could instead be uses of `MixStoreDirMethods`. Now I am doing that, and renaming `MixStoreDirMethods` to just `StoreDirConfig` to reduce churn.

Ericson2314 mentioned this pull request Mar 12, 2022

Decode string context straight to using StorePaths #6237

Merged

Ericson2314 marked this pull request as draft March 14, 2022 15:00

Ericson2314 force-pushed the store-dir-config branch from c139364 to e725d25 Compare March 18, 2022 16:55

Ericson2314 force-pushed the store-dir-config branch from e725d25 to a558084 Compare March 25, 2022 03:34

stale bot added the stale label Sep 21, 2022

Ericson2314 mentioned this pull request Jan 6, 2023

Introduce StoreReferences and ContentAddressWithReferences #3746

Merged

7 tasks

Ericson2314 force-pushed the store-dir-config branch from a558084 to 8774d4a Compare January 6, 2023 23:21

stale bot removed the stale label Jan 6, 2023

Ericson2314 mentioned this pull request Feb 12, 2023

Remove workarounds for broken GCC #7815

Open

github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Oct 31, 2023

Ericson2314 force-pushed the store-dir-config branch from 8774d4a to 606a47a Compare October 31, 2023 20:28

Ericson2314 marked this pull request as ready for review October 31, 2023 20:28

Ericson2314 requested review from edolstra and thufschmitt as code owners October 31, 2023 20:28

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

github-actions bot removed the fetching Networking with the outside (non-Nix) world, input locking label Oct 31, 2023

Ericson2314 force-pushed the store-dir-config branch 2 times, most recently from 629cb4e to ea5b608 Compare November 2, 2023 13:33

Ericson2314 added 2 commits November 4, 2023 19:05

Factor out StoreDirConfig

e97ac09

More progress on NixOS#5729.

Restrict some code to StoreDirConfig

dde1d86

- part of eval cache - part of derivations - derived path - store path with outputs - serializers

Ericson2314 force-pushed the store-dir-config branch from ea5b608 to dde1d86 Compare November 4, 2023 23:05

thufschmitt self-assigned this Nov 10, 2023

roberth approved these changes Dec 1, 2023

View reviewed changes

roberth merged commit fcf0981 into NixOS:master Dec 1, 2023

Ericson2314 deleted the store-dir-config branch December 1, 2023 14:38

Ericson2314 added backport 2.18-maintenance labels Dec 7, 2023

github-actions bot mentioned this pull request Dec 7, 2023

[Backport 2.19-maintenance] Factor out StoreDirConfig #9561

Closed

Ericson2314 mentioned this pull request Dec 7, 2023

[Backport 2.19-maintenance] Factor out StoreDirConfig #9562

Closed

Ericson2314 mentioned this pull request Aug 15, 2025

Simplify "Store dir" superclass #13764

Merged

Uh oh!

Comments

Conversation

Ericson2314 commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

edolstra commented Mar 14, 2022

Uh oh!

Ericson2314 commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Mar 14, 2022

Uh oh!

Ericson2314 commented Mar 14, 2022

Uh oh!

Ericson2314 commented Mar 14, 2022

Uh oh!

edolstra commented Mar 18, 2022

Uh oh!

Ericson2314 commented Mar 18, 2022

Uh oh!

edolstra commented Mar 18, 2022

Uh oh!

Ericson2314 commented Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ericson2314 commented Mar 25, 2022

Uh oh!

dpulls bot commented Oct 31, 2023

Uh oh!

nixos-discourse commented Nov 13, 2023

Uh oh!

nixos-discourse commented Nov 27, 2023

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023

Uh oh!

github-actions bot commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ericson2314 commented Mar 11, 2022 •

edited

Loading

Ericson2314 commented Mar 14, 2022 •

edited

Loading

Ericson2314 commented Mar 18, 2022 •

edited

Loading