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

Don't copy nix store path to nix store #11229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yshui
Copy link
Contributor

@yshui yshui commented Jul 31, 2024

Motivation

Avoid unnecessary store -> store copies

Context

See #11228

I checked the PR introduced this change, and didn't find an explanation why this == "source" check is necessary.

IMO all store -> store copies are unnecessary. And even if there is indeed a valid reason this is only done for store paths named "source", it's not like the user can't just name their derivation "source".

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 fetching Networking with the outside (non-Nix) world, input locking label Jul 31, 2024
@edolstra
Copy link
Member

I think this would violate the postcondition that the resulting store path is named source. The store path should only depend on the contents of the input path. E.g. path:/foo and path:/nix/store/<hash>-xyzzy should result in the same store path, if /foo and /nix/store/blabla have the same contents. But this change would cause the first to return a store path like /nix/store/<hash'>-source and the latter /nix/store/<hash>-xyzzy.

@yshui
Copy link
Contributor Author

yshui commented Jul 31, 2024

hmm, i see your point. but nix store paths aren't content addressable to begin with (unless ca-derivation is used), is there really a benefit to treat input store paths differently? (or is there anything in nix/nixpkgs currently depends on this behavior?)

(and besides, i can still bypass all this by naming my derivation "source")

@Ericson2314
Copy link
Member

They are content addressed to begin with without that feature, just not ones produced by derivations.

@yshui
Copy link
Contributor Author

yshui commented Jul 31, 2024

@Ericson2314 is there a real way to tell them apart? is it possible to tell apart store paths that are actually content-addressable, vs store paths that just happen to be named "source"?

@yshui
Copy link
Contributor Author

yshui commented Aug 4, 2024

I have observed assertion failure from nix if I name my derivation "source" (without this patch). I think this is poorly thought out.

@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-08-07-nix-team-meeting-minutes-167/50287/1

@roberth
Copy link
Member

roberth commented Aug 7, 2024

Based on today's meeting, returning a path named source may become more relevant as part of a different solution that avoids copying in other cases or potentially more cases, so we're holding off on a decision on this PR for now.

@yshui
Copy link
Contributor Author

yshui commented Aug 8, 2024

would naming a derivation "source" become prohibited in the future?

@Ericson2314
Copy link
Member

@yshui No, I don't think we would need to prohibit that.

@roberth
Copy link
Member

roberth commented Aug 8, 2024

The reason to hold off is that we may need to revert this change in behavior soon, so that we don't have to track the custom name in the lazy path value returned by fetchTree.

would naming a derivation "source" become prohibited in the future?

No, I don't think we would need to prohibit that.

I agree and in fact it is desirable when performing file set filtering in a content-addressed derivation. If that derivation is also named source, and the contents are the same, it means that a flake-based derivation and a Nixpkgs derivation could potentially produce derivations with identical output hashes or identical realisations, avoiding a duplicate build.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/confused-about-nix-copying-nixpkgs/53737/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants