-
Notifications
You must be signed in to change notification settings - Fork 248
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.
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
chore(cli): Forward nargo execute
to noir_artifact_cli
#7406
Conversation
… 7381-use-artifact-fs
* master: chore!: bump rust edition to 2024 (#7533)
2f3f53e
to
cefcd25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little concerned that we're building up a number of redundant versions of ProgramArtifact
but we can deal with that later.
Which one do you mean? Getting rid of redundancy was my motivation for getting rid of the |
Sorry, my comment isn't specific to this PR. Just something I'm thinking about related to the CLI in general recently, I think we're on the same page. |
Description
Problem*
Resolves #7380
Builds on #7391
Summary*
Changed
nargo_cli::execute_cmd::run
to create and run an instance ofnoir_artifact_cli::commands::execute_cmd::ExecuteCommand
to completely outsource program execution tonoir_artifact_cli
.Additional Context
Up to now the
ExecuteCommand
innoir_artifact_cli
was part of the binary package; now it's exposed as part of the library through thecommands
module, so we can forward calls fromnargo
by using it as a dependency and just mapping command parameters.Other options would be to:
noir-artifact execute
as a sub-processnoir_artifact_cli::executions
The reason I thought creating and running
ExecuteCommand
would be good are:nargo
aware of wherenoir-execute
can be found, which requires changing a bunch of scripts that currently usenargo
noir-execute
expects, which is a bit more error pronenargo execute
to work with them as well, and there wouldn't be a need to differentiate.nargo
has to span the compilation and the execution phase; if they are in one process there cannot be any clashes, although at the momentnoir-execute
doesn't do any lockingexecution
functions option there is still some duplication of the workflow, and we're a bit more removed from the conceptual "shelling out"I mainly wanted to leave aztec-packages untouched for now, but with this change at least it would be simpler as
nargo
does very little on its own.Documentation*
Check one:
PR Checklist*
cargo aartifact
on default settings.