Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cargo extension for compiling Rust to MASM #61
Changes from 15 commits
367bb3c
f09a893
e706024
66a3d32
134fce3
2db01ff
555051b
5ad3645
e108242
58d3309
824a8eb
62adf34
af32c71
5a8a6ed
91a8b3c
29ffb83
426096c
5f0892f
a69711b
253d491
6e563ef
769d983
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 justtarget
, and remove thedisplay_order
flag (unless there is a specific need for it that isn't apparent from the context).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.
This can just be
#[command]
the rest is implied by default (and I don't thinknext_display_order
is needed here).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.
We should combine all these
midenc_session
imports into a singleuse
since they are all from the same namespace.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'm so used to
"rust-analyzer.imports.granularity.group": "item"
to avoid the merge conflicts, so I forgot to turn it off, given that our codebase employs a different import style. Fixed.Although I'd strongly consider switching to
item
since it's more rebase/merge friendly.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 generally agree with you, and I'm not really a fan of the approach used by the other Miden projects, so I've compromised by using something that is more of a blend: if it's in the same namespace, combine it, e.g.
use a::b::Foo;
anduse a::b::Bar
would get combined, butuse a::b::Foo
anduse a::c::Baz
would be separated. That generally avoids most conflicts due to changes in imports, while collapsing them considerably.If this was my own project, I'd probably switch to grouping by item, but I wanted to at least make the attempt to follow the Miden guidelines to some degree here.
If there's a config file we can add to the project root that sets that rust-analyzer config option locally for the project, feel free to add it. Definitely annoying to have to change it globally just for the Miden projects, so if there's any way to avoid that, I'm open to it.
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.
Got it. It seems this import granularity is
Module
. I made an issue - #78There 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 should use
cargo_metadata::MetadataCommand
to get metadata about the current workspace, and use that metadata to drive the build. In particular, we can use package metadata section, to configure defaults for the project. Most importantly, we'll need to do that anyway in order to get configuration forcargo miden
(and we may also want to accesscargo component
configuration) fromCargo.toml
.For example, a single crate project we can use
cargo_metadata::MetadataCommand
to get the crate metadata - project name, targets, etc.; and use that to determine the default project type being built, the output file name we expect, and so on. We can extend this, or override defaults using package metadata, e.g.[package.metadata.miden]
.On the other hand, for a workspace consisting of multiple crates, we can use the workspace metadata section ( e.g.
[workspace.metadata.miden]
) to specify which projects in the workspace are Miden projects, and usecargo_metadata::MetadataCommand
to obtain metadata for all crates in the workspace, and then get thecargo_metadata::Package
information about the crates specified in our package metadata section. By default,cargo miden build
in this situation would build all Miden projects using their default configuration (using separate invocations ofcompile
internally).To illustrate what I mean:
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.
Sounds good! We'll need options to specify dependencies, etc.. Still, I think
components
anddefault-target
options will add extra complexity for the newcomers and might do more harm than good. Given that they are not strictly necessary for the build process, we should get back to them later when we have a clear picture of the smart contract development workflow (crate structure, testing, etc.).To drive the build process, I'd instead take the
cargo-component
approach. See my comment - #61 (comment)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.
To be clear, those are not needed with a default project setup, only for more complex workspaces which contain a blend of Miden and non-Miden crates. For simple cases, we can just assume all crates are Miden crates, and use the default targets implied by the Cargo configuration. It is very unlikely that most crates will have anything other than a single lib or bin target anyway.
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.
So both
--lib
and--bin
are implied by default withcargo build
, depending on whether the crate has a default target (--bin
is implied if there is asrc/main.rs
, and--lib
ifsrc/lib.rs
). So we only need to obtain the default target from the Cargo metadata of the crate being compiled to detect the appropriateProjectType
.If we want to allow specifying a target, we should follow the conventions of
cargo build
I think, to avoid having different defaults than expected.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.
NOTE: See #61 (comment), but my point about following the conventions of
cargo build
still holds in terms of choosing defaults IMO.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 like the idea of mirroring
cargo build
options and semantics! It got me thinking about taking the approach ofcargo component
for building the Wasm component - just pass all the options tocargo
and pick up Wasm files afterward. See https://github.com/bytecodealliance/cargo-component/blob/87949555d067e92a1872c03daa272794a6c0f6a5/src/lib.rs#L132-L180I think we can take the same approach for our
build
(compile
) command. This means for ourbuild
command, we'll have only--miden-target
option for VM vs. rollup (I'm not even sure we need that), and the rest will be passed tocargo build
(or rathercargo component build
) as is adding--target wasm32-wasi
if it's not present. I believe the Wasm component binary alone would suffice to figure out if Miden program or library is being built.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 I'm fine with following
cargo component
here, though I think it remains to be seen whether our more complex build process forces us to be more restrictive than they are. That said, it feels like a better starting point for sure, and will probably get us quite far before we need to do anything special.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
tomidenc
or add anAutoDetect
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.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.