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.

Sign up for GitHub

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

Jump to bottom

fix(nargo)!: Remove -p short flag from the --program-dir flag #2300

Merged
merged 1 commit into from
Aug 14, 2023
Merged

fix(nargo)!: Remove -p short flag from the --program-dir flag #2300

merged 1 commit into from
Aug 14, 2023

Conversation

alexvitkov
Copy link
Contributor

@alexvitkov alexvitkov commented Aug 14, 2023 •
edited
Loading

Description

The recently added --program-name conflicts with the --prover-name argument of nargo prove, and nargo prove panics with the following error:

Message:  Command prove: Short option names must be unique for each argument, but '-p' is in use by both 'prover_name' and 'program_dir'

Problem*

Resolves #2299

Summary*

Remove the option to have --program-dir as a short argument so it doesn't conflict with --prover-name

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@alexvitkov
Copy link
Contributor Author

@phated tagging for visibility

@alexvitkov alexvitkov mentioned this pull request Aug 14, 2023
Closed
@alexvitkov alexvitkov changed the title fix: --prover-name and --program-dir shorthands conflict fix(nargo): --prover-name and --program-dir shorthands conflict Aug 14, 2023
jfecher
jfecher approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This issue was preventing all uses of nargo prove on master. Any adjustments can be done in another PR.

@jfecher jfecher added this pull request to the merge queue Aug 14, 2023
@jfecher
Copy link
Contributor

jfecher commented Aug 14, 2023

@phated, @TomAFrench, this is the second time a similar issue to this has happened. Is there anything we can try to prevent this in the future?

@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Aug 14, 2023
@TomAFrench
Copy link
Member

Removing from the queue as I want to look into how these came to conflict. We are also going to be removing the --prover-name flag in future so it would probably make sense to adjust the short flag on that instead.

@TomAFrench TomAFrench added this pull request to the merge queue Aug 14, 2023
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Aug 14, 2023
@phated phated changed the title fix(nargo): --prover-name and --program-dir shorthands conflict fix(nargo)!: Remove -p short flag from the --program-dir flag Aug 14, 2023
@phated phated added this pull request to the merge queue Aug 14, 2023
Merged via the queue into noir-lang:master with commit cc2af74 Aug 14, 2023
22 checks passed
@kevaundray kevaundray mentioned this pull request Aug 14, 2023
Merged
TomAFrench added a commit that referenced this pull request Aug 14, 2023
@TomAFrench
Merge branch 'master' into remove-backend-solver
ecfba96 
* master:
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
d9236f4 
* master:
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into abi-wasm
075fde8 
* master: (23 commits)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
  fix: Fix assignment when both `mut` and `&mut` are used (#2264)
  feat: Add `assert_constant` (#2242)
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  ...
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into intrinsic-die
70aaf04 
* master: (26 commits)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexvitkov @jfecher @TomAFrench @phated