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.

Sign up for GitHub

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

Jump to bottom

[fix]:Build script not rerun when target rustflags change #13560

Merged
merged 2 commits into from
Apr 8, 2024
Merged

[fix]:Build script not rerun when target rustflags change #13560

merged 2 commits into from
Apr 8, 2024

Conversation

heisen-li
Copy link
Contributor

What does this PR try to resolve?

Fixes #13003

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-rebuild-detection Area: rebuild detection and fingerprinting labels Mar 8, 2024
@heisen-li
Copy link
Contributor Author

@weihanglo @epage I haven't found a better way to limit environment variables to what the build script requires. This seems to require first confirming which environment variables the build script cares about, but I haven't figured out how to confirm this.I will keep looking and if you have any suggestions it would save a lot of time, thank you.

The test cases are from anyhow, I hope it will be helpful for understanding.

@heisen-li heisen-li changed the title [WIP][fix]:Build script not rerun when target rustflags change [fix]:Build script not rerun when target rustflags change Mar 15, 2024
@heisen-li
Copy link
Contributor Author

Note that we'd also need tests for this. Ideally a test is by itself in the first commit, showing the problem (by passing). The second commit fixes the problem and updates the test. The change in the test helps show that we are testing the right thing and what the intended change was.

If we understand it correctly，
first commit, showing the problem (by passing) is dc88e34

second commit fixes the problem and updates the test is b3c42f6

@heisen-li
Copy link
Contributor Author

If you want to limit the environment variables to those required by the build script, then you need to determine which environment variables are required by the build script.

I haven't come up with a better solution other than progressive scanning (and I don't think this is the right kind of solution), and it's harder than I thought it would be if I wanted to do it.

Also, more information about whether unnecessary reconstruction occurred might have been better.

Honorable @weihanglo @epage

@epage epage mentioned this pull request Mar 15, 2024
Closed
@heisen-li
Copy link
Contributor Author

The current revision will collect in target extra flags to pass to rustc.

Thanks for the discussion in issue, it has been very beneficial for me. Is there still a revision I need to make?

weihanglo
weihanglo reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Could you clean up commits to two commits? Assumely we want to have first commit for the test, and last for the fix.

tests/testsuite/build_script.rs
@@ -5527,3 +5527,80 @@ fn test_old_syntax_with_old_msrv() {
p.cargo("build -v").run();
p.cargo("run -v").with_stdout("foo\n").run();
}

#[cargo_test]
fn build_script_rerun_when_target_rustflags_change() {
Copy link
Member

Choose a reason for hiding this comment

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

The first commit didn't really demonstrate the existing bug.

  • It failed on macOS and Windows as x86_64-unknown-linux-gnu might not be available. I would suggest using rustc_host instead of hardcoded target triple.
  • It requires nightly toolchain. I understand you want to replicate the case in anyhow, though I think it could be simplified to avoid nightly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I may have wasted some computing resources due to my oversight.

The revised test may make it easier to understand the fix.

src/cargo/core/compiler/fingerprint/mod.rs Show resolved Hide resolved
@heisen-li heisen-li force-pushed the build_flag branch 3 times, most recently from 6919690 to 5ebe68c Compare April 6, 2024 09:30
@heisen-li heisen-li marked this pull request as draft April 6, 2024 10:06
@heisen-li heisen-li marked this pull request as ready for review April 7, 2024 08:00
@heisen-li heisen-li force-pushed the build_flag branch 2 times, most recently from 396faf2 to 11d3591 Compare April 7, 2024 11:23
weihanglo
weihanglo approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks. I went ahead and updated the commit messages a bit. Will merge this when CI is all green.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

📌 Commit db7afeb has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2024
@bors
Copy link
Collaborator

bors commented Apr 8, 2024

⌛ Testing commit db7afeb with merge bd1cf58...

@bors
Copy link
Collaborator

bors commented Apr 8, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing bd1cf58 to master...

@bors bors merged commit bd1cf58 into rust-lang:master Apr 8, 2024
21 checks passed
@weihanglo weihanglo mentioned this pull request Apr 10, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
@bors
Auto merge of rust-lang#123745 - weihanglo:update-cargo, r=weihanglo
aa067fb 
Update cargo

11 commits in 28e7b2bc0a812f90126be30f48a00a4ada990eaa..74fd5bc730b828dbc956335b229ac34ba47f7ef7
2024-04-05 19:31:01 +0000 to 2024-04-10 18:40:49 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#13731)
- fix(cargo-fix): dont apply same suggestion twice (rust-lang/cargo#13728)
- refactor: make `resolve_with_previous` clearer (rust-lang/cargo#13727)
- fix(package): Normalize paths in `Cargo.toml` (rust-lang/cargo#13729)
- refactor: Track when MSRV is explicitly set, either way (rust-lang/cargo#13732)
- [fix]:Build script not rerun when target rustflags change (rust-lang/cargo#13560)
- feat(add): Stabilize MSRV-aware version req selection (rust-lang/cargo#13608)
- Fix github fast path redirect. (rust-lang/cargo#13718)
- Add release notes for 1.77.1 (rust-lang/cargo#13717)
- doc(semver): remove mention of deprecated tool rust-semverver (rust-lang/cargo#13715)
- chore: fix some typos (rust-lang/cargo#13714)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Apr 10, 2024
@dtolnay dtolnay mentioned this pull request Apr 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.

Build script not rerun when target rustflags change
4 participants
@heisen-li @rustbot @weihanglo @bors