Skip to content

Commit

Permalink
feat: improve nargo check cli with --override flag and feedback for e…
Browse files Browse the repository at this point in the history
…xisting files (#4575)

Recently, while working with `nargo check`, I noticed when Prover.toml
and Verifier.toml files already existed, running `nargo check` would not
update these files unless they were manually deleted and the command was
rerun. This was a bit frustrating, as there was no indication of why
changes weren't being applied and a success message was still output.

To address this, I've made an enhancement that improves t he user
experience in two ways:
1. If Prover.toml and Verifier.toml exist and `--override` is not used,
`nargo check` will no longer show a success message misleadingly. This
clarifies that no changes were made due to the presence of these files.
2. It introduces guidance to use the `--override` flag if the intention
is to update these existing files, making it clear how to proceed to
apply the desired changes.

This makes it more intuitive by removing the need to manually delete
files Prover.toml and Verifier.toml

### Changes:

- **When both files exist without using --override**: The tool will
alert you that Prover.toml and Verifier.toml already exist and suggest
using `--override` to update them.
```sh
sh-5.2# nargo check 
Warning: Prover.toml already exists. Use --override to force overwrite.
Warning: Verifier.toml already exists. Use --override to force overwrite.
```

- **When no file exists**: It proceeds as expected, building the
constraint system successfully.
```sh
sh-5.2# nargo check
[simple_range] Constraint system successfully built!
```

- **When files exist and --override is used**: It overwrites the
existing files and rebuilds the constraint system.
```sh
sh-5.2# nargo check --override
[simple_range] Constraint system successfully built!
```

- **When only one of the files exists**: It creates or overwrites the
missing file, as needed, without the need for `--override` for the
missing one, but warns about the existing file.
```sh
sh-5.2# nargo check 
Warning: Prover.toml already exists. Use --override to force overwrite.
[simple_range] Constraint system successfully built!
```
```sh
sh-5.2# nargo check 
Warning: Verifier.toml already exists. Use --override to force overwrite.
[simple_range] Constraint system successfully built!
```

- **With --override, even if only one file exists**: Both files are
re/created.
```sh
sh-5.2# nargo check --override
[simple_range] Constraint system successfully built!
```

**override flag in --help:**

```sh
sh-5.2# nargo check --help
Checks the constraint system for errors

Usage: nargo check [OPTIONS]

Options:
      --package <PACKAGE>                    The name of the package to check
      --workspace                            Check all packages in the workspace
      --override                             Force overwrite of existing files
      --expression-width <EXPRESSION_WIDTH>  Override the expression width requested by the backend
      --force                                Force a full recompilation
      --print-acir                           Display the ACIR for compiled circuit
      --deny-warnings                        Treat all warnings as errors
      --silence-warnings                     Suppress warnings
  -h, --help                                 Print help
```

---------

Co-authored-by: jfecher <[email protected]>
  • Loading branch information
SamiAlHassan and jfecher authored Apr 4, 2024
1 parent 62423d5 commit 5e7fbd4
Showing 1 changed file with 35 additions and 8 deletions.
43 changes: 35 additions & 8 deletions tooling/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ pub(crate) struct CheckCommand {
#[clap(long, conflicts_with = "package")]
workspace: bool,

/// Force overwrite of existing files
#[clap(long = "overwrite")]
allow_overwrite: bool,

#[clap(flatten)]
compile_options: CompileOptions,
}
Expand All @@ -58,18 +62,29 @@ pub(crate) fn run(
let parsed_files = parse_all(&workspace_file_manager);

for package in &workspace {
check_package(&workspace_file_manager, &parsed_files, package, &args.compile_options)?;
println!("[{}] Constraint system successfully built!", package.name);
let any_file_written = check_package(
&workspace_file_manager,
&parsed_files,
package,
&args.compile_options,
args.allow_overwrite,
)?;
if any_file_written {
println!("[{}] Constraint system successfully built!", package.name);
}
}
Ok(())
}

/// Evaluates the necessity to create or update Prover.toml and Verifier.toml based on the allow_overwrite flag and files' existence.
/// Returns `true` if any file was generated or updated, `false` otherwise.
fn check_package(
file_manager: &FileManager,
parsed_files: &ParsedFiles,
package: &Package,
compile_options: &CompileOptions,
) -> Result<(), CompileError> {
allow_overwrite: bool,
) -> Result<bool, CompileError> {
let (mut context, crate_id) = prepare_package(file_manager, parsed_files, package);
check_crate_and_report_errors(
&mut context,
Expand All @@ -81,27 +96,39 @@ fn check_package(

if package.is_library() || package.is_contract() {
// Libraries do not have ABIs while contracts have many, so we cannot generate a `Prover.toml` file.
Ok(())
Ok(false)
} else {
// XXX: We can have a --overwrite flag to determine if you want to overwrite the Prover/Verifier.toml files
if let Some((parameters, return_type)) = compute_function_abi(&context, &crate_id) {
let path_to_prover_input = package.prover_input_path();
let path_to_verifier_input = package.verifier_input_path();

// If they are not available, then create them and populate them based on the ABI
if !path_to_prover_input.exists() {
// Before writing the file, check if it exists and whether overwrite is set
let should_write_prover = !path_to_prover_input.exists() || allow_overwrite;
let should_write_verifier = !path_to_verifier_input.exists() || allow_overwrite;

if should_write_prover {
let prover_toml = create_input_toml_template(parameters.clone(), None);
write_to_file(prover_toml.as_bytes(), &path_to_prover_input);
} else {
eprintln!("Note: Prover.toml already exists. Use --overwrite to force overwrite.");
}
if !path_to_verifier_input.exists() {

if should_write_verifier {
let public_inputs =
parameters.into_iter().filter(|param| param.is_public()).collect();

let verifier_toml = create_input_toml_template(public_inputs, return_type);
write_to_file(verifier_toml.as_bytes(), &path_to_verifier_input);
} else {
eprintln!(
"Note: Verifier.toml already exists. Use --overwrite to force overwrite."
);
}

Ok(())
let any_file_written = should_write_prover || should_write_verifier;

Ok(any_file_written)
} else {
Err(CompileError::MissingMainFunction(package.name.clone()))
}
Expand Down

0 comments on commit 5e7fbd4

Please sign in to comment.