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

stabilize ci_rustc_if_unchanged_logic test for local environments #138245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

stabilize ci_rustc_if_unchanged_logic test for local environments #138245

wants to merge 1 commit into from

Conversation

onur-ozkan
Copy link
Member

Fixes #138239

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2025

r? @Kobzol

rustbot has assigned @Kobzol.
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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 8, 2025
@onur-ozkan onur-ozkan mentioned this pull request Mar 8, 2025
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@onur-ozkan onur-ozkan force-pushed the ci-rustc-test-fix branch 2 times, most recently from a592504 to e441c97 Compare March 8, 2025 22:50
jieyouxu
jieyouxu approved these changes Mar 9, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, the test fix LGTM. I have one suggestion re. how to make this test less likely to diverge in the future.

src/bootstrap/src/core/builder/tests.rs
config.last_modified_commit(&["compiler", "library"], "download-rustc", true).is_none();
let mut paths = vec!["compiler"];

// Handle library tree the same way as in `Config::download_ci_rustc_commit`.
Copy link
Member

@jieyouxu jieyouxu Mar 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion: I think we should either:

  • Have a single source of truth for the paths calculation fed to last_modified_commit for this purpose,
  • Or at least add a comment at the source site where we also do this paths adjustment to remind also updating this test.

wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have a single source of truth for the paths calculation fed to last_modified_commit for this purpose,

We can't do that as one side excludes those paths and the other includes them. Also, using that approach would cause this coverage to miss the expected behaviour (imagine that function somehow have bugs in its logic).

Or at least add a comment at the source site where we also do this paths adjustment to remind also updating this test.

Sure, let me know exactly where and what you want me to write.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just cc this test next to https://github.com/rust-lang/rust/blob/master/src%2Fbootstrap%2Fsrc%2Fcore%2Fconfig%2Fconfig.rs#L2984-L2990?

@jieyouxu jieyouxu assigned jieyouxu and unassigned Kobzol Mar 9, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Mar 9, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bootstrap test ci_rustc_if_unchanged_logic fails locally
5 participants
@onur-ozkan @rustbot @rust-log-analyzer @jieyouxu @Kobzol