-
Notifications
You must be signed in to change notification settings - Fork 22
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
Cargo extension for compiling Rust to MASM #61
Conversation
@bobbinth Could you please create a new repo |
46d0dbc
to
0727932
Compare
What kind of projects would this be for? For example, if this is to create smart contracts (i.e., account code), it should probably be called One other question: do we need to create a separate repo per command? Or could we create one repo (e.g., |
I think this is probably just for a simple Rust project template initially.
We need a repo per template I believe, so if we have three project templates, that's three separate repos. I'm not sure if we can put all templates in a single repo or not, @greenhat would be able to say for sure. |
Yes, so far, it's just compiling a
Good question! |
@greenhat - I've created rust-templates and you should have write access to it. Let me know if anything else is needed. |
38853fa
to
6851153
Compare
compiling a template project
…ches in ApplyRewritesStage Otherwise, ApplyRewritesStage crashes when arg matches are not provided in Session.
7f98ec9
to
af32c71
Compare
…put folder in cargo extension Add WasmTranslationConfig::module_name_fallback to use if Wasm module doesn't specify a name in the custom section. In `Session::emit` if the path is a directory, emit to a file in that directory named after a compiled item(module) name.
@bitwalker ready for review |
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.
Nice work!
There's a few things I'd like to change about the way the extension works, and a few cosmetic things, but overall this looks good to me. I think we can probably have this merged tomorrow, but let me know if some of the changes I've suggested will complicate that and we can discuss - but I can either pick up where you leave off tomorrow, or we can just wait an extra day or whatever.
cargo-ext/src/cli_commands.rs
Outdated
#[command(next_display_order(10), name = "compile")] | ||
Compile { | ||
/// The target environment to compile for | ||
#[arg(long = "target", value_name = "TARGET", default_value_t = TargetEnv::Base, display_order(2))] |
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.
You can change long = "target"
to just target
, and remove the display_order
flag (unless there is a specific need for it that isn't apparent from the context).
cargo-ext/src/compile.rs
Outdated
} | ||
let wasm_file_path = wasm_artifacts[0].clone(); | ||
match project_type { | ||
ProjectType::Program => { |
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.
I think we will have enough project metadata with the other changes I've mentioned that we can just filter the CompilerArtifact
messages. We either have a matching artifact or we don't have any once the build finishes.
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.
If we'll go the route I outlined in #61 (comment), I think we might get away without specifying ProjectType
to midenc
or add an AutoDetect
variant. If not, we can peek inside the Wasm component binary to determine what we're building. We may need this in other parts of our ecosystem anyway.
@@ -285,13 +285,17 @@ impl Session { | |||
if self.should_emit(output_type) { | |||
match self.output_files.path(output_type) { | |||
OutputFile::Real(path) => { | |||
let path = if let Some(name) = item.name() { | |||
path.with_file_name(name.as_str()) | |||
let file_path = if path.is_dir() { |
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.
The output file path is guaranteed to always be a file path, not a directory, so this check is unnecessary.
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.
In my case, it's a folder since I'm passing a target
folder as output_dir
in Session
.
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.
Hmm, the OutputFile API is designed to always return a file path, so if it's returning a directory there's a bug somewhere. I'll take a closer look
@bitwalker Thank you for the review! I addressed all your comments. I converted this PR to draft and I'm working on it. I outlined for myself what is left to do in the PR description. |
and pick up the artifacts from the cargo build to compile the MASM. Similar to the way cargo-component does it. Keep in mind that this cargo extension is planned to be the cargo-component replacement when it comes to building/publishing/etc the Wasm/Miden components. This should explain some implementation details. The overaching idea is to introduce specific to Miden subcommands (e.g. `new`) and pass the rest of the subcommands to cargo where its (e.g. `build`).
@bitwalker I finished implementing all the review notes and it's ready for review. Please check the updated PR description for a summary. |
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.
LGTM. I'll need to look into the OutputFile
thing I mentioned, but I'm not going to hold this PR up for that
Close #60
This PR introduces a cargo extension in the
cargo-miden
crate, which supports the following commands:miden build
- compiles the selected target to MASMmiden new
- creates a new project from a template. The template is downloaded from a git repository.Keep in mind that this cargo extension is planned to be the cargo-component
replacement when it comes to building/publishing/etc the Wasm/Miden components.
This should explain some implementation details. The overarching idea is
to introduce specific to Miden subcommands (e.g.,
new
) and pass therest of the subcommands to cargo (e.g.,
build
) and process the produced artifacts.If the
wasm32-was
target is not present, it will be installed.