improve bootstrap change-tracking system #117815

onur-ozkan · 2023-11-11T13:21:07Z

This PR aims to improve how change-tracking system works for bootstrap changes by doing the followings:

Enforce embedding directive informations about the changes on bootstrap/src/lib.rs.
Give more informative change inputs on the terminal (improve bootstrap change-tracking system #117815 (comment)).
Avoid spamming the change informations(by reading and creating .last-warned-change-id under build output dir).

see the zulip conversation for more details: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/config.2Etoml.20change.20tracking

cc @RalfJung

rustbot · 2023-11-11T13:21:16Z

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

triagebot.toml

src/bootstrap/src/bin/main.rs

albertlarsan68 · 2023-11-11T13:38:35Z

I have a few questions:

Does x clean clear the file? I would prefer if it did.
Is the file created/updated only upon printing the second message? It would be bad if it was only shown once at the start, eclipsed by a few walls of texts, then never show again until the next time the field is updated.
Maybe add a config (off by default) to always show it?

Noratrieb · 2023-11-11T13:39:11Z

I think a problem with this system is that there are several audiences. First, there are rust-lang/rust developers. Then there are distros packaging Rust. These two audiences have very different needs. I would think that distros want to know about the all the breakage, while rust-lang/rust devs don't really care.
Maybe the message should look at the profile and only display the messages for profile = "dist"? Or maybe have two lists, those with less critical changes for distros and those with super important critical changes for everyone. Although I'm not sure whether super important critical changes for everyone even exist, or should ever exist.

onur-ozkan · 2023-11-11T13:59:09Z

// PR ID(to provide PR link for more input), log prefix, change summary
pub const CONFIG_CHANGE_HISTORY: &[(usize, &str, &str)] = &[
    (115898, "NOTE", "Implementation of this change-tracking system. Ignore this."),
    (116998, "INFO", "Removed android-ndk r15 support in favor of android-ndk r25b."),
    (117435, "INFO", "New option `rust.parallel-compiler` added to config.toml."),
    (116881, "WARNING", "Default value of `download-ci-llvm` was changed for `codegen` profile.")
];

Any thoughts on this(ignore the ugliness of the code)? This approach can automatically force PR authors to add change summaries.

@RalfJung in your use case, you will be able find what you are looking for directly on your terminal without needing to explore PR changes/descriptions. And if you need more input, you will be able to visit PR links still.

albertlarsan68 · 2023-11-11T14:03:19Z

Would be good if the severity was an Enum, so that different actions could be taken based on if it should be fatal or not (really breaking or just an information, show just once (at the end of the build preferably), or always)

RalfJung · 2023-11-11T14:26:30Z

I would expect distros to check the changelog. So the changelog should have a section on config.toml changes. I don't think this system needs to be concerned with distros at all.

Any thoughts on this(ignore the ugliness of the code)? This approach can automatically force PR authors to add change summaries.

Yeah, that seems nice!

onur-ozkan · 2023-11-11T15:04:08Z

src/bootstrap/src/bin/main.rs

+                msg.push_str(&format!("  [{}] {}\n", change.severity.to_string(), change.summary));
+                msg.push_str(&format!(
+                    "    - PR Link https://github.com/rust-lang/rust/pull/{}\n",
+                    change.change_id
+                ));


This change results this format:

Building bootstrap Compiling bootstrap v0.0.0 (/home/nimda/devspace/onur-ozkan/rust/src/bootstrap) Finished dev [unoptimized] target(s) in 4.85s There have been changes to x.py since you last updated: [INFO] New option `rust.parallel-compiler` added to config.toml. - PR Link https://github.com/rust-lang/rust/pull/117435 [INFO] Default value of `download-ci-llvm` was changed for `codegen` profile. - PR Link https://github.com/rust-lang/rust/pull/116881 note: to silence this warning, update `config.toml` to use `change-id = 116881` instead Building stage0 tool tidy (x86_64-unknown-linux-gnu)

src/bootstrap/src/lib.rs

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2023-11-12T06:14:50Z

Does x clean clear the file? I would prefer if it did.

Yes it does clear the file.

It would be bad if it was only shown once at the start, eclipsed by a few walls of texts, then never show again until the next time the field is updated.

We print these logs twice (2nd print is at the end of bootstrap) to ensure devs will see them.

Maybe add a config (off by default) to always show it?

As this option doesn't exists in bootstrap for old branches, builds will fail on recent-to-old checkouts.

Printing these logs once for per initialization (after x clean, or on the fresh repository, or after rm -rf build) is fine in my opinion.

src/bootstrap/src/lib.rs

Signed-off-by: onur-ozkan <work@onurozkan.dev>

As the .last-warned-change-id is only used for change tracking, we don't need to generate/write it outside of the tty. Otherwise, rust-analyzer could create this file, and developers wouldn't be able to see the bootstrap change alerts, assuming that they have already seen them. Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan · 2023-11-21T12:56:01Z

-kindly reminder ping- @albertlarsan68

albertlarsan68

LGTM, sorry for the delay

albertlarsan68 · 2023-11-23T10:18:59Z

Thanks for the PR!
@bors r+

bors · 2023-11-23T10:19:02Z

📌 Commit 9a6afd0 has been approved by albertlarsan68

It is now in the queue for this repository.

bors · 2023-11-23T12:54:50Z

⌛ Testing commit 9a6afd0 with merge 91f7f26...

bors · 2023-11-23T15:04:48Z

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 91f7f26 to master...

rust-timer · 2023-11-23T17:12:38Z

Finished benchmarking commit (91f7f26): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.6%	[3.7%, 5.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.6%	[3.7%, 5.6%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.441s -> 677.025s (0.09%)
Artifact size: 313.75 MiB -> 313.73 MiB (-0.01%)

…mulacrum Fix build on Solaris after rust-lang#117815.

…ertlarsan68 improve bootstrap change-tracking system This PR aims to improve how change-tracking system works for bootstrap changes by doing the followings: - Enforce embedding directive informations about the changes on `bootstrap/src/lib.rs`. - Give more informative change inputs on the terminal (rust-lang/rust#117815 (comment)). - Avoid spamming the change informations(by reading and creating `.last-warned-change-id` under build output dir). see the zulip conversation for more details: https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/config.2Etoml.20change.20tracking cc `@RalfJung`

rustbot assigned albertlarsan68 Nov 11, 2023

rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself 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 Nov 11, 2023

RalfJung reviewed Nov 11, 2023

View reviewed changes

triagebot.toml Outdated Show resolved Hide resolved

RalfJung reviewed Nov 11, 2023

View reviewed changes

src/bootstrap/src/bin/main.rs Outdated Show resolved Hide resolved

onur-ozkan force-pushed the update-change-tracking-impl branch 2 times, most recently from 6684b44 to 1903c98 Compare November 11, 2023 15:03

onur-ozkan commented Nov 11, 2023

View reviewed changes

onur-ozkan force-pushed the update-change-tracking-impl branch 2 times, most recently from f479034 to de2bc42 Compare November 11, 2023 15:14

onur-ozkan changed the title ~~print the change warnings once for per id~~ improve bootstrap change-tracking system Nov 11, 2023

onur-ozkan mentioned this pull request Nov 11, 2023

bootstrap: please document why a config change requires migration #117809

Closed

Kobzol reviewed Nov 11, 2023

View reviewed changes

src/bootstrap/src/lib.rs Show resolved Hide resolved

print the change warnings once for per id

056d4e4

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the update-change-tracking-impl branch from a4f05c6 to 426a2f8 Compare November 11, 2023 15:50

This comment has been minimized.

Sign in to view

RalfJung reviewed Nov 12, 2023

View reviewed changes

src/bootstrap/src/lib.rs Outdated Show resolved Hide resolved

onur-ozkan added 2 commits November 12, 2023 13:21

refactor change-tracking implementation

12ab8c6

Signed-off-by: onur-ozkan <work@onurozkan.dev>

remove .last-warned-change-id on x clean

d3d3eb9

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the update-change-tracking-impl branch from 89829bb to 009e165 Compare November 12, 2023 10:26

onur-ozkan added 2 commits November 12, 2023 13:31

bootstrap: add doc-comments for ChangeSeverity

06f6cd9

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the update-change-tracking-impl branch from 009e165 to 9a6afd0 Compare November 12, 2023 10:31

onur-ozkan mentioned this pull request Nov 18, 2023

deprecate if-available value of download-ci-llvm #117813

Merged

albertlarsan68 approved these changes Nov 23, 2023

View reviewed changes

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 Nov 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2023

bors merged commit 91f7f26 into rust-lang:master Nov 23, 2023
11 checks passed

rustbot added this to the 1.76.0 milestone Nov 23, 2023

psumbera added a commit to psumbera/rust that referenced this pull request Nov 24, 2023

Fix build on Solaris after rust-lang#117815.

879aff9

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2023

Auto merge of rust-lang#118235 - psumbera:bootstrap-main-t, r=Mark-Si…

2a48155

…mulacrum Fix build on Solaris after rust-lang#117815.

onur-ozkan deleted the update-change-tracking-impl branch January 27, 2024 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve bootstrap change-tracking system #117815

improve bootstrap change-tracking system #117815

onur-ozkan commented Nov 11, 2023 •

edited

Loading

rustbot commented Nov 11, 2023

albertlarsan68 commented Nov 11, 2023

Noratrieb commented Nov 11, 2023

onur-ozkan commented Nov 11, 2023 •

edited

Loading

albertlarsan68 commented Nov 11, 2023

RalfJung commented Nov 11, 2023

onur-ozkan Nov 11, 2023

This comment has been minimized.

onur-ozkan commented Nov 12, 2023

onur-ozkan commented Nov 21, 2023

albertlarsan68 left a comment

albertlarsan68 commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

improve bootstrap change-tracking system #117815

improve bootstrap change-tracking system #117815

Conversation

onur-ozkan commented Nov 11, 2023 • edited Loading

rustbot commented Nov 11, 2023

albertlarsan68 commented Nov 11, 2023

Noratrieb commented Nov 11, 2023

onur-ozkan commented Nov 11, 2023 • edited Loading

albertlarsan68 commented Nov 11, 2023

RalfJung commented Nov 11, 2023

onur-ozkan Nov 11, 2023

Choose a reason for hiding this comment

This comment has been minimized.

onur-ozkan commented Nov 12, 2023

onur-ozkan commented Nov 21, 2023

albertlarsan68 left a comment

Choose a reason for hiding this comment

albertlarsan68 commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

bors commented Nov 23, 2023

rust-timer commented Nov 23, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

onur-ozkan commented Nov 11, 2023 •

edited

Loading

onur-ozkan commented Nov 11, 2023 •

edited

Loading