Skip to content

Include the source location when warning about inefficient double copies#79

Merged
grahamc merged 2 commits into
detsys-mainfrom
path-position-info
Jun 2, 2025
Merged

Include the source location when warning about inefficient double copies#79
grahamc merged 2 commits into
detsys-mainfrom
path-position-info

Conversation

@edolstra
Copy link
Copy Markdown
Collaborator

@edolstra edolstra commented May 30, 2025

Motivation

This improves the "inefficient double copy" warning by showing where the source path came from in the source, e.g.

warning: Performing inefficient double copy of path '/home/eelco/Dev/patchelf/' to the store at /home/eelco/Dev/patchelf/flake.nix:30:17. This can typically be avoided by rewriting an attribute like `src = ./.` to `src = builtins.path { path = ./.; name = "source"; }`.

This is implemented by adding position info to path values (actually to all values).

Context

(Actually, this adds a position field to *all* values.)

This allows improving the "inefficient double copy" warning by showing
where the source path came from in the source, e.g.

  warning: Performing inefficient double copy of path '/home/eelco/Dev/patchelf/' to the store at /home/eelco/Dev/patchelf/flake.nix:30:17. This can typically be avoided by rewriting an attribute like `src = ./.` to `src = builtins.path { path = ./.; name = "source"; }`.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 30, 2025

@github-actions github-actions Bot temporarily deployed to pull request May 30, 2025 15:59 Inactive
@grahamc grahamc changed the title Improve "inefficient double copy" warning Include the source location when warning about inefficient double copies Jun 2, 2025
@edolstra
Copy link
Copy Markdown
Collaborator Author

edolstra commented Jun 2, 2025

This could probably use a quick performance regression test, to make sure that adding a pos argument to finishValue() didn't make things slower.

Comment thread src/libexpr/paths.cc Outdated
@grahamc grahamc enabled auto-merge June 2, 2025 18:08
@grahamc
Copy link
Copy Markdown
Member

grahamc commented Jun 2, 2025

Got confirmation on Slack that there was no measurable performance impact.

@github-actions github-actions Bot temporarily deployed to pull request June 2, 2025 18:21 Inactive
@grahamc grahamc added this pull request to the merge queue Jun 2, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2025
@grahamc grahamc added this pull request to the merge queue Jun 2, 2025
Merged via the queue into detsys-main with commit f8aabb0 Jun 2, 2025
26 checks passed
@grahamc grahamc deleted the path-position-info branch June 2, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants