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

Add option disallow-copy-paths to track down unnecessary copying #11746

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

Conversation

roberth
Copy link
Member

@roberth roberth commented Oct 24, 2024

Motivation

To quote the docs:

This is useful for finding expressions which copy sources, which can slow down evaluation.
You may find copied sources by running nix commands with increased verbosity, such as nix build -vvvv 2>&1 | grep /nix/store.
After identifying one more more paths, run nix build --option disallow-copy-paths /nix/store/... --show-trace to find the expression that copies the path, or add --debugger.

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.

@roberth roberth added error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc performance labels Oct 24, 2024
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Oct 24, 2024
@roberth roberth changed the title Add option disallow-copy-paths Add option disallow-copy-paths to track down unnecessary copying Oct 24, 2024
@@ -247,6 +247,17 @@ struct EvalSettings : Config

This option can be enabled by setting `NIX_ABORT_ON_WARN=1` in the environment.
)"};

Setting<std::set<std::string>> disallowCopyPaths{this, {}, "disallow-copy-paths",
Copy link
Member

Choose a reason for hiding this comment

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

std::string?

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Like the idea

@roberth
Copy link
Member Author

roberth commented Nov 4, 2024

We chatted a bit that the meaning of these strings may have to change a bit. Currently they're absolute paths, and that's ok for now, but we'll need something else when we have virtual paths.
Ideally whatever Nix logs about can be passed into this setting verbatim, in support of the workflow in the option description.

Perhaps easier to use would be an option where you can specify globs of forbidden file names which may be fetched and read, but not copied. A simple *.nix may already be a useful check for most projects, capable of finding unintentional Nixpkgs copying (although NixOS does put nixos/ in the sandbox for building the manual)

An option where you specify substrings of file contents that mustn't be copied would also be interesting, but quickly turns in a security feature that I don't know if we want to support. Maybe later.

@roberth roberth marked this pull request as draft November 4, 2024 20:13
@roberth
Copy link
Member Author

roberth commented Nov 4, 2024

I think this should implement the globbing idea instead. It's a bit more work, requiring something like a nar sink wrapper and some more glue, but digging through logs is bad UX.
Marked as draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc performance 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.

2 participants