-
Notifications
You must be signed in to change notification settings - Fork 57
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
feat: support run, test and bench cargo commands #143
feat: support run, test and bench cargo commands #143
Conversation
dc08b18
to
075e5a9
Compare
@peterhuene, does this approach makes sense to you? |
Hi @eduardomourar, thanks for the PR! Unfortunately, it isn't quite as simple as copying what The first problem is that componentization is expected to occur between the "build" and "run" stages of the "run" command; right now if you pass through to If we assume the first problem is solved, we run into the second problem: the component produced by To solve the first problem, Solving the second problem is much more involved and one of the reasons I've been holding off implementing these commands until a proper design is put in place. |
@peterhuene, my particular use case is only to speed the development cycle while building individual components. You can find a sample GitHub run here. There you will see that I am running the test using this branch and the WASI Preview 2 functions are actually being invoked. It probably works because I don't have a composed component. Is there anything I can change in this PR to get us in the expected direction? Maybe scope it down to include only the |
Your test is running I suspect if you update your CI to use the latest Wasmtime, you'll get an error: As I mentioned earlier, the reason for this is that componentization of the build output occurs after a In addition to the problems I outlined above, |
That said, it is possible to implement a first-class Those commands would do a build ( That would probably be the first step in implementing this, upon which we can add an integration of a composition step prior to executing the runner. |
Thanks for the explanation. Apologies for the confusion on my part. I always forget that wasmtime will follow the component/core module path based on its content and not the arguments passed. I will check how complex it is to have the logic you suggested in this PR. |
Ok, so finally got it working on unix. Just need to remove the hack to get the executable file. |
@peterhuene , this is ready for review now. |
15bfdfd
to
759c477
Compare
@eduardomourar thanks for all the work on this! I was away at a conference this week so haven't yet had time to review yet. I'm still clawing my way out of a backlog, but I plan on reviewing this in full this coming Monday. |
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.
Hi @eduardomourar, sorry for the delay.
I have some general comments about the implementation before going deeper into the review.
Generally, I do like the approach of using the JSON messages to detect what got built, which is much more robust than the current implementation.
But we simply cannot buffer cargo's output like this implementation, as it really hurts DX.
Would it be possible to explore using a pipe for stdout and process the JSON messages as they arrive?
I will mention that using I'm also really grateful that you're driving this forward and we'll have a functioning |
By the way, I could not figure out how to ensure that only the current package is executed in a cargo workspace context. |
This commit pipes the stdout and inherits stderr when spawning `cargo`, allowing for immediate output from cargo without buffering. It also improves the processing of wasm artifacts to componentize; the "creating component" message no longer prints on every build for macOS users. Removed forcing colors in cargo output as we now inherit stderr, allowing cargo to detect if it is a terminal or not.
1c1b8af
to
7aa84a5
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.
@eduardomourar thank you very much for your work on implementing these features!
I kept your approach mostly intact with my additional commits, except I piped stdout (when needed) and inherited stderr so that cargo both handles colors appropriately and output is displayed to users in "real time" (i.e. not buffering it).
The tests were modified slightly to rely on the cli world from the adapter, where applicable. This helped solve the conflicting run
implementations.
This will take a similar approach as used by
cargo-wasi
(source code here) in order to expose the following cargo commands:run
,test
andbench
.The following steps will be done per action:
run
: execute build command, componentize the output, and then feed what it componentized to the configured runner.test
andbench
: execute test/bench with--no-run
argument, componentize the output, and then feed componentized wasm to the configured runner.Related to: #94.