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

Add two options, /quiet and /silent #694

Merged
merged 4 commits into from
Feb 20, 2023

Conversation

atomb
Copy link
Collaborator

@atomb atomb commented Feb 15, 2023

The /quiet option suppresses output other than warnings and errors. The /silent option suppresses all output except the exit code.

The /quiet option suppresses output other than warnings and errors. The
/silent option suppresses all output except the exit code.
Copy link

@davidcok davidcok left a comment

Choose a reason for hiding this comment

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

LGTM, aside from my comments, but I'm not familiar with boogie code, so another review would be appropriate.

@@ -56,6 +64,10 @@ public virtual void ErrorWriteLine(TextWriter tw, string format, params object[]
public virtual void AdvisoryWriteLine(TextWriter output, string format, params object[] args)
{
Contract.Requires(format != null);
if (Options.Verbosity < CoreOptions.VerbosityLevel.Quiet) {

Choose a reason for hiding this comment

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

Elsewhere you test with == against Silent. Choose one consistent method of comparison.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,7 @@
// RUN: %parallel-boogie -silent "%s" > "%t"

Choose a reason for hiding this comment

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

Does boogie have an exit code to test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought it did, but apparently not! So -silent is not super useful for Boogie alone. The internal option it controls is useful for clients of Boogie as a library, like Dafny, however.

Choose a reason for hiding this comment

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

How does dafny know whether boogie is successful or not? I presume it makes some API call rather than spawning a command-line invocation.

I suggest you add an issue for boogie to return a proper exit code (though it is not an issue for me) like all good citizens should

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the exit code typically spit out on the console along with other output?

It would be great if we could add the standard story for exit code to Boogie also.

Choose a reason for hiding this comment

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

In dafny testing, we write things like %exits-with 1 %parallel-boogie ...
to check the output exit code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dafny uses Boogie as a library, so it inspects the verification results directly. I added #695 to make Boogie emit a non-zero exit code when verification fails, like Dafny does.

Boogie doesn't set a non-zero exit code on verification errors.
Copy link
Collaborator

@keyboardDrummer keyboardDrummer left a comment

Choose a reason for hiding this comment

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

Could you say something about the use-case for this change? I'm guessing it was made in support of Dafny. However, I'm surprised this would affect Dafny, because it works through the ConsolePrinter, a class that Dafny will override anyways.

From Boogie's perspective, it seems like a reasonable change, but as you commented the silent option can't affectively be used right now. Should we fix that?


More generally, I wonder whether Dafny should allow Boogie to write anything to the Console, or whether Dafny should take full ownership of its stdio, just like it new takes full ownership of its options.

I'm guessing that by default Dafny should not allow Boogie to write anything to stdio, and rely purely on an C# API provided by Boogie to get data that it then uses to write output. If Dafny is configured to show debug output, then it could show debug output from Boogie as well.

@atomb
Copy link
Collaborator Author

atomb commented Feb 17, 2023

Could you say something about the use-case for this change? I'm guessing it was made in support of Dafny. However, I'm surprised this would affect Dafny, because it works through the ConsolePrinter, a class that Dafny will override anyways.

The key use case for this is in testing, particularly when testing manually. Running with /silent and seeing no output is an indication of no internal failures (regardless of the status of verification).

Dafny extends ConsolePrinter, but doesn't override these methods (though it could). My inclination is that it's nice to have Dafny and Boogie behave the same way when it comes to printing errors, warnings, and verification summaries, and therefore to have this in Boogie.

From Boogie's perspective, it seems like a reasonable change, but as you commented the silent option can't affectively be used right now. Should we fix that?

As I mentioned above, I think there still is a useful role for /silent even with the current exit code behavior. Addressing #695 would make it more useful, but I'm happy to leave that as a separate task.

More generally, I wonder whether Dafny should allow Boogie to write anything to the Console, or whether Dafny should take full ownership of its stdio, just like it new takes full ownership of its options.

I'm guessing that by default Dafny should not allow Boogie to write anything to stdio, and rely purely on an C# API provided by Boogie to get data that it then uses to write output. If Dafny is configured to show debug output, then it could show debug output from Boogie as well.

This seems like a reasonable approach, on the Dafny side. :) I don't think the answer will affect what we do with Boogie right now, though.

@davidcok
Copy link

I like tools that have a quiet option for scripting. This just makes dafny fit in well -- plus I've heard requests for an option to shut off info message.

Completely silent is a rarer need, but I've used it in CI on other projects.

@keyboardDrummer keyboardDrummer enabled auto-merge (squash) February 20, 2023 11:46
@keyboardDrummer
Copy link
Collaborator

Could you say something about the use-case for this change? I'm guessing it was made in support of Dafny. However, I'm surprised this would affect Dafny, because it works through the ConsolePrinter, a class that Dafny will override anyways.

The key use case for this is in testing, particularly when testing manually. Running with /silent and seeing no output is an indication of no internal failures (regardless of the status of verification).

Dafny extends ConsolePrinter, but doesn't override these methods (though it could). My inclination is that it's nice to have Dafny and Boogie behave the same way when it comes to printing errors, warnings, and verification summaries, and therefore to have this in Boogie.

From Boogie's perspective, it seems like a reasonable change, but as you commented the silent option can't affectively be used right now. Should we fix that?

As I mentioned above, I think there still is a useful role for /silent even with the current exit code behavior. Addressing #695 would make it more useful, but I'm happy to leave that as a separate task.

More generally, I wonder whether Dafny should allow Boogie to write anything to the Console, or whether Dafny should take full ownership of its stdio, just like it new takes full ownership of its options.
I'm guessing that by default Dafny should not allow Boogie to write anything to stdio, and rely purely on an C# API provided by Boogie to get data that it then uses to write output. If Dafny is configured to show debug output, then it could show debug output from Boogie as well.

This seems like a reasonable approach, on the Dafny side. :) I don't think the answer will affect what we do with Boogie right now, though.

Makes sense, thanks.

@keyboardDrummer keyboardDrummer merged commit fb4e7ef into boogie-org:master Feb 20, 2023
@atomb atomb deleted the quiet-silent branch January 4, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants