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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport SourcePath from the lazy-trees branch #8172

Merged
merged 6 commits into from Apr 24, 2023

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Apr 6, 2023

Motivation

This introduces the SourcePath type from lazy-trees as an abstraction for accessing files from inputs that may not be materialized in the real filesystem (e.g. Git repositories). Currently, however, it's just a wrapper around CanonPath, so it shouldn't change any behaviour. (On lazy-trees, SourcePath is a <InputAccessor, CanonPath> tuple.)

The reason for this PR is to make the lazy-trees diff smaller and make it easier to keep that branch in sync.

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 馃憤 to pull requests you find important.

This introduces the SourcePath type from lazy-trees as an abstraction
for accessing files from inputs that may not be materialized in the
real filesystem (e.g. Git repositories). Currently, however, it's just
a wrapper around CanonPath, so it shouldn't change any behaviour. (On
lazy-trees, SourcePath is a <InputAccessor, CanonPath> tuple.)
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 6, 2023
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.

Partial review.
Mostly questions to be addressed by doc comments.

/**
* Copy this `SourcePath` to the Nix store.
*/
StorePath fetchToStore(
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this addToStore. Fetching only makes sense for some StorePaths, and currently none.

Copy link
Member Author

Choose a reason for hiding this comment

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

addToStore is already pretty overloaded. fetchToStore makes clear that this involves libfetcher.

Comment on lines +115 to +116
std::string to_string() const
{ return path.abs(); }
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the string?
What's the syntax of the returned string?
Does it need to be deterministic?

Copy link
Member Author

Choose a reason for hiding this comment

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

On the lazy-trees branch, it represents a human-readable representation of the path, such as 芦github:NixOS/nixpkgs/bla:foo/bar.nix禄, intended in error messages etc. So it doesn't need to be deterministic, but it currently is.


/**
* Return the location of this path in the "real" filesystem, if
* it has a physical location.
Copy link
Member

Choose a reason for hiding this comment

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

All source paths can be given a physical location.
What are valid reasons not to return one?
Should this guarantee accurate file contents at that path?
What are valid use cases for this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, all source paths can be written to the filesystem, but for some fetchers this could be expensive. If the caller needs a physical path they can use fetchToStore().

This is currently used for nix-instantiate --find-file, nix edit and nix develop (where we can't chdir into a flake that doesn't exist in the filesystem).

src/libfetchers/input-accessor.hh Show resolved Hide resolved
@github-actions github-actions bot added fetching Networking with the outside (non-Nix) world new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels Apr 17, 2023
@Ericson2314 Ericson2314 changed the title Backport SourcePath from the lazy-trees branch Backport SourcePath from the lazy-trees branch Apr 17, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-04-17-nix-team-meeting-minutes-49/27379/1

@edolstra edolstra enabled auto-merge April 24, 2023 11:38
@edolstra edolstra merged commit 249ce28 into NixOS:master Apr 24, 2023
8 checks passed
@infinisil
Copy link
Member

This causes a regression where builtins.pathExists "/some/file/" now returns true when it returned false before, see #8838

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 new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger with-tests Issues related to testing. PRs with tests have some priority
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants