Unite bless environment variables under `RUSTC_BLESS` #113298

tgross35 · 2023-07-03T18:22:27Z

Currently, Clippy and Miri both use an environment variable to indicate that output should be blessed, but they use different variable names. In order to improve consistency, this patch applies the following changes:

Rename the variable MIRI_BLESS (as used in the Miri subtree) to RUST_BLESS
Rename the variable BLESS (as used in the Clippy subtree) to RUST_BLESS
Move emitting RUST_BLESS into prepare_cargo_test so it is always available (I need this for a WIP PR)

I prefer something like RUST_BLESS to BLESS just for a lower chance of conflict (not super common but other tools do use BLESS), but I can change it to whatever is preferred.

Original discussion: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/BLESS.20env.20var.3A.20rename.20to.20CLIPPY_BLESS

r? @oli-obk
cc @flip1995

rustbot · 2023-07-03T18:22:36Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

The Miri subtree was changed

cc @rust-lang/miri

src/tools/miri/test-cargo-miri/run-test.py

flip1995

I would name it RUSTC_BLESS, if you want a prefix. But then if you work in tool repos like Clippy or MIRI, you'll have to run RUSTC_BLESS=1 cargo test, which is a bit weird, because the tool is not rustc. I feel the same with just RUST_BLESS. But maybe that's just me 🤔

src/bootstrap/test.rs

tgross35 · 2023-07-03T18:42:52Z

I would name it RUSTC_BLESS, if you want a prefix. But then if you work in tool repos like Clippy or MIRI, you'll have to run RUSTC_BLESS=1 cargo test, which is a bit weird, because the tool is not rustc. I feel the same with just RUST_BLESS. But maybe that's just me 🤔

I did have the same thoughts. Any thoughts on RUSTBLESS maybe? To match RUSTFLAGS/RUSTDOCFLAGS (not that those are bootstrap-only...)

Anyhow I set it to RUSTC_BLESS for now

RalfJung · 2023-07-04T06:15:29Z

RUSTC_BLESS works for me. We anyway invoke this as "./miri bless" (or hopefully soon "./miri test --bless") so users never see the env var.

Manishearth · 2023-07-04T10:39:29Z

cc @jyn514 as our resident "bootstrap should be consistent dammit" person.

jyn514 · 2023-07-04T12:32:02Z

i think the clippy user experience is more important than making bootstrap's code simpler. bootstrap kind of fundamentally has to workaround quirks in the tools it invokes so i place a fairly low priority on removing the workarounds as long as they're reliable.

that said, the inconsistency i see from the user perspective is that bootstrap uses --bless and clippy uses an env variable. i don't think there's a way to solve that while still allowing clippy to use cargo test without a wrapper though, i definitely agree there's a lot of value in having cargo test work out of the box.

so my preference is probably to close this PR and only change env variables within rust-lang/rust itself to be consistent, not in any subtrees.

jyn514 · 2023-07-04T12:37:05Z

that said, the inconsistency i see from the user perspective is that bootstrap uses --bless and clippy uses an env variable. i don't think there's a way to solve that while still allowing clippy to use cargo test without a wrapper though, i definitely agree there's a lot of value in having cargo test work out of the box.

(oh, this just gave me the idea to get rid of TESTNAME and use normal cargo test -- foo by switching to a custom test harness like nexttest in clippy ...)

flip1995 · 2023-07-04T14:04:53Z

i think the clippy user experience is more important than making bootstrap's code simpler.

We're not married to those env vars, we just added them like 1 week ago. So changing them doesn't cause any harm. And I myself have no objective objections against RUSTC_BLESS (or similar), just that it subjectively feels weird to me. So don't block this PR on this.

that said, the inconsistency i see from the user perspective is that bootstrap uses --bless and clippy uses an env variable

That's what MIRI also does. But now in miri the env var is called MIRI_BLESS and in Clippy it's just called BLESS. The reason for this is how ui-test (not compiletest-rs!) works. In the Clippy repo itself we have a cargo bless alias. But not sure if that would work in bootstrap? Also that alias doesn't work when trying to compile with additional features. And I want to enable the internal Clippy feature in the rustc repo sooner or later.

RalfJung · 2023-07-04T14:24:04Z

From a user perspective, Miri doesn't use an env var, it uses ./miri bless. The fact that there exists an env var is an internal implementation detail, not visible to users.

tgross35 · 2023-07-04T17:52:05Z

I think at least the portion of this it at sets a commonly usable variable should be fine and needed (at least I use it in #112697). Clippy and Miri could always split off their own variables again if it ever turns out they need finer control - but it seems like there isn’t much objection to trying to use the same one as-is? At least until there is something better than the env solution, which sounds cleaner but also not necessarily close.

I’ll make it your call, I can adjust as needed

r? @jyn514

bors · 2023-07-05T09:50:17Z

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

jyn514 · 2023-07-05T12:35:22Z

i don't have time for reviews, sorry

r? bootstrap

blyxyas · 2023-07-05T15:01:50Z

BLESS just for a lower chance of conflict (not super common but other tools do use BLESS)

AFAIK, BLESS' intended use is as $ BLESS=true my_command .... So it gets removed as soon as my_command is over. In this case, collisions wouldn't be an issue (I hope no one is working on blessing a Rust test and a Facebook test at the same time 😅 )

bors · 2023-07-15T15:43:35Z

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

clubby789 · 2023-07-19T22:32:05Z

I don't have a strong opinion on the variable name to use

r? bootstrap

oli-obk · 2023-07-26T19:43:47Z

rustfmt now also has an env var for blessing: #114054

oli-obk

lgtm, and as you said, we can bikeshed in the future.

needs a rebase and rustfmt taken into the fold

rustbot · 2023-07-26T20:42:18Z

Some changes occurred in src/tools/cargo

cc @ehuss

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Currently, Clippy, Miri, Rustfmt, and rustc all use an environment variable to indicate that output should be blessed, but they use different variable names. In order to improve consistency, this patch applies the following changes: - Emit `RUSTC_BLESS` within `prepare_cargo_test` so it is always available - Change usage of `MIRI_BLESS` in the Miri subtree to use `RUSTC_BLESS` - Change usage of `BLESS` in the Clippy subtree to `RUSTC_BLESS` - Change usage of `BLESS` in the Rustfmt subtree to `RUSTC_BLESS` - Adjust the blessable test in `rustc_errors` to use this same convention - Update documentation where applicable Any tools that uses `RUSTC_BLESS` should check that it is set to any value other than `"0"`.

tgross35 · 2023-07-26T20:54:30Z

@oli-obk thanks for taking the potato, this should be in sync again. I noted in your linked PR that you check the variable is not set to 0, so I made that consistent everywhere.

Some changes occurred in src/tools/cargo

cc @ehuss

This was a submodule whoops, should be gone now

oli-obk · 2023-07-27T08:29:49Z

@bors r+

Thanks for the quick and thorough fixes!

bors · 2023-07-27T08:29:51Z

📌 Commit 9439e02 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-07-27T11:07:40Z

⌛ Testing commit 9439e02 with merge fda2f9d1cf87618de855581b4c56161c69a5ac67...

rust-log-analyzer · 2023-07-27T12:12:05Z

The job x86_64-mingw failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok

failures:

---- sys::windows::process::tests::test_thread_handle stdout ----
thread 'sys::windows::process::tests::test_thread_handle' panicked at 'assertion failed: res.unwrap().is_some()', library\std\src\sys\windows\process\tests.rs:48:5

failures:
    sys::windows::process::tests::test_thread_handle


test result: FAILED. 910 passed; 1 failed; 4 ignored; 0 measured; 0 filtered out; finished in 13.43s

error: test failed, to rerun pass `-p std --lib`
make: *** [Makefile:75: ci-mingw-bootstrap] Error 1
##[group]Clock drift check
  local time: Thu Jul 27 12:11:41 CUT 2023
  network time: Thu, 27 Jul 2023 12:11:41 GMT

bors · 2023-07-27T12:12:24Z

💔 Test failed - checks-actions

oli-obk · 2023-07-27T12:14:13Z

@bors retry windows process failure

bors · 2023-07-27T17:45:15Z

⌛ Testing commit 9439e02 with merge 0eb5efc...

bors · 2023-07-27T19:35:11Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 0eb5efc to master...

rust-timer · 2023-07-28T04:19:24Z

Finished benchmarking commit (0eb5efc): 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)	3.7%	[3.7%, 3.7%]	1
Regressions ❌ (secondary)	3.8%	[2.2%, 5.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.7%	[3.7%, 3.7%]	1

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: 654.227s -> 652.516s (-0.26%)

Unite bless environment variables under `RUST_BLESS` Currently, Clippy and Miri both use an environment variable to indicate that output should be blessed, but they use different variable names. In order to improve consistency, this patch applies the following changes: - Rename the variable `MIRI_BLESS` (as used in the Miri subtree) to `RUST_BLESS` - Rename the variable `BLESS` (as used in the Clippy subtree) to `RUST_BLESS` - Move emitting `RUST_BLESS` into `prepare_cargo_test` so it is always available (I need this for a WIP PR) --- I prefer something like `RUST_BLESS` to `BLESS` just for a lower chance of conflict (not super common but other tools [do use `BLESS`](https://grep.app/search?q=%22BLESS%22&case=true&words=true&filter[lang][0]=Text&filter[lang][1]=Rust&filter[lang][2]=Python&filter[lang][3]=C%2B%2B&filter[lang][4]=Markdown&filter[lang][5]=C&filter[lang][6]=JSON)), but I can change it to whatever is preferred. Original discussion: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/BLESS.20env.20var.3A.20rename.20to.20CLIPPY_BLESS r? `@oli-obk` cc `@flip1995`

rustbot assigned oli-obk Jul 3, 2023

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 Jul 3, 2023

tgross35 mentioned this pull request Jul 3, 2023

Add simple markdown formatting to rustc --explain output #112697

Merged

tgross35 commented Jul 3, 2023

View reviewed changes

src/tools/miri/test-cargo-miri/run-test.py Show resolved Hide resolved

flip1995 reviewed Jul 3, 2023

View reviewed changes

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

tgross35 force-pushed the update-bless-envs branch from e811857 to c8d0fda Compare July 3, 2023 18:41

rustbot assigned jyn514 and unassigned oli-obk Jul 4, 2023

rustbot assigned clubby789 and unassigned jyn514 Jul 5, 2023

tgross35 force-pushed the update-bless-envs branch from c8d0fda to d29052e Compare July 6, 2023 05:35

rustbot assigned Mark-Simulacrum and unassigned clubby789 Jul 19, 2023

oli-obk mentioned this pull request Jul 26, 2023

Split some functions with many arguments into builder pattern functions #114054

Merged

oli-obk approved these changes Jul 26, 2023

View reviewed changes

tgross35 force-pushed the update-bless-envs branch from d29052e to 1dff779 Compare July 26, 2023 20:42

tgross35 force-pushed the update-bless-envs branch from 1dff779 to ae451f9 Compare July 26, 2023 20:47

tgross35 force-pushed the update-bless-envs branch from ae451f9 to 9439e02 Compare July 26, 2023 20:54

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 Jul 27, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 27, 2023

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 Jul 27, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 27, 2023

bors merged commit 0eb5efc into rust-lang:master Jul 27, 2023

rustbot added this to the 1.73.0 milestone Jul 27, 2023

tgross35 deleted the update-bless-envs branch July 27, 2023 20:59

lqd mentioned this pull request Jul 28, 2023

Build the first LLVM without LTO in try builds #113779

Merged

tgross35 changed the title ~~Unite bless environment variables under RUST_BLESS~~ Unite bless environment variables under RUSTC_BLESS Apr 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unite bless environment variables under `RUSTC_BLESS` #113298

Unite bless environment variables under `RUSTC_BLESS` #113298

tgross35 commented Jul 3, 2023 •

edited

Loading

rustbot commented Jul 3, 2023

flip1995 left a comment

tgross35 commented Jul 3, 2023

RalfJung commented Jul 4, 2023 via email

Manishearth commented Jul 4, 2023

jyn514 commented Jul 4, 2023

jyn514 commented Jul 4, 2023

flip1995 commented Jul 4, 2023 •

edited

Loading

RalfJung commented Jul 4, 2023

tgross35 commented Jul 4, 2023

bors commented Jul 5, 2023

jyn514 commented Jul 5, 2023

blyxyas commented Jul 5, 2023 •

edited

Loading

bors commented Jul 15, 2023

clubby789 commented Jul 19, 2023

oli-obk commented Jul 26, 2023

oli-obk left a comment

rustbot commented Jul 26, 2023

tgross35 commented Jul 26, 2023

oli-obk commented Jul 27, 2023

bors commented Jul 27, 2023

bors commented Jul 27, 2023

rust-log-analyzer commented Jul 27, 2023

bors commented Jul 27, 2023

oli-obk commented Jul 27, 2023

bors commented Jul 27, 2023

bors commented Jul 27, 2023

rust-timer commented Jul 28, 2023

Unite bless environment variables under RUSTC_BLESS #113298

Unite bless environment variables under RUSTC_BLESS #113298

Conversation

tgross35 commented Jul 3, 2023 • edited Loading

rustbot commented Jul 3, 2023

flip1995 left a comment

Choose a reason for hiding this comment

tgross35 commented Jul 3, 2023

RalfJung commented Jul 4, 2023 via email

Manishearth commented Jul 4, 2023

jyn514 commented Jul 4, 2023

jyn514 commented Jul 4, 2023

flip1995 commented Jul 4, 2023 • edited Loading

RalfJung commented Jul 4, 2023

tgross35 commented Jul 4, 2023

bors commented Jul 5, 2023

jyn514 commented Jul 5, 2023

blyxyas commented Jul 5, 2023 • edited Loading

bors commented Jul 15, 2023

clubby789 commented Jul 19, 2023

oli-obk commented Jul 26, 2023

oli-obk left a comment

Choose a reason for hiding this comment

rustbot commented Jul 26, 2023

tgross35 commented Jul 26, 2023

oli-obk commented Jul 27, 2023

bors commented Jul 27, 2023

bors commented Jul 27, 2023

rust-log-analyzer commented Jul 27, 2023

bors commented Jul 27, 2023

oli-obk commented Jul 27, 2023

bors commented Jul 27, 2023

bors commented Jul 27, 2023

rust-timer commented Jul 28, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Unite bless environment variables under `RUSTC_BLESS` #113298

Unite bless environment variables under `RUSTC_BLESS` #113298

tgross35 commented Jul 3, 2023 •

edited

Loading

flip1995 commented Jul 4, 2023 •

edited

Loading

blyxyas commented Jul 5, 2023 •

edited

Loading