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

Make rootFS's showPath() render the paths from the original accessors #12532

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

Conversation

edolstra
Copy link
Member

Motivation

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|     ];

This should also enable fixing #4507 (similar to lazy-trees) but that will require a bit more work.

Depends on #12512.

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 Feb 20, 2025
Copy link

dpulls bot commented Feb 20, 2025

🎉 All dependencies have been resolved !

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|     ];
@edolstra edolstra force-pushed the map-to-original-accessors branch from 82cb354 to 172d57e Compare February 20, 2025 00:54
@roberth
Copy link
Member

roberth commented Feb 25, 2025

It seems that this would affect the equality of roots. That seems acceptable for entirely flake-based code, but some Nixpkgs and third-party code will be used in this context without the code or their maintainers being aware of the change.
If I'm correct about the following, this needs a release note describing the new phenomenon.

let rootOf = lib.converge dirOf;
in rootOf self.outPath != /.;

This seems to bring us close to the semantics of #11367, which also introduces distinct roots, but I dont think this PR implements something to avoid creating those <hash>-<hash>-source paths yet (which could of course be done later).

@@ -245,6 +245,16 @@ public:
/** `"unknown"` */
Value vStringUnknown;

using StorePathAccessors = std::map<CanonPath, ref<SourceAccessor>>;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more future-compat if this did not use CanonPath keys, both for lazy trees and for Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now it must use CanonPath, because we don't have a concept of non-CanonPaths in the virtual filesystem exposed in the evaluator.

@edolstra
Copy link
Member Author

It seems that this would affect the equality of roots.

I don't think so. This PR should only affect the display of error messages. It shouldn't change the semantics of evaluation.

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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants