Skip to content

Make dummy store also store path info#14023

Merged
roberth merged 7 commits intomasterfrom
dummy-store-path-info
Sep 21, 2025
Merged

Make dummy store also store path info#14023
roberth merged 7 commits intomasterfrom
dummy-store-path-info

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 19, 2025

Motivation

Filling in the missing methods.

Context

See discussion in #10915. We might hereafter split dummy:// and memory://.

Depends on #14035.


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 store Issues and pull requests concerning the Nix store label Sep 19, 2025
@xokdvium xokdvium force-pushed the dummy-store-path-info branch 2 times, most recently from 26d5af0 to a4cb582 Compare September 20, 2025 10:24
@xokdvium xokdvium marked this pull request as ready for review September 20, 2025 10:29
@xokdvium xokdvium requested a review from edolstra as a code owner September 20, 2025 10:29
@xokdvium xokdvium marked this pull request as draft September 20, 2025 14:12
@xokdvium xokdvium force-pushed the dummy-store-path-info branch from a4cb582 to 5276eee Compare September 20, 2025 14:37
xokdvium added a commit that referenced this pull request Sep 20, 2025
We should use proper abstractions for reading files from the store.
E.g. this caused errors when trying to download github flakes into
an in-memory store in #14023.
@dpulls
Copy link

dpulls bot commented Sep 21, 2025

🎉 All dependencies have been resolved !

@xokdvium xokdvium force-pushed the dummy-store-path-info branch from 5276eee to a453a49 Compare September 21, 2025 10:50
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Sep 21, 2025
@xokdvium xokdvium marked this pull request as ready for review September 21, 2025 10:50
Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

Needs a bit more work to make thread-safe. Not sure if that is a a very important consideration for this first pass on this.

Comment on lines +34 to +41
/**
* This view conceptually just borrows the file systems objects of
* each store object from `contents`, and combines them together
* into one store-wide source accessor.
*
* This is needed just in order to implement `Store::getFSAccessor`.
*/
ref<MemorySourceAccessor> wholeStoreView = make_ref<MemorySourceAccessor>();
Copy link
Member

Choose a reason for hiding this comment

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

This would be a lot simpler if we didn't have a store-wide getFSAccessor requirement.

Either we could keep it:
Also it seems that this is a general problem, and we could have a store mix-in that implements getFSAccessor by means of an abstract method that gets an accessor for an individual store path.
Then all synchronization would be limited to that particular method.

Or we could drop it in favor of per-store path accessors.
Then non-listable stores also become more "valid" Store implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we also could do is have a "subdirectory" view of mounted accessors. That would help with thread safety as well.

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 like the point about non-listable stores.

The store-wide accessor could go in the same interface (in the store hierarchy) as the stuff for listing stores, like #8213

Non-listable stores can also contain different objects for different store dirs too. E.g. binary cache can be heterogeneous in store dir.

Copy link
Member Author

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I approve the follow up work!

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

@roberth roberth merged commit ab7feb3 into master Sep 21, 2025
29 checks passed
@roberth roberth deleted the dummy-store-path-info branch September 21, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store 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.

3 participants