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

extend the "if-unchanged" logic for compiler builds #131831

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Oct 17, 2024

Implements the first item from this tracking issue.

In short, we want to make "if-unchanged" logic to check for changes outside of certain allowed directories, and this PR implements that.

See #131658 for more context.

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 Oct 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

Comment on lines 74 to 75
/// Holds subpaths from [`RUSTC_IF_UNCHANGED_ALLOWED_PATHS`] that should not be allowed.
pub(crate) const RUSTC_IF_UNCHANGED_DENIED_SUBPATHS: &[&str] = &[
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to have this list, but without it, it will be extremely hard to manage RUSTC_IF_UNCHANGED_ALLOWED_PATHS since there will be hundreds of paths 🙁.

Copy link
Member

Choose a reason for hiding this comment

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

I think it really doesn't matter if there are a few more rebuilds than necessary. I'd remove this list here and significantly reduce the list above. I think I'd be fine if it only contained library.

Copy link
Member

Choose a reason for hiding this comment

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

If we have to maintain a list of paths, making it simple and obvious and handle the most important cases seems way more important than making it granular and perfect.

@bjorn3
Copy link
Member

bjorn3 commented Oct 17, 2024

Would it make sense to compute the set of files that may not be changed for download-rustc to work based on the .d files in build/host/stage0-rustc? This is what cargo uses for determining if a crate should be recompiled. It doesn't list the Cargo.toml files though. For the aggregated .d file generated by cargo in build/host/stage0-rustc/<host>/release/rustc-main.d I think it would make sense to add Cargo.toml there too. These aggregated .d files are meant for integration with external build systems AFAIK, which need to track Cargo.toml changes too.

":!CODE_OF_CONDUCT.md",
":!CONTRIBUTING.md",
":!COPYRIGHT",
":!Cargo.lock",
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

How?

Copy link
Member

Choose a reason for hiding this comment

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

if the dependencies of the compiler are updated, the compiler needs to be rebuilt

Copy link
Member Author

@onur-ozkan onur-ozkan Oct 17, 2024

Choose a reason for hiding this comment

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

Oh, I didn't consider (but I should) the small bumps that can happen without changing Cargo.toml files in compiler and library tree.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, there are so many things to consider or to not consider here that I think it makes the most sense to be super simple and only special case library as not rebuilding. in case anyone complains we can add more as needed

@onur-ozkan
Copy link
Member Author

@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 Oct 18, 2024
@onur-ozkan onur-ozkan force-pushed the improve-rustc-if-unchanged-logic branch from 2468461 to 135b903 Compare October 18, 2024 17:09
@onur-ozkan
Copy link
Member Author

RUSTC_IF_UNCHANGED_DENIED_SUBPATHS is removed to rely on allowed paths only. Also, "src/bootstrap" is not included in the allowed paths on purpose since it can affect compiler builds.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 18, 2024
@onur-ozkan onur-ozkan force-pushed the improve-rustc-if-unchanged-logic branch from 135b903 to 6e6625b Compare October 18, 2024 17:16
@onur-ozkan
Copy link
Member Author

Would it make sense to compute the set of files that may not be changed for download-rustc to work based on the .d files in build/host/stage0-rustc?

We are comparing against the nightly compiler, "stage0-rustc" wouldn't help.

@bors
Copy link
Contributor

bors commented Oct 19, 2024

☔ The latest upstream changes (presumably #131934) made this pull request unmergeable. Please resolve the merge conflicts.

@onur-ozkan onur-ozkan force-pushed the improve-rustc-if-unchanged-logic branch from 6e6625b to a140442 Compare October 20, 2024 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants