Add custom "Satisfies" terms #330

apiraino · 2024-04-09T15:54:07Z

cargo-bisect-rustc prints "Yes" or "No" on whether or not a build
satisfies the condition that it is looking for (default: Yes =
reproduces regression, No = does not reproduce the regression). However,
this terminology is either confusing or backwards depending on what is
being tested.

For example, one can use one of --regress options (f.e. --regress success) to find when a regression was fixed. In that sense, the "old"
is "regression found" and the "new" is "regression fixed", which is
backwards from the normal behavior.

Taking inspiration from git bisect, we are introducing custom terms for
Satisfies. This is implemented with 2 new cli options:

--term-old, will apply for Satisfy::Yes (condition requested is matched)
--term-new, will apply for Satisfy::No (condition requested is NOT matched)

This will allow the user to specify their own wording. Then, the
--regress option could set the defaults for those terms appropriate for
the regression type.

Fixes #316

ehuss · 2024-04-09T19:22:54Z

Nice! I'm not sure if there is more work to do since this is in a Draft state.

One initial comment I have is that it seems like maybe the terms are backwards. My expectation is that this would work pretty much the same as git bisect where the default assumption is that you are looking for when a regression starts (and that is also cargo-bisect-rustc's default). With git bisect, the term "good" is synonymous with "old" which means that it successfully compiles. The term "bad" is synonymous with "new" which means that it fails to compile.

This PR seems to treat "good" meaning it found the regression, which is the opposite of git's behavior.

One of the reasons I suggested in #316 to use the old/new terminology is so that it is less ambiguous with good/bad. Good/bad could mean "good, I found the regression" or "good, it successfully compiles", which are opposites. I think with old/new, there is no ambiguity since there is only one direction of time.

apiraino · 2024-04-10T16:41:34Z

... since there is only one direction of time

@ehuss Hmm you're right. When I look at it from that point of view it makes def. more sense, sorry for not applying cleanly your suggestion.

Now it should work as expected. Can you check it one more time when you have a sec? Thanks

The draft state was because this patch stlll needs some documentation (I will update the book) and the tests are giving me headaches (the trycmd crate is not updating the .h .out files - looking into it...)

apiraino · 2024-04-15T13:40:12Z

@ehuss ok now tests are passing.

Feel free to review when you have a sec. I especially want to be sure the documentation explains the logic correctly and clearly.

thanks

apiraino · 2024-04-28T11:53:00Z

r? @ehuss

when you have a chance

tests/cmd/start-in-future.stderr

ehuss · 2024-04-29T19:34:49Z

src/least_satisfying.rs

+        let msg_yes = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
+            // YES: condition satisfied, reproduces the regression
+            term_old
+        } else {
+            // NO: condition not satisfied, does not reproduce the regression
+            term_new
+        };
+
+        let msg_no = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
+            // YES: condition satisfied, compiles successfully, does not reproduce the regression
+            term_new
+        } else {
+            // NO: condition not satisfied, the regression still reproduces
+            term_old
+        };


These messages still seem to be backwards to me.

For example, if I run cargo-bisect-rustc --term-old=compiles --term-new=errors, it prints "errors" when it successfully compiles the old code and "compiles" when it fails to compile the new code.

Also, not a blocker, but I think it would be nice to customize the default old/new terms based on the RegressOn setting. For example:

RegressOn Old New

Error "Successfully compiles" "Compile error"

Success "Compile error" "Successfully compiles"

Ice "Did not ICE" "Found ICE"

NonIce "Found ICE" "Did not ICE"

NonError "Compile error (no ICE)" "Successfully compiles"

That way there is a clear indication of what cbr observed.

@ehuss I have restarted this patchset from scratch. Flipped the previous logic wrong and corrected/clarified the commit message. Are we there?

if the new commit looks good, I will update in a second commit documentation and tests

thanks

The new default messages are very nice and will implement them in a second patch

Looks good, thanks!

OK, so @ehuss now what's the next step? Merge? Or is there anything else to do in this PR?

EDIT: oh, I'll fix the tests

cargo-bisect-rustc prints "Yes" or "No" on whether or not a build satisfies the condition that it is looking for (default: Yes = reproduces regression, No = does not reproduce the regression). However, this terminology is either confusing or backwards depending on what is being tested. For example, one can use one of `--regress` options (f.e. `--regress success`) to find when a regression was fixed. In that sense, the "old" is "regression found" and the "new" is "regression fixed", which is backwards from the normal behavior. Taking inspiration from git bisect, we are introducing custom terms for Satisfies. This is implemented with 2 new cli options: --term-old, will apply for Satisfy::Yes (condition requested is matched) --term-new, will apply for Satisfy::No (condition requested is NOT matched) This will allow the user to specify their own wording. Then, the --regress option could set the defaults for those terms appropriate for the regression type. Fixes rust-lang#316

ehuss

Thanks!

The default values for --term-old and --term-new could use some reasonable defaults, here the mapping: | RegressOn | Old | New | | --------------------------------------------------------------- | | Error | "Successfully compiles" | "Compile error" | | Success | "Compile error" | "Successfully compiles" | | Ice | "Did not ICE" | "Found ICE" | | NonIce | "Found ICE" | "Did not ICE" | | NonError | "Compile error (no ICE)" | "Successfully compiles" | Suggested here: rust-lang#330 (comment)

apiraino force-pushed the custom-satisfy-terms branch 3 times, most recently from bd38135 to e66020b Compare April 10, 2024 16:36

apiraino marked this pull request as ready for review April 10, 2024 16:54

apiraino force-pushed the custom-satisfy-terms branch 2 times, most recently from 8dd0ac0 to ef37a65 Compare April 15, 2024 13:34

ehuss reviewed Apr 29, 2024

View reviewed changes

apiraino force-pushed the custom-satisfy-terms branch 2 times, most recently from 1292a3b to 556a00b Compare May 3, 2024 15:23

apiraino closed this May 3, 2024

apiraino force-pushed the custom-satisfy-terms branch from 556a00b to 0859f47 Compare May 3, 2024 15:54

apiraino reopened this May 3, 2024

apiraino force-pushed the custom-satisfy-terms branch from dee4e39 to ba7eb63 Compare May 14, 2024 08:01

ehuss approved these changes May 14, 2024

View reviewed changes

ehuss merged commit e75f187 into rust-lang:master May 14, 2024
4 checks passed

apiraino deleted the custom-satisfy-terms branch May 22, 2024 08:40

apiraino mentioned this pull request May 22, 2024

Improve terms defaults #337

Closed

ehuss mentioned this pull request May 30, 2024

Add default regression terms based on --regress option. #339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom "Satisfies" terms #330

Add custom "Satisfies" terms #330

apiraino commented Apr 9, 2024 •

edited

Loading

ehuss commented Apr 9, 2024

apiraino commented Apr 10, 2024 •

edited

Loading

apiraino commented Apr 15, 2024

apiraino commented Apr 28, 2024

ehuss Apr 29, 2024

apiraino May 3, 2024

apiraino May 3, 2024

ehuss May 10, 2024

apiraino May 14, 2024 •

edited

Loading

ehuss left a comment

RegressOn	Old	New
Error	"Successfully compiles"	"Compile error"
Success	"Compile error"	"Successfully compiles"
Ice	"Did not ICE"	"Found ICE"
NonIce	"Found ICE"	"Did not ICE"
NonError	"Compile error (no ICE)"	"Successfully compiles"

Add custom "Satisfies" terms #330

Add custom "Satisfies" terms #330

Conversation

apiraino commented Apr 9, 2024 • edited Loading

ehuss commented Apr 9, 2024

apiraino commented Apr 10, 2024 • edited Loading

apiraino commented Apr 15, 2024

apiraino commented Apr 28, 2024

ehuss Apr 29, 2024

Choose a reason for hiding this comment

apiraino May 3, 2024

Choose a reason for hiding this comment

apiraino May 3, 2024

Choose a reason for hiding this comment

ehuss May 10, 2024

Choose a reason for hiding this comment

apiraino May 14, 2024 • edited Loading

Choose a reason for hiding this comment

ehuss left a comment

Choose a reason for hiding this comment

apiraino commented Apr 9, 2024 •

edited

Loading

apiraino commented Apr 10, 2024 •

edited

Loading

apiraino May 14, 2024 •

edited

Loading