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

Source path error improvements #12915

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Apr 3, 2025

Motivation

Based on #12532 but heavily modified.

We now "mount" the source accessors of flake inputs on top of /nix/store (i.e. the MountedSourceAccessor named storeFS). This allows error messages similar to the lazy-trees branch, i.e. referring to the original source trees (like a Git repo) instead of the store path to which the input was copied.

For example, instead of

error: attribute 'foobar' missing
at /nix/store/ddzfiipzqlrh3gnprmqbadnsnrxsmc9i-source/machine/configuration.nix:209:7:
   208|
   209|       pkgs.foobar
      |       ^
   210|     ];

you now get

error: attribute 'foobar' missing
at /home/eelco/Misc/eelco-configurations/machine/configuration.nix:209:7:
   208|
   209|       pkgs.foobar
      |       ^
   210|     ];

Context


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 new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority fetching Networking with the outside (non-Nix) world, input locking labels Apr 3, 2025
@Mic92
Copy link
Member

Mic92 commented Apr 3, 2025

Cool. Could you write a changelog message for this? This is a major UX improvement.

@edolstra edolstra force-pushed the source-path-errors branch from ffd65d7 to 8513598 Compare April 3, 2025 15:07
Comment on lines +3073 to +3092
// Backward compatibility hack: throw an exception if access
// to this path is not allowed.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a hack?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah where is FilteringSourceAccessor even being used anymore? I only see it used with GitExportIgnoreSourceAccessor

Comment on lines +3147 to +3166
// Backward compatibility hack: throw an exception if access
// to this path is not allowed.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a hack?

@@ -91,7 +92,9 @@ static StorePath copyInputToStore(
{
auto storePath = fetchToStore(*state.store, accessor, FetchMode::Copy, input.getName());

state.allowPath(storePath);
state.allowPath(storePath); // FIXME: should just whitelist the entire virtual store
Copy link
Member

Choose a reason for hiding this comment

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

Not a bug:

Suggested change
state.allowPath(storePath); // FIXME: should just whitelist the entire virtual store
state.allowPath(storePath); // TODO: we could simply whitelist the entire virtual store, because all of that is created by the evaluator and its allowed external sources

@roberth
Copy link
Member

roberth commented Apr 5, 2025

In fact, we don't even need to copy the inputs to the store at all, so
this gets us very close to lazy trees. We just need to know the store
path so that requires hashing the entire input, which isn't lazy. But
the next step will be to use a virtual store path that gets rewritten
to the actual store path only when needed.

This could use some clarification so that others can follow.
We have multiple operations or optimizations that may be called "lazy":

  • lazy copying: copy as little to the store as possible
  • lazy evaluation and lazy locking: completely ignore an input if possible (already somewhat possible, and improving with Make nix flake metadata|update|lock lazy #12432, Reuse input lock files #7730)
  • lazy outPath: use input metadata without fetching anything (e.g. (fetchTree foo).rev) probably already provided by call-flake.nix.
  • fetch as little as possible (we don't call this laziness, but someone might, and that'd be confusing)
  • incremental fetching: e.g. Git submodules and LFS only on access
  • lazy hashing: hash as little as possible, avoiding read operations from local cache

What you're suggesting as the next step is to provide lazy copying.
Lazy hashing and incremental fetching would be part of #11367 or #6530.

I hope I'm not adding to the confusion.

@Mic92 Mic92 added this to Nix team Apr 5, 2025
@github-project-automation github-project-automation bot moved this to To triage in Nix team Apr 5, 2025
@Ericson2314
Copy link
Member

I suggest we do this in conjunction with #12531

@Ericson2314
Copy link
Member

I changed #12531 as discussed in the meeting, and then rebased this on top in https://github.com/Ericson2314/nix/tree/source-path-errors

This makes paths in error messages behave similar to lazy-trees,
e.g. instead of store paths like

       error: attribute 'foobar' missing
       at /nix/store/ddzfiipzqlrh3gnprmqbadnsnrxsmc9i-source/machine/configuration.nix:209:7:
          208|
          209|       pkgs.foobar
             |       ^
          210|     ];

you now get

       error: attribute 'foobar' missing
       at /home/eelco/Misc/eelco-configurations/machine/configuration.nix:209:7:
          208|
          209|       pkgs.foobar
             |       ^
          210|     ];

In fact, we don't even need to copy the inputs to the store at all, so
this gets us very close to lazy trees. We just need to know the store
path so that requires hashing the entire input, which isn't lazy. But
the next step will be to use a virtual store path that gets rewritten
to the actual store path only when needed.
@edolstra edolstra force-pushed the source-path-errors branch from 8513598 to addec3c Compare April 10, 2025 09:00
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 new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

4 participants