Harness output individual files #3360

Alexander-Aghili · 2024-07-19T05:02:52Z

Resolves #3356

Changes allow for directory with individual output of files named by the full harness name. --output-into-files command line argument will allow for placing all output of individual harnesses into files named by the full harness pretty_name. The output directory is either --target-dir or a hard coded default: "kani_output" directory. (These can be changed if there is a better interface). Still prints output to std::out exactly as previous.
Previously, all output was placed into std::out. This will allow for some control over output. It will also enable easier parsing of harness output.
Only solved Harness Output in Individual Files #3356 but could be expanded to include more features.
Ran manually to check the flags and output behaved as expected. Indeed:
--output-into-files enabled will place output into individual files, disabled will not
--output-terse will create terse output to command line, regular output to individual files if --output-into-files is enabled.
--target-dir will place output into target-dir directory when --output-into-files is enabled, and will not place file output when disabled.

Let me know if you need specific explanations of code segments, a clean list of all the testing configurations, or any feature enhancement for greater configuration options.

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

…input and target directory

celinval · 2024-07-19T20:54:52Z

kani-driver/src/harness_runner.rs

+ }
+ }
+
+ fn get_target_dir(&self) -> String {


Can you please rename this to result_output_dir to separate these files from the other artifacts that Kani may generate? Also, can you return a Result<PathBuf> instead?

celinval · 2024-07-19T21:06:34Z

kani-driver/src/harness_runner.rs

+ self.args.target_dir
+ .clone()
+ .map_or_else(|| "./kani_output".to_string(), |dir| {
+ format!("{}/", dir.into_os_string().into_string().unwrap())
+ })


Maybe something like:

Suggested change

self.args.target_dir

.clone()

.map_or_else(|| "./kani_output".to_string(), |dir| {

format!("{}/", dir.into_os_string().into_string().unwrap())

})

let target_dir = self.args.target_dir

.clone()

.map_or_else(|| current_dir(), |dir| Ok(dir))?;

let outdir = target_dir.join("kani_results");

fs::create_dir_all(&outdir)?; // This is a no-op if directory exists.

Okay, I will get to fixing this on late Sunday or Monday as I am away this weekend. Thanks for the review!

celinval · 2024-07-20T18:33:10Z

kani-driver/src/args/mod.rs

@@ -275,6 +275,10 @@ pub struct VerificationArgs {
 )]
 pub synthesize_loop_contracts: bool,

+ //Harness Output into individual files
+ #[arg(long, hide_short_help = true, requires("enable_unstable"))]


I forgot to mention. We are deprecating enable_unstable, so can you please make sure the new feature can be enabled via -Z unstable-options instead?

Hi there, is there any example of how to do that?

Yes, unfortunately this cannot be done with clap directly. Here is an example:

kani/kani-driver/src/args/mod.rs

Lines 632 to 640 in 7ad4d1c

if self.coverage

&& !self.common_args.unstable_features.contains(UnstableFeature::LineCoverage)

{

return Err(Error::raw(

ErrorKind::MissingRequiredArgument,

"The `--coverage` argument is unstable and requires `-Z \

line-coverage` to be used.",

));

}

In this case, you can use UnstableFeature::UnstableOptions.

jaisnan · 2024-07-21T12:50:52Z

This output format is untested as of this PR. I'd like to see one regression test added at least. This isn't a blocking request, but likely something we want to add immediately, maybe in a follow up PR.

Alexander-Aghili · 2024-07-22T05:45:23Z

This output format is untested as of this PR. I'd like to see one regression test added at least. This isn't a blocking request, but likely something we want to add immediately, maybe in a follow up PR.

I agree that tests should be added. I'd be fine adding it as a component of this PR or as a separate one, whichever is easier for you guys.

Alexander-Aghili · 2024-07-26T00:28:50Z

Hey guys, sorry I haven't gotten to this in a minute. I was testing this on a server used by my research group, but my laptop's SSD failed with my ssh key so I can't really access the server at the moment. Should be fixed in a couple days. Thanks for the patience.

Alexander-Aghili · 2024-07-31T20:32:15Z

Okay I tested it and added the -Z --unstable-options instead of --enable-unstable. Do we still want to add tests as a part of this PR?

jaisnan · 2024-07-31T20:43:23Z

Okay I tested it and added the -Z --unstable-options instead of --enable-unstable. Do we still want to add tests as a part of this PR?

I'm okay without a regression for this specific output format since its a really special case and testing will involve some work. I can take that up in a follow up PR. You might want to do the following though (in order of priorities),

Required :-

Fix existing regression tests that are failing
Add comments on the functions related to processing the output.
Format the code with cargo fmt and run cargo clippy on it once.

Optionally

Create an issue to test the new output format.

Alexander-Aghili · 2024-08-01T17:59:33Z

Hey there, I've done most of it but I ran into this issue when running regression tests:
ERROR: CBMC version is 5.89.0, expected at least 5.95.0
How can I upgrade the CBMC version?

artemagvanian · 2024-08-01T18:02:19Z

Hey there, I've done most of it but I ran into this issue when running regression tests: ERROR: CBMC version is 5.89.0, expected at least 5.95.0 How can I upgrade the CBMC version?

You can re-run the script responsible for installing the dependencies to install the appropriate CBMC version. Here is a link to the wiki page that explains what exactly to run: https://model-checking.github.io/kani/build-from-source.html#dependencies.

zhassan-aws · 2024-08-01T18:02:52Z

If you're using a Mac, run:

./scripts/setup/macos/install_cbmc.sh

If you're using Ubuntu, run:

./scripts/setup/ubuntu/install_cbmc.sh

Alexander-Aghili · 2024-08-04T17:54:01Z

I don't want to be the bottleneck on this but in short, I am having a difficult time upgrading because of permissions issues on my remote server, and thus, if others want to try and fix the regression tests, then that is fine as well. Otherwise I am going to have to spend some time fixing the issue on the server I do my development work on and I'm out of town at the moment.

kani-driver/src/harness_runner.rs

jaisnan · 2024-08-14T21:47:08Z

I don't want to be the bottleneck on this but in short, I am having a difficult time upgrading because of permissions issues on my remote server, and thus, if others want to try and fix the regression tests, then that is fine as well. Otherwise I am going to have to spend some time fixing the issue on the server I do my development work on and I'm out of town at the moment.

Hi @Alexander-Aghili, the regression tests should pass now after the fix from @celinval. You should be able to resume development if you have the time to add tests. If not, please let us know and we can take on the task of adding a test on this PR 😄

Alexander-Aghili · 2024-08-20T00:00:05Z

Hi guys,
Sorry for taking so long, I've been out on holiday. I made an issue for adding regression tests. Do we want to complete that in this PR? If not, what needs to be done with this PR to merge it? If we do, how should the regression test be done?

jaisnan · 2024-08-20T14:46:27Z

Hi guys, Sorry for taking so long, I've been out on holiday. I made an issue for adding regression tests. Do we want to complete that in this PR? If not, what needs to be done with this PR to merge it? If we do, how should the regression test be done?

For this pr to be merged, we are looking for at least something that uses the new flag and ensure Kani still succeeds. It doesn't need to check that the files exist, just needs to test that Kani doesn't break. If you can add that, I am OK with merging the PR. Lmk if thats putting too much on your plate, and I can take over this PR and add those changes myself as well. In any case, thank you very much for the changes :D.

Alexander-Aghili · 2024-08-20T15:38:03Z

I swear I'm not as flaky as I seem but my computer's screen just broke and I'm going to have to get it replaced which might take some time. If you want to take it over and finish the PR feel free to do so otherwise I'll get on it immediately when I can.

jaisnan · 2024-08-20T15:44:39Z

I swear I'm not as flaky as I seem but my computer's screen just broke and I'm going to have to get it replaced which might take some time. If you want to take it over and finish the PR feel free to do so otherwise I'll get on it immediately when I can.

No worries :D

Alexander-Aghili · 2024-09-18T00:52:53Z

Finally getting back around to this. I will be creating that test and merging in the new base, then asking for a review to be merged.

jaisnan · 2024-09-18T14:26:08Z

Finally getting back around to this. I will be creating that test and merging in the new base, then asking for a review to be merged.

Awesome, thank you!

Alexander-Aghili · 2024-09-21T17:38:13Z

Hey, I was checking the other UI tests and they verify the validity of the response by running a proof in a main.rs file and checking it with an expected output which is a single txt. With the proper output being a directory, how should I handle that? Also, do you use git merge or git rebase when updating the git structure?

Alexander-Aghili added 3 commits July 19, 2024 03:56

Added individual output to files + args, still issues with directory …

72522bc

…input and target directory

Harness refactor + directory fix

5b85c39

Quick format fix and refactor to fix the output correctness

0a40e1b

Alexander-Aghili requested a review from a team as a code owner July 19, 2024 05:02

celinval reviewed Jul 19, 2024

View reviewed changes

celinval reviewed Jul 20, 2024

View reviewed changes

Refactor of harness runner update for Harness Output Individual Files

6a951b4

celinval requested a review from jaisnan July 26, 2024 18:14

celinval assigned jaisnan Jul 26, 2024

Alexander-Aghili and others added 2 commits July 31, 2024 07:16

Untested configuration options change (commit to get to server to test)

01bd260

Remove enable-unstable requirement

2524fa1

Comments + formatting

1e125f4

Alexander-Aghili mentioned this pull request Aug 4, 2024

Adding regressions tests for output based on output configuration #3412

Open

Merge branch 'main' into harness_output_individual_files

8b008c4

feliperodri added the [E] User Experience An UX enhancement for an existing feature. Including deprecation of an existing one. label Aug 6, 2024

Merge branch 'main' into harness_output_individual_files

38b6698

celinval reviewed Aug 14, 2024

View reviewed changes

kani-driver/src/harness_runner.rs Outdated Show resolved Hide resolved

celinval and others added 2 commits August 14, 2024 14:22

Update kani-driver/src/harness_runner.rs

cca9648

Merge branch 'main' into harness_output_individual_files

698d718

Merge branch 'main' into harness_output_individual_files

56159a1

jaisnan removed their assignment Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harness output individual files #3360

Harness output individual files #3360

Alexander-Aghili commented Jul 19, 2024

celinval Jul 19, 2024 •

edited

Loading

celinval Jul 19, 2024

Alexander-Aghili Jul 20, 2024

celinval Jul 20, 2024

Alexander-Aghili Jul 22, 2024

celinval Jul 23, 2024

jaisnan commented Jul 21, 2024

Alexander-Aghili commented Jul 22, 2024

Alexander-Aghili commented Jul 26, 2024

Alexander-Aghili commented Jul 31, 2024

jaisnan commented Jul 31, 2024 •

edited

Loading

Alexander-Aghili commented Aug 1, 2024

artemagvanian commented Aug 1, 2024

zhassan-aws commented Aug 1, 2024

Alexander-Aghili commented Aug 4, 2024 •

edited

Loading

jaisnan commented Aug 14, 2024 •

edited

Loading

Alexander-Aghili commented Aug 20, 2024

jaisnan commented Aug 20, 2024

Alexander-Aghili commented Aug 20, 2024

jaisnan commented Aug 20, 2024

Alexander-Aghili commented Sep 18, 2024

jaisnan commented Sep 18, 2024

Alexander-Aghili commented Sep 21, 2024

	if self.coverage
	&& !self.common_args.unstable_features.contains(UnstableFeature::LineCoverage)
	{
	return Err(Error::raw(
	ErrorKind::MissingRequiredArgument,
	"The `--coverage` argument is unstable and requires `-Z \
	line-coverage` to be used.",
	));
	}

Harness output individual files #3360

Are you sure you want to change the base?

Harness output individual files #3360

Conversation

Alexander-Aghili commented Jul 19, 2024

celinval Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

celinval Jul 19, 2024

Choose a reason for hiding this comment

Alexander-Aghili Jul 20, 2024

Choose a reason for hiding this comment

celinval Jul 20, 2024

Choose a reason for hiding this comment

Alexander-Aghili Jul 22, 2024

Choose a reason for hiding this comment

celinval Jul 23, 2024

Choose a reason for hiding this comment

jaisnan commented Jul 21, 2024

Alexander-Aghili commented Jul 22, 2024

Alexander-Aghili commented Jul 26, 2024

Alexander-Aghili commented Jul 31, 2024

jaisnan commented Jul 31, 2024 • edited Loading

Alexander-Aghili commented Aug 1, 2024

artemagvanian commented Aug 1, 2024

zhassan-aws commented Aug 1, 2024

Alexander-Aghili commented Aug 4, 2024 • edited Loading

jaisnan commented Aug 14, 2024 • edited Loading

Alexander-Aghili commented Aug 20, 2024

jaisnan commented Aug 20, 2024

Alexander-Aghili commented Aug 20, 2024

jaisnan commented Aug 20, 2024

Alexander-Aghili commented Sep 18, 2024

jaisnan commented Sep 18, 2024

Alexander-Aghili commented Sep 21, 2024

celinval Jul 19, 2024 •

edited

Loading

jaisnan commented Jul 31, 2024 •

edited

Loading

Alexander-Aghili commented Aug 4, 2024 •

edited

Loading

jaisnan commented Aug 14, 2024 •

edited

Loading