Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support by jieyouxu · Pull Request #131913 · rust-lang/rust

jieyouxu · 2024-10-19T02:25:05Z

Add {ignore,needs}-{rustc,std}-debug-assertions compiletest directives and retire the old {ignore,only}-debug directives. The old {ignore,only}-debug directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having {ignore,only}-debug will be very confusing.

cc @matthiaskrgr

Closes #123987.

r? bootstrap (or compiler tbh)

Mark-Simulacrum · 2024-10-20T14:04:29Z

tests/codegen/mem-replace-simple-type.rs

@@ -1,6 +1,7 @@
 //@ compile-flags: -O -C no-prepopulate-passes
 //@ only-x86_64 (to not worry about usize differing)
-//@ ignore-debug: precondition checks make mem::replace not a candidate for MIR inlining


Just to check: we have some lint or w/e that will fail CI if someone adds a test with ignore-debug as we merge this PR?

compiletest (currently) has an allowlist of known directives, I've removed ignore-debug from that so compiletest will error if someone writes ignore-debug (should be in the first commit) which will fail locally and in CI. (This is current stopgap solution before fixing how directives are handled)

See #131913 (comment)

It will fail looking like (I know it's not super user friendly)

Testing stage1 compiletest suite=ui mode=ui (x86_64-pc-windows-msvc) error: detected unknown compiletest test directive `only-debug` in X:\repos\rust\tests\ui\print_type_sizes\niche-filling.rs:13 errors encountered during EarlyProps parsing: X:\repos\rust\tests\ui\print_type_sizes\niche-filling.rs thread 'main' panicked at src\tools\compiletest\src\header.rs:68:13: errors encountered during EarlyProps parsing note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

src/bootstrap/src/core/build_steps/test.rs

jieyouxu · 2024-10-20T14:54:16Z

src/tools/compiletest/src/command-list.rs

    "ignore-coverage-run",
    "ignore-cross-compile",
-    "ignore-debug",
+    "ignore-debug-assertions-rustc",


Specifically here

rustbot · 2024-10-20T15:34:07Z

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

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

jieyouxu · 2024-10-20T15:43:34Z

Accompanying rustc-dev-guide PR: rust-lang/rustc-dev-guide#2101

jieyouxu · 2024-10-20T15:46:06Z

src/bootstrap/src/core/config/config.rs

-        debug_assertions: Option<bool> = "debug-assertions",
+        rustc_debug_assertions: Option<bool> = "debug-assertions",
        randomize_layout: Option<bool> = "randomize-layout",
-        debug_assertions_std: Option<bool> = "debug-assertions-std",
+        std_debug_assertions: Option<bool> = "debug-assertions-std",


I did not change the config.toml user-facing toml name.

jieyouxu · 2024-10-20T16:03:27Z

Changes since last review

Renamed compiletest/bootstrap fields/variables/flags to use the prefix form of {rustc,std}-debug-assertions. This does not change the config.toml option names of debug-assertions and debug-assertions-std.
Added a directives unit test for the {needs,ignore}-{rustc,std}-debug-assertions to check that they are recognized.
Updated the tests to use ignore-std-debug-assertions.

jieyouxu · 2024-10-20T17:36:11Z

@rustbot ready

jieyouxu · 2024-10-21T12:18:17Z

Rebased and split compiletest/bootstrap changes into two commits, but no functional changes.

@rustbot ready

…ive support And retire the old `only-debug` directive which was ambiguous and only for std debug assertions.

And rename local variables and field names in bootstrap from `debug_assertions{,_std}` -> `{rustc,std}-debug-assertions` to avoid confusion where applicable.

jieyouxu · 2024-10-31T09:34:36Z

Rebased, no changes.

jieyouxu · 2024-11-04T08:25:52Z

r? bootstrap

onur-ozkan

LGTM!

Only one question:

onur-ozkan · 2024-11-05T18:39:13Z

tests/codegen/mem-replace-big-type.rs

-//@ ignore-debug: precondition checks in ptr::read make them a bad candidate for MIR inlining
+//@ ignore-std-debug-assertions
+// Reason: precondition checks in ptr::read make them a bad candidate for MIR inlining


Can't the comment format be the same (without newline) as it was before? Or is it due to the tidy length check?

Oh it definitely can be, but it gets quite long and I found it a bit easier to read on a different line.

(and yes it would hit the tidy line length check lol)

onur-ozkan · 2024-11-07T18:59:27Z

@bors r+

bors · 2024-11-07T18:59:31Z

📌 Commit 0d5cc8e has been approved by onur-ozkan

It is now in the queue for this repository.

…onur-ozkan Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support Add `{ignore,needs}-{rustc,std}-debug-assertions` compiletest directives and retire the old `{ignore,only}-debug` directives. The old `{ignore,only}-debug` directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having `{ignore,only}-debug` will be very confusing. cc `@matthiaskrgr` Closes rust-lang#123987. r? bootstrap (or compiler tbh)

…onur-ozkan Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support Add `{ignore,needs}-{rustc,std}-debug-assertions` compiletest directives and retire the old `{ignore,only}-debug` directives. The old `{ignore,only}-debug` directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having `{ignore,only}-debug` will be very confusing. cc ``@matthiaskrgr`` Closes rust-lang#123987. r? bootstrap (or compiler tbh)

…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name) - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support) - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion) - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates) - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config) - rust-lang#132738 (Initialize channel `Block`s directly on the heap) - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting) - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax) r? `@ghost` `@rustbot` modify labels: rollup

…onur-ozkan Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support Add `{ignore,needs}-{rustc,std}-debug-assertions` compiletest directives and retire the old `{ignore,only}-debug` directives. The old `{ignore,only}-debug` directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having `{ignore,only}-debug` will be very confusing. cc ```@matthiaskrgr``` Closes rust-lang#123987. r? bootstrap (or compiler tbh)

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name) - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support) - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion) - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates) - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder) - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config) - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available) - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting) - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax) - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131913 - jieyouxu:only_debug_assertions, r=onur-ozkan Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support Add `{ignore,needs}-{rustc,std}-debug-assertions` compiletest directives and retire the old `{ignore,only}-debug` directives. The old `{ignore,only}-debug` directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having `{ignore,only}-debug` will be very confusing. cc ````@matthiaskrgr```` Closes rust-lang#123987. r? bootstrap (or compiler tbh)

…onur-ozkan Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support Add `{ignore,needs}-{rustc,std}-debug-assertions` compiletest directives and retire the old `{ignore,only}-debug` directives. The old `{ignore,only}-debug` directives were ambiguous because you could have std built with debug assertions but rustc not built with debug assertions or vice versa. If we want to support the use case of controlling test run based on if rustc was built with debug assertions, then having `{ignore,only}-debug` will be very confusing. cc ````@matthiaskrgr```` Closes rust-lang#123987. r? bootstrap (or compiler tbh)

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name) - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support) - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion) - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates) - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder) - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config) - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available) - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting) - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax) - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned Mark-Simulacrum Oct 19, 2024

Mark-Simulacrum reviewed Oct 20, 2024

View reviewed changes

jieyouxu commented Oct 20, 2024

View reviewed changes

jieyouxu force-pushed the only_debug_assertions branch from 30e1d8a to 2653a4a Compare October 20, 2024 15:34

jieyouxu mentioned this pull request Oct 20, 2024

Rename {ignore,only}-debug -> {ignore,needs}-{rustc,std}-debug-assertions rust-lang/rustc-dev-guide#2101

Merged

jieyouxu changed the title ~~Add {ignore,needs}-debug-assertions-{rustc,std} directive support~~ Add {ignore,needs}-{rustc,std}-debug-assertions directive support Oct 20, 2024

jieyouxu 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 20, 2024

jieyouxu commented Oct 20, 2024

View reviewed changes

jieyouxu force-pushed the only_debug_assertions branch from 2653a4a to 014191d Compare October 20, 2024 16:00

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 20, 2024

This comment was marked as resolved.

Sign in to view

jieyouxu 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 21, 2024

jieyouxu force-pushed the only_debug_assertions branch from 014191d to c46a1ca Compare October 21, 2024 09:58

This comment has been minimized.

Sign in to view

jieyouxu force-pushed the only_debug_assertions branch from c46a1ca to 2021e10 Compare October 21, 2024 10:46

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 21, 2024

This comment was marked as resolved.

Sign in to view

jieyouxu added 3 commits October 31, 2024 17:33

compiletest: add {ignore,needs}-{rustc,std}-debug-assertions direct…

65b986b

…ive support And retire the old `only-debug` directive which was ambiguous and only for std debug assertions.

bootstrap: pass --with-{rustc,std}-debug-assertions to compiletest

3870022

And rename local variables and field names in bootstrap from `debug_assertions{,_std}` -> `{rustc,std}-debug-assertions` to avoid confusion where applicable.

tests: ignore-debug -> ignore-std-debug-assertions

0d5cc8e

jieyouxu force-pushed the only_debug_assertions branch from 2021e10 to 0d5cc8e Compare October 31, 2024 09:34

rustbot assigned onur-ozkan and unassigned Mark-Simulacrum Nov 4, 2024

onur-ozkan approved these changes Nov 5, 2024

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 7, 2024

GuillaumeGomez mentioned this pull request Nov 7, 2024

Rollup of 8 pull requests #132747

Closed

GuillaumeGomez mentioned this pull request Nov 7, 2024

Rollup of 8 pull requests #132751

Closed

This was referenced Nov 8, 2024

Rollup of 10 pull requests #132753

Closed

Rollup of 10 pull requests #132756

Merged

bors merged commit 93e9ec0 into rust-lang:master Nov 8, 2024

rustbot added this to the 1.84.0 milestone Nov 8, 2024

jieyouxu deleted the only_debug_assertions branch November 8, 2024 07:06

Uh oh!

Conversation

jieyouxu commented Oct 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Oct 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Oct 20, 2024

Uh oh!

jieyouxu commented Oct 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Oct 20, 2024

Changes since last review

Uh oh!

jieyouxu commented Oct 20, 2024

Uh oh!

This comment was marked as resolved.

This comment has been minimized.

jieyouxu commented Oct 21, 2024

Uh oh!

This comment was marked as resolved.

jieyouxu commented Oct 31, 2024

Uh oh!

jieyouxu commented Nov 4, 2024

Uh oh!

onur-ozkan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Nov 7, 2024

Uh oh!

bors commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jieyouxu commented Oct 19, 2024 •

edited

Loading

jieyouxu Oct 20, 2024 •

edited

Loading

onur-ozkan left a comment •

edited

Loading