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

Use SourcePath for reading flake.{nix,lock} #10088

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

edolstra
Copy link
Member

Motivation

Flakes still reside in the Nix store (so there shouldn't be any change in behaviour), but they are now accessed via the rootFS accessor. Since rootFS implements access checks, we no longer have to worry about flake.{nix,lock} or their parents being symlinks that escape from the flake.

Extracted from the lazy-trees branch.

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.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Feb 26, 2024
Flakes still reside in the Nix store (so there shouldn't be any change
in behaviour), but they are now accessed via the rootFS
accessor. Since rootFS implements access checks, we no longer have to
worry about flake.{nix,lock} or their parents being symlinks that
escape from the flake.

Extracted from the lazy-trees branch.
@Ericson2314
Copy link
Member

@edolstra Can we use store->getFSAccessor() for this? That will dovetail with fixing some existing issues.

@@ -55,16 +55,14 @@ struct LockFile
ref<Node> root = make_ref<Node>();

LockFile() {};
LockFile(const nlohmann::json & json, const Path & path);
LockFile(std::string_view contents, std::string_view path);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LockFile(std::string_view contents, std::string_view path);
LockFile(nlohmann::json json, std::string_view path);

I think I would prefer that: let the caller parse and move in. Just as efficient, and keeps are more informative method type.

Copy link
Member Author

@edolstra edolstra Feb 26, 2024

Choose a reason for hiding this comment

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

I don't think that would be better. To the caller the lockfile is just an opaque string. The fact that it's JSON is an implementation detail of LockFile.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the caller does want to know that toJSON() and this are related by a round trip law, right?

I don't mind also exposing the current signature as a simple wrapper if it is easier for call sites.

@edolstra
Copy link
Member Author

Can we use store->getFSAccessor() for this?

Probably best not to do that right now, because it could change semantics if the paths seen by the evaluator are not in rootFS. (That is the whole problem with lazy-trees.)

@roberth
Copy link
Member

roberth commented Feb 26, 2024

could change semantics if the paths seen by the evaluator are not in rootFS. (That is the whole problem with lazy-trees.)

Wouldn't it only affect flakes, and only affect flakes that have bugs? Granted, legacy code should be usable through flakes, but maybe this particular issue is very minor.
IIRC most of the issues came from toString and /nix/store/virtual${...} paths.

I think we could try that in 2.22, if 2.21 goes fairly well.

@Ericson2314
Copy link
Member

#9852 This is the bug I am thinking of, FYI.

@thufschmitt thufschmitt merged commit eaa6c26 into NixOS:master Mar 4, 2024
8 checks passed
@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-03-04-nix-team-meeting-minute-130/40830/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants