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

Use relative directory for obj files hash #1270

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Nov 5, 2024

When producing .o files, the absolute path of each source file is hashed and used as a prefix for the output object files. While this avoids clashes between different source files with the same basenames, this causes determinism issues in the build as the full system path influences the build output.

This change strips the out_dir parent from the source file paths, assuming that the source files are in the out_dir as well.

When producing `.o` files, the absolute path of each source file is
hashed and used as a prefix for the output object files. While this
avoids clashes between different source files with the same basenames,
this causes determinism issues in the build as the full system path
influences the build output.

This change strips the out_dir parent from the source file paths,
assuming that the source files are in the out_dir as well.
src/command_helpers.rs Outdated Show resolved Hide resolved
@nmattia
Copy link
Contributor Author

nmattia commented Nov 7, 2024

@NobodyXu I missing some context re. Build::getenv vs std::env::var, can you advise here?

#1103

@NobodyXu

This comment has been minimized.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 7, 2024

Actually, it's ok to use env::var here, because CARGO_* is a cargo env that always triggers rebuild.

You just need to add a clippy ignore and a comment emplaning why the clippy lint is ignored

@nmattia nmattia marked this pull request as ready for review November 7, 2024 13:34
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 7, 2024

Thanks, just a few change requests

@nmattia nmattia requested a review from NobodyXu November 7, 2024 13:52
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, just one final comment:

src/command_helpers.rs Outdated Show resolved Hide resolved
@nmattia nmattia requested a review from NobodyXu November 7, 2024 15:16
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you!

@NobodyXu NobodyXu merged commit 15d2f7d into rust-lang:main Nov 8, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 8, 2024
@nmattia nmattia deleted the nm-relative-dst branch November 8, 2024 11: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.

2 participants