Skip to content

Conversation

@zhassan-aws
Copy link
Contributor

Description of changes:

Install Kissat as part of the Kani setup, and include it in the Kani release bundle.

This is required by a PR that adds a kani::solver attribute (whic will follow shortly).

Keeping this as a draft until we integrate a CBMC version that fixes #1962.

Resolved issues:

Towards #1656

Related RFC:

Optional #ISSUE-NUMBER.

Call-outs:

Testing:

  • How is this change tested? A test is included in a PR that will follow shortly.

  • Is this a refactor change? No

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.

@rahulku
Copy link
Contributor

rahulku commented Jan 6, 2023

do we need to include this in the dependencies also?

@zhassan-aws
Copy link
Contributor Author

do we need to include this in the dependencies also?

Good point. Added.

@celinval
Copy link
Contributor

celinval commented Jan 6, 2023

Can we also check the expected version as part of the regression script?

For the follow up PR, can you also please add a test that uses kissat as part of the bundle tests.

@zhassan-aws
Copy link
Contributor Author

The follow-up PR is #2088. I'll make sure to add a bundle test to it.

@zhassan-aws
Copy link
Contributor Author

zhassan-aws commented Jan 9, 2023

Can we also check the expected version as part of the regression script?

Added a check.

For the follow up PR, can you also please add a test that uses kissat as part of the bundle tests.

I added a test to this PR since #2088 needs some work. The test is invoked using --enable-unstable --cbmc-args --external-sat-solver kissat. The test will fail though until #1962 is fixed.

@zhassan-aws zhassan-aws marked this pull request as ready for review January 20, 2023 18:05
@zhassan-aws zhassan-aws requested a review from a team as a code owner January 20, 2023 18:05
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.

I suggested a few little improvements to make our lives easier, but not a big deal if you don't do that as part of this PR. But something for us to keep in mind. Thanks!

# To enable "unsound_experimental features, run as follows:
# `KANI_ENABLE_UNSOUND_EXPERIMENTS=1 scripts/kani-regression.sh`

# Check if kissat has the minimum required version
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move this to its own check_kissat_version? Or even augment the CBMC to include the kissat version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to a separate script. Didn't feel it should be part of the CBMC one. Perhaps as a cleanup, we can create a single script, e.g. check_dep_versions that calls the other scripts.

./scripts/setup/ubuntu/install_deps.sh
./scripts/setup/ubuntu/install_cbmc.sh
./scripts/setup/ubuntu/install_viewer.sh
./scripts/setup/install_kissat.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the install_deps.sh to invoke all the other installation scripts? We can keep the other scripts since they are handy to update versions, but this list is starting to grow too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this in a follow-up PR.

fi


# Kissat release
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check if the version is already installed first?

Copy link
Contributor Author

@zhassan-aws zhassan-aws Jan 20, 2023

Choose a reason for hiding this comment

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

Good idea. Done.

@@ -156,6 +156,7 @@ jobs:
docker run -w /tmp/kani/tests/cargo-kani/simple-lib $tag cargo kani
Copy link
Contributor

Choose a reason for hiding this comment

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

These should also be a separate script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do this one as well in a follow-up PR.

@zhassan-aws zhassan-aws merged commit 326b35e into model-checking:main Jan 21, 2023
@zhassan-aws zhassan-aws deleted the integrate-kissat branch January 21, 2023 00:24
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.

Verification crashes or gives wrong answer when using external SAT solver

3 participants