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 path to cache key for Rust dep inference, for relative imports (cherry-pick of #19630) #19640

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Aug 22, 2023

This adds the file path for the file being dependency-inferred to the cache key for the Rust dep inference. Without this, the same file contents appearing multiple times in different places will give the wrong results for relative imports, because the dep inference process mashes together the file path and the relative import.

The circumstances here seem most likely to occur in the real world if a file is moved, with the inference results from before the move reused for the file after the move too.

I've tested this manually on the reproducer in #19618 (comment), in addition to comparing the before (fails) and after (passes) behaviour of the new test.

To try to make this code more resilient to this sort of mistake, I've removed the top level PreparedInferenceRequest.path key, in favour of using the new input_file_path on the request protobuf struct, at the cost of an additional String -> PathBuf conversion when doing the inference.

Fixes #19618

This is a cherry pick of #19630, but is essentially a slightly weird rewrite that partially cherry-picks #19001 too, by copying over the whole DependencyInferenceRequest protobuf type as is (even with some fields that are unused) because that's the easiest way to generate appropriate bytes for turning into the digest for use in the cache-key. I think it's okay to have this "weird" behaviour in the 2.17 branch, with the real/normal code in main/2.18?

@huonw huonw requested a review from thejcannon August 22, 2023 02:54
@huonw huonw added this to the 2.17.x milestone Aug 22, 2023
@huonw huonw changed the title Add path to cache key for Rust dep inference, for relative imports (cherry-pick of #19360) Add path to cache key for Rust dep inference, for relative imports (cherry-pick of #19630) Aug 22, 2023
@huonw huonw added the category:internal CI, fixes for not-yet-released features, etc. label Aug 22, 2023
@thejcannon thejcannon merged commit feeb60a into pantsbuild:2.17.x Aug 22, 2023
@huonw huonw deleted the cherry-pick-19630-to-2.17.x branch August 22, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants