feat!: Support workspaces and package selection on every nargo command by phated · Pull Request #1992 · noir-lang/noir

phated · 2023-07-21T00:39:17Z

Description

Problem*

Closes #794
Closes #1201 (we might want a follow-up issue for the caching vs rebuilding a package)
Towards #1838

Summary*

This replaces the originally merged workspace logic by unifying the various ways you can configure a project (top-level package, workspace, etc) into a single Workspace type that can contain one of more Package and optionally a selected Package. It also separates these concepts from the serde logic when parsing toml and adds functions that resolve a Workspace completely. It adds an iterator implementation on Workspace that iterates the correct Packages where each package can be prepared with it's own Context so we don't leak any compilation details.

Note: This PR is breaking as it removes circuit_name, proof_name and proof positional arguments for various commands and replaces them with --package CLI argument. This means that each Package is expected to contain exactly one "named circuit" and users will be expected to setup a workspace to achieve the old behavior.

Below is a diagram of the pipeline between Nargo.toml files, the --package CLI flag, and Workspace struct.

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.

We need to update each command that now accepts the --package argument, remove the circuit_name positional argument, and we need to document workspaces thoroughly.

Additional Context

PR Checklist*

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

TomAFrench

This lines up with a lot of my thinking on workspaces from looking at how Cargo handles them. I've made a couple of notes but I don't think that they'll be anything you don't already know. Don't want to raise too many nits while this is draft so let me know when you make this PR ready for review.

crates/nargo_cli/src/manifest.rs

crates/nargo/src/workspace.rs

crates/nargo_cli/src/cli/codegen_verifier_cmd.rs

crates/nargo_cli/src/manifest.rs

crates/nargo_cli/tests/test_data/ecdsa_secp256k1/Nargo.toml

crates/noirc_driver/src/lib.rs

crates/noirc_frontend/src/graph/mod.rs

crates/nargo/src/constants.rs

crates/nargo/src/package.rs

crates/nargo_cli/src/resolver.rs

crates/nargo_cli/src/cli/info_cmd.rs

crates/nargo_cli/src/cli/compile_cmd.rs

crates/nargo_cli/src/cli/codegen_verifier_cmd.rs

crates/nargo/src/workspace.rs

crates/nargo_cli/src/manifest.rs

crates/nargo_cli/tests/test_data_ssa_refactor/config.toml

crates/nargo_cli/src/cli/check_cmd.rs

…g it to be a Package

crates/nargo/src/package.rs

TomAFrench

We're getting collisions due to not namespacing proofs/witnesses/etc. I've updated the integration tests to ensure that the workspace packages are distinct so we get test failures.

crates/nargo_cli/src/git.rs

crates/nargo_cli/src/manifest.rs

crates/nargo_cli/src/cli/prove_cmd.rs

crates/nargo_cli/src/cli/verify_cmd.rs

TomAFrench · 2023-08-01T11:09:53Z

I'd be ok with just restricting prove/verify/execute to run on a single package and error if no default is set.

phated · 2023-08-01T15:01:00Z

I'd be ok with just restricting prove/verify/execute to run on a single package and error if no default is set.

I think we should prefix the package name as a directory, or use the package name as the proof name by default.

…ng of proof

* master: feat!: Support workspaces and package selection on every nargo command (#1992) chore: Make a more clear error for slices passed to std::println (#2113) feat: Implement type aliases (#2112) feat: Add `Option<T>` to noir stdlib (#1781) feat: Format strings for prints (#1952) feat(acir_gen): RecursiveAggregation opcode and updates to black box func call generation (#2097) fix: Mutating a variable no longer mutates its copy (#2057) fix: Implement `.len()` in Acir-Gen (#2077) chore: clippy fixes (#2101)

* master: (50 commits) fix(globals): Accurately filter literals for resolving globals (#2126) feat: Optimize away constant calls to black box functions (#1981) fix: Rename `Option::value` to `Option::_value` (#2127) feat: replace boolean `AND`s with multiplication (#1954) chore: create a `const` to hold the panic message (#2122) feat: Add support for bitshifts by distances known at runtime (#2072) feat: Add additional `BinaryOp` simplifications (#2124) fix: flattening pass no longer overwrites previously mapped condition values (#2117) chore(noirc_driver): Unify crate preparation (#2119) feat!: Support workspaces and package selection on every nargo command (#1992) chore: Make a more clear error for slices passed to std::println (#2113) feat: Implement type aliases (#2112) feat: Add `Option<T>` to noir stdlib (#1781) feat: Format strings for prints (#1952) feat(acir_gen): RecursiveAggregation opcode and updates to black box func call generation (#2097) fix: Mutating a variable no longer mutates its copy (#2057) fix: Implement `.len()` in Acir-Gen (#2077) chore: clippy fixes (#2101) chore: Update `noir-source-resolver` to v1.1.3 (#1912) chore: Document `GeneratedAcir::more_than_eq_comparison` (#2085) ...

* master: chore: rename `ssa_refactor` module to `ssa` (#2129) chore: Use `--show-output` flag on execution rather than compilation (#2116) fix(globals): Accurately filter literals for resolving globals (#2126) feat: Optimize away constant calls to black box functions (#1981) fix: Rename `Option::value` to `Option::_value` (#2127) feat: replace boolean `AND`s with multiplication (#1954) chore: create a `const` to hold the panic message (#2122) feat: Add support for bitshifts by distances known at runtime (#2072) feat: Add additional `BinaryOp` simplifications (#2124) fix: flattening pass no longer overwrites previously mapped condition values (#2117) chore(noirc_driver): Unify crate preparation (#2119) feat!: Support workspaces and package selection on every nargo command (#1992) chore: Make a more clear error for slices passed to std::println (#2113) feat: Implement type aliases (#2112) feat: Add `Option<T>` to noir stdlib (#1781) feat: Format strings for prints (#1952) feat(acir_gen): RecursiveAggregation opcode and updates to black box func call generation (#2097) fix: Mutating a variable no longer mutates its copy (#2057) fix: Implement `.len()` in Acir-Gen (#2077)

phated requested a review from TomAFrench July 21, 2023 00:39

TomAFrench reviewed Jul 24, 2023

View reviewed changes

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

phated commented Jul 25, 2023

View reviewed changes

crates/nargo/src/workspace.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/tests/test_data/ecdsa_secp256k1/Nargo.toml Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/noirc_driver/src/lib.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/noirc_frontend/src/graph/mod.rs Show resolved Hide resolved

kobyhallx reviewed Jul 26, 2023

View reviewed changes

crates/nargo/src/constants.rs Show resolved Hide resolved

kobyhallx reviewed Jul 26, 2023

View reviewed changes

crates/nargo/src/package.rs Outdated Show resolved Hide resolved

This was referenced Jul 26, 2023

chore!: Require package names in Nargo.toml files #2056

Merged

chore: Implement FromStr and Into<String> for CrateName #2061

Merged

phated force-pushed the phated/normalize-workspace branch from cd11b28 to b18483f Compare July 26, 2023 19:52

phated changed the base branch from master to phated/package-name July 26, 2023 19:52

phated changed the title ~~chore: Redo workspace logic to unify resolved project structure~~ feat!: Support workspaces and package selection on every nargo command Jul 26, 2023

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/resolver.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/cli/info_cmd.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/cli/compile_cmd.rs Outdated Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/cli/codegen_verifier_cmd.rs Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo/src/workspace.rs Show resolved Hide resolved

phated commented Jul 26, 2023

View reviewed changes

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

Base automatically changed from phated/package-name to master July 27, 2023 16:29

phated force-pushed the phated/normalize-workspace branch from 4963946 to 3e58f37 Compare July 27, 2023 17:18

phated marked this pull request as ready for review July 27, 2023 20:41

github-actions bot added the doc needed label Jul 27, 2023

github-actions bot mentioned this pull request Jul 27, 2023

Document workspaces noir-lang/docs#288

Closed

TomAFrench reviewed Jul 28, 2023

View reviewed changes

crates/nargo_cli/tests/test_data_ssa_refactor/config.toml Show resolved Hide resolved

TomAFrench reviewed Jul 28, 2023

View reviewed changes

crates/nargo_cli/src/cli/check_cmd.rs Outdated Show resolved Hide resolved

feat!: Support workspaces and package selection on every nargo command

6e9ac11

phated force-pushed the phated/normalize-workspace branch from 48028aa to 6e9ac11 Compare July 28, 2023 16:32

phated added 2 commits July 31, 2023 10:07

remove resolve_workspace_in_directory

e39d041

avoid resolving dependencies as a Workspace struct by always requirin…

4cec2dc

…g it to be a Package

phated commented Jul 31, 2023

View reviewed changes

crates/nargo/src/package.rs Show resolved Hide resolved

chore: ensure workspace packages are distinct

3273892

TomAFrench requested changes Aug 1, 2023

View reviewed changes

crates/nargo_cli/src/git.rs Outdated Show resolved Hide resolved

crates/nargo_cli/src/manifest.rs Outdated Show resolved Hide resolved

crates/nargo_cli/src/cli/prove_cmd.rs Outdated Show resolved Hide resolved

crates/nargo_cli/src/cli/verify_cmd.rs Show resolved Hide resolved

Update crates/nargo_cli/src/git.rs

4638343

phated mentioned this pull request Aug 1, 2023

Panic message is confusing if dependency is missing a name field in toml #2086

Closed

phated added 3 commits August 1, 2023 09:57

remove proof name argument and use package name, remove stdout printi…

450b450

…ng of proof

fix tests

e0ffb48

rename functions to be more descriptive

15309af

This was referenced Aug 1, 2023

Gather feedback about solidity verifier output paths #2108

Closed

Gather feedback on printing proofs to stdout #2109

Closed

add issue number to todo

e2c48ab

phated requested a review from TomAFrench August 1, 2023 18:14

Merge branch 'master' into phated/normalize-workspace

2900ead

TomAFrench approved these changes Aug 1, 2023

View reviewed changes

phated added this pull request to the merge queue Aug 1, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Document Noir PR 1992 noir-lang/docs#299

Closed

Merged via the queue into master with commit 940b189 Aug 1, 2023

phated deleted the phated/normalize-workspace branch August 1, 2023 22:13

kevaundray mentioned this pull request Aug 1, 2023

chore(noir): Release 0.10.0 #2039

Merged

This was referenced Aug 3, 2023

feat: Add ability to create a proof for a workspace member using nargo prove -p {crate_name} #1930

Merged

Epic: Workspaces MVP #2162

Closed

Conversation

phated commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Problem*

Summary*

Documentation

Additional Context

PR Checklist*

Uh oh!

TomAFrench left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomAFrench left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomAFrench commented Aug 1, 2023

Uh oh!

phated commented Aug 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phated commented Jul 21, 2023 •

edited

Loading