-
Notifications
You must be signed in to change notification settings - Fork 63
docs: Clarify setup for "Rust Components" tutorial #218
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
docs: Clarify setup for "Rust Components" tutorial #218
Conversation
Hey @mkatychev would you mind taking another look at this? One of the thing we've wanted to do for a bit was refactor to use the same WIT files everywhere, and we finally have that in now for all the guides -- I think the changes here are in conflict with that, but it should simplify your diff as well. |
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 🚀
One nit about an alert but this looks good, let's get this merged :)
Co-authored-by: Victor Adossi <123968127+vados-cosmonic@users.noreply.github.com>
@kate-goldenring this PR should be ready for a final review & merge |
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.
Thank you @mkatychev for these updates. My main suggestion is to not require the reader to clone the repository. Rather, have them just fetch the WIT file.
|
||
```toml | ||
[package.metadata.component.target.dependencies] | ||
"docs:adder" = { path = "../adder/wit" } # directory containing the WIT package | ||
"docs:adder" = { path = "../wit/adder" } # directory containing the WIT package |
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 assumes they cloned the repo rather than just fetching the wit. I'd prefer to stay with the approach of fetching the one wit file rather than cloning the repo
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.
Following up on #218 (comment), if we don't specify cloning the repo then we'd have to specify fetching the wit/adder
, wit/calculator
, and examples/adder
directories explicitly, I don't mind doing this but it seems like that adds more complexity than a git clone
.
git clone
is a temporary approach regardless until #241 (comment) is done.
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 fetching a file is the most analogous to what the wkg experience will be like
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.
Agreed that that's what we should aim for, however, calling cargo run --release -- 1 2 ../add/target/wasm32-wasip1/release/adder.wasm
for this step requires us to have all seven files from the example-host directory present.
Should I instead add an instruction to wget
the directory and extract 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.
I agree that the example host is an exception -- for now, they will need to clone the repo to run it. Maybe this line should be cargo run --release -- 1 2 adder.wasm
to not imply that they build the adder component from the repo.
The main thing i am thinking about is that the fact that the example host is not fetchable should not define how we instruct users to build components. We can push the host up in a container for example or release 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.
I think it's a great and useful example that demonstrantes using external logic to execute the user's code.
I haven't found a succinct way, other than git
, to fetch the example-host logic unless something like wkg
can do 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.
Does it make sense to have a wit in ghcr that they can depend on? Then there would be no question about file paths because they wouldn't be needed when specifying the dependency.
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.
That is definitely the plan: upload the packages to ghcr and update the tutorial and guides to use wkg. We just need the work to be prioritized
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.
Created an issue to get them in GHCR: #243
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 the meantime for this PR, should I revert the path changes and references to git clone
?
EDIT: that's what was done
14098df
to
9102bc9
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.
Thank you for making this a smoother experience!
This PR aims to make explicit some assumptions made in the Rust Components tutorial.
Changes made to
component-model/src/language-support/rust.md
:example:component/example
to help disambiguate between similar sounding names such asadd
andadder
component-docs/component-model/src/language-support/rust.md
Lines 51 to 53 in 2e207e4
$
from shell commands to make them easier to copy