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

RFC: Print Kani version #2571

Closed
wants to merge 4 commits into from

Conversation

adpaco-aws
Copy link
Contributor

@adpaco-aws adpaco-aws commented Jun 27, 2023

Description of changes:

This RFC proposes printing the Kani version at the beginning of a run.

Rendered version available here.

Resolved issues:

Resolves #2570

Testing:

  • How is this change tested? N/A

  • Is this a refactor change? N/A

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@adpaco-aws adpaco-aws added the T-RFC Label RFC PRs and Issues label Jun 27, 2023
@adpaco-aws adpaco-aws requested a review from a team as a code owner June 27, 2023 19:06
@adpaco-aws adpaco-aws changed the title RFC: Kani version RFC: Print Kani version Jun 27, 2023
Copy link
Contributor

@JustusAdam JustusAdam left a comment

Choose a reason for hiding this comment

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

I like this idea. It's especially nice for cases where Kani is run programmatically, e.g. from the extension which might cause a different Kani version to run (because $PATH sucks).

## User Experience

The first line printed in any Kani invocation (either through `kani` or `cargo kani`, and regardless of subcommands) will inform users of the version.
The behavior will be extended for development versions, where it'll print the short hash of the HEAD commit in addition to the version.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd even include the branch the commit came from (makes it easier to find) in the development version.


In my experience, we should print the version because users and developers use text-based log files containing Kani's output to discuss verification results.
In some cases, we've had to "calculate" the Kani version from the CBMC versions appearing in the log.
But we shouldn't need to in the first place.
Copy link
Contributor

Choose a reason for hiding this comment

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

I we care strongly about being accommodating here, the printing of the version could be a feature flag that is on by default so people can toggle it off in a custom build.

Another option would be to check (e.g. atty) whether the user is running a TTY, and only print the version if it isn't a tty.

- Prefix the version with `v` (so the version gets printed as `v0.29.0`, for example).
- Just print `Kani Rust Verifier <version>`, nothing else.

These are low-level details which I'd love to discuss with you all.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel super strongly about Launching or v, though one might argue they are redundant.
However it might makes sense to print whether it is running the cargo plugin or the standalone Kani. Maybe Kani Rust Verifier (Cargo Plugin) 1.29.0 and vice versa with standalone.

## Open questions

I'm hoping that we can answer the following questions during the RFC:
1. Do we want to print Kani's version?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Especially because of the VSCode extension.

```

where `<version>` is the version of Kani under use, which follows the semantic versioning format `MAJOR.MINOR.PATCH`,
and `<commit>` is the short hash (i.e., 7 hexadecimal digits with format `hhhhhhh`) of the `HEAD` commit.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted to be super extra we could also print (dirty) if your worktree has uncommitted changes or add a hash for all files in src 😂

@JustusAdam
Copy link
Contributor

Also I think the version should be included in the header when printing --help

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

IMHO, I don't think we need an RFC for this change. It's a nice improvement to the output Kani. But it's a fairly simple change that shouldn't impact current and future users.

The first line to be printed will be:

```
Launching the Kani Rust Verifier <version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the "the" here?

@adpaco-aws
Copy link
Contributor Author

IMHO, I don't think we need an RFC for this change. It's a nice improvement to the output Kani. But it's a fairly simple change that shouldn't impact current and future users.

Agreed, I'll close this RFC and we can continue the discussion in the (to-be-developed) PR. Thanks @JustusAdam for the suggestions, I'll consider them for the implementation!

@adpaco-aws adpaco-aws closed this Jun 28, 2023
@adpaco-aws adpaco-aws mentioned this pull request Jul 23, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-RFC Label RFC PRs and Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output - Kani version
3 participants