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

Remove some rerun-if-env-changed lines from build.rs. #576

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

sunfishcode
Copy link
Member

Now that we appear to have a known-working fix for #526, we can investigate relaxing the fix. If #526 reappears, we now have a known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds in some situations, so remove these. In theory, cargo should already be aware of these environment variables. In practice, there were reports of build failures which had the appearance of using an older version of rustc with build.rs output from a newer version of rustc, and it wasn't clear what was happening. But, it's possible that #563 fixes the issue properly. So let's try removing these rerun-if-env-changed lines, and see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being unnecessary here.

Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
@OvermindDL1
Copy link

I can reliably test both states, when it is too strict and when it is not strict enough. If you give me a branch or checkout to test then I can change my deps to that and test it out?

@sunfishcode
Copy link
Member Author

@OvermindDL1 I'd much appreciate it. Would you be able to test the "sunfishcode/build-scripts" branch, which is what's in this PR? https://github.com/bytecodealliance/rustix/tree/sunfishcode/build-scripts

@OvermindDL1
Copy link

Patched it in via:

[patch.crates-io]
rustix = { git = "https://github.com/bytecodealliance/rustix.git", branch = "sunfishcode/build-scripts" }

Running cargo clean, then cargo build in cli, then cargo build in intellij, success, then cargo build in cli again, success, then cargo clean, then cargo build in intellij, then cargo build in cli, success, then cargo build in intellij, success.

I'm not experiencing either the old or the new issue with it, it's looking good for me at least. :-)

@OvermindDL1
Copy link

OvermindDL1 commented Mar 29, 2023

Also went through my normal workflow (cargo-watch test in terminal on one screen, compiling and running in intellij on the other, while editing code of course), it's much faster now!

@OvermindDL1
Copy link

And a quick test with rust-analyzer via emacs and I can't reproduce it there either.

@OvermindDL1
Copy link

Note, I tested this with latest stable rust, 1.68.2, I cannot comment about older versions. If you want me to test with a specific older version then I can install it and test?

@sunfishcode
Copy link
Member Author

Thanks for testing this! In #526 the problem was reported in Rust 1.65, so if you could install and test Rust 1.65, that'd be a useful data point.

@OvermindDL1
Copy link

Sorry for the delay, just got back, installing 1.65 now, setting up intellij to 1.65, performing the same tests, same tests pass in intllij, testing emacs/rust-analyzer, same tests pass again, cargo watch test, also still showing good, confirmed cargo version is 1.65 reporting in all locations.

Anything else you want tested while I have all this still prepared?

@sunfishcode
Copy link
Member Author

This is really great, thanks! I can't think of anything else that urgently needs testing here. It seems very likely that #563 fixed the root issue here. So I'll plan to merge this PR soon and do a point release, and then keep an eye out to see if any problems surface.

@sunfishcode sunfishcode merged commit 89a6cc1 into main Mar 29, 2023
@sunfishcode sunfishcode deleted the sunfishcode/build-scripts branch March 29, 2023 18:10
sunfishcode added a commit that referenced this pull request May 19, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
sunfishcode added a commit that referenced this pull request May 19, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
sunfishcode added a commit that referenced this pull request Oct 12, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
sunfishcode added a commit that referenced this pull request Oct 12, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
sunfishcode added a commit that referenced this pull request Oct 12, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
sunfishcode added a commit that referenced this pull request Oct 12, 2023
Now that we appear to have a known-working fix for #526, we can
investigate relaxing the fix. If #526 reappears, we now have a
known-working state we can quickly revert to.

Reports in #562 and #575 are that having rerun-if-env-changed=RUSTC
and rerun-if-env-changed=RUSTC_WRAPPER are triggering spurious rebuilds
in some situations, so remove these. In theory, cargo should already be
aware of these environment variables. In practice, there were reports of
build failures which had the appearance of using an older version of
rustc with build.rs output from a newer version of rustc, and it wasn't
clear what was happening. But, it's possible that #563 fixes the issue
properly. So let's try removing these rerun-if-env-changed lines, and
see if the problem resurfaces.

And, rerun-if-env-changed=TARGET is explicitly documented as being
unnecessary [here].

[here]: https://doc.rust-lang.org/cargo/reference/build-scripts.html
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.

None yet

2 participants