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

Don't rerun if PATH changes #1215

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Don't rerun if PATH changes #1215

merged 1 commit into from
Sep 27, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented Sep 27, 2024

Prevents spurious rebuilds on Windows (https://github.com/rust-lang/cc-rs/pull/1103/files#r1674907945) as suggested by @NobodyXu.

It looks like this change causes crates to rebuild on Windows any time the PATH changes. Anecdotally, that seems to have a pretty high false positive rate, and since some PATH changes are associated with tools like rust-analyer that run constantly in the background, this can make it so that every build is a complete rebuild in some setups / with some dependencies. See for example BLAKE3-team/BLAKE3#324. I might file this as an issue shortly, but I wanted to mention it real quick before I forget.

Originally posted by oconnor663

Perhaps we can special-case PATH here?

For example, Update the getenv to not emit anything for PATH

Originally posted by NobodyXu

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.

(duplicate)

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.

I wonder if we should do something more?

I.e. when executing binaries, get the path of binary and ask cargo to watch it.

Unfortunately there isn't any API in stdlib to do so and it sounds a hit too hard for now, but I think we should think about this.

@NobodyXu NobodyXu merged commit 71c672b into rust-lang:main Sep 27, 2024
26 checks passed
@github-actions github-actions bot mentioned this pull request Sep 27, 2024
@ChrisDenton ChrisDenton deleted the special branch September 27, 2024 09:26
@ChrisDenton
Copy link
Member Author

Yeah, that might be nice but would definitely be a more complex feature as we'd need to account for different platforms.

@NobodyXu
Copy link
Collaborator

Yeah, that might be nice but would definitely be a more complex feature as we'd need to account for different platforms.

And it'd still be half broken: If user add another compiler in /usr/local/bin/cc while we ask cargo to detect change at /usr/bin/cc, it'd still miss that.

I think if we want to do this correctly, by making sure we always recompiles if the compiler gets swapped, we will have to do it the same way sccache detects change.

We will have to open the compiler binary, hashed the binary itself, and all the dynamic libraries it linked in (libclang.so, libllvm.so).

We'd need a new cargo:rerun-if primitives for such thing, because cargo decides if the build-script gets run.

@NobodyXu
Copy link
Collaborator

And even that may not work, if the compiler is wrapped by a bash script.

Honestly I feel like this is something incredibly hard to get right, we probably want to document this in our crate-level doc.

@ChrisDenton
Copy link
Member Author

That's probably a better approach. Honestly, users know their build better than we can possibly infer and are therefore better able to do this. Or at least there's no way we can reasonably cover all cases well.

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