Skip to content

proof-of-concept playground#1597

Merged
alexcrichton merged 10 commits intobytecodealliance:mainfrom
bakkot-org:playground
Jun 10, 2024
Merged

proof-of-concept playground#1597
alexcrichton merged 10 commits intobytecodealliance:mainfrom
bakkot-org:playground

Conversation

@bakkot
Copy link
Copy Markdown
Contributor

@bakkot bakkot commented Jun 8, 2024

Fixes #1595.

You can try print playground and the parse playground.

It's extremely basic and not very pretty, but it does work. I've only implemented wasm-tools parse so far because I want to give maintainers a chance to weigh in on the approach before doing more work. I'm happy to implement another for wasm-tools print as well, and maybe a third for dump+objdump+metadata show if you think that's worth doing. EDIT: and the wasm-tools print one.

I'm using cargo-component and jco, mostly because I thought that would be more fun than wasm-bindgen. Unfortunately I did need to target wasm32-wasi since I get an unreachable when targeting wasm32-unknown-unknown. And there does not seem to be a simple way to provide my own shims of the preview-1 functions when using cargo component; it unconditionally translates them to preview-2. But that's fine because jco has built in support for shimming preview-2. This adds about 50Kb of (unminimized) JavaScript, but whatever.

This includes a new workflow which a) ensures the playground builds in every PR and b) uploads the built artifact to gh-pages on every commit to main. I think you'll need to manually enable GitHub pages and set the source to "GitHub Actions" for this to actually do anything, but it's possible the included actions will take care of that.

I've commented out the main CI for the sake of not taking up a bunch of resources while iterating on this. I'll revert that before marking this as ready, of course.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this it looks great to me! I've got a few minor comments but I'm happy to land the overall structure and iterate in-tree as necessary.

I think I just frobbed the button to say github pages comes from actions, so I think that's ready to go?

Comment thread playground/component/Cargo.lock Outdated
Comment on lines +1 to +4
[package]
name = "component"
edition = "2021"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a publish = false here, and as part of the main workspace thisi can also use edition.workspace = true along with [lints] workspace = true too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, actually, adding [lints] workspace = true prevents building because the lints don't like the code that cargo component generates. I've removed it for the moment; happy to take other suggestions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok no worries, I can try to work on that once this lands.

run:
shell: bash

jobs:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind adding this to the main.yml file as well? The final "gate everything here" job could gate on the build job here and the deploy job could continue to depend on main. Otherwise though this looks great!

I learned about actions/{configure-pages,upload-pages-artifact,deploy-pages} here, I didn't know what those were before!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not totally sure what you're asking here - are you suggesting getting rid of playground.yml and moving both of these new jobs into main.yml? I was aiming to have this run after commits land on main, which that file doesn't (appear to) do. But I guess if everything goes through the merge queue it ends up being roughly equivalent?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm good point yeah. We try to keep CI automation on the main branch itself pretty light and instead defer most of the work to the merge queue when possible. For example release binaries are built on the merge queue and then when the commit gets pushed to main the release binaries are uploaded for tagged commits.

My initial thinking is that something similar could be done here where the build job executes as part of main.yml on the merge queue and then the deploy job runs like publish.yml on pushes to main.

That being said when I write it all down it feels like that may be overly complicated. Do you know if it would be difficult to split it up like that? If so the downside of having playground.yml is that I'll need to update the CI settings to gate on this workflow in addition to the current gated workflows for the merge queue, but that's not the end of the world as this won't really change much over time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if it would be difficult to split it up like that?

Just looked and the answer appears to be that yes, it's (a little) difficult. Artifacts are meant to be used within a single workflow. To make this work, you'd need to download and re-upload them in the publish job - there doesn't appear to be a (supported) way to publish an artifact from a different run.

You're already downloading artifacts from a previous run in publish.yml, but in trying it on my own fork I couldn't immediately get that to work - it's failing to find the run_id, despite that working in this project, and I don't have enough experience or interest in GitHub actions to debug it further.

@bakkot
Copy link
Copy Markdown
Contributor Author

bakkot commented Jun 9, 2024

I'll go ahead and make the print page as well, then, and add a root index.html which I guess just redirects to the github project (unless you want to have a separate page listing the tools, but that seems a bit redundant). Then I think think it should be fit to land.

@bakkot
Copy link
Copy Markdown
Contributor Author

bakkot commented Jun 9, 2024

Alright, added the printer as well. This is good to go from my end once the CI thread above is resolved. Which I'm happy to do but if you'd find it easier to make the changes yourself rather than explaining them that's also fine.

Comment thread playground/component/src/lib.rs Outdated
Comment on lines +10 to +15
impl wasmprinter::Print for StringWriter {
fn write_str(&mut self, s: &str) -> std::io::Result<()> {
self.0.push_str(s);
Ok(())
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're interested in getting particularly fancy here it might be neat to hook into the start_* methods and emit <span> entries to have highlighted colos on the web too. That's fine to do later though as a follow-up as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, fun, I'll play around with that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, following roughly the terminal colors used for the same parts.

@@ -0,0 +1,85 @@
<!doctype html>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For discoverability and sharing code do you think that it'd be reasonable to have only one landing page for the various tools and and sort of "tabs" between them to explore different functionality?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. Are you thinking of one HTML page, or multiple pages that link to each other? Having multiple makes it easier to bookmark a specific tool, though I guess I could use the #fragment and some JS to make that possible with a single page.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking one page, but I'm not wed to the idea either. It mostly just seemed like there was some duplication between the styles/js/etc. Having links to each page from the other I think is also fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added links from each page to the other. There's not really much duplicated that isn't already shared - they use the same core stylesheet and worker, it's just the actual relevant UI which differs.

Comment on lines +1 to +4
[package]
name = "component"
edition = "2021"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok no worries, I can try to work on that once this lands.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks reasonable to me, thanks again for providing the foundation for all this!

@alexcrichton alexcrichton added this pull request to the merge queue Jun 10, 2024
Merged via the queue into bytecodealliance:main with commit 92861f1 Jun 10, 2024
@alexcrichton
Copy link
Copy Markdown
Member

Looks like the deploy worked, yay! I've got some minor follow-ups but when you feel it's ready I think it'd be great to link this from the README in the root of the repo as well

@bakkot bakkot deleted the playground branch June 11, 2024 01:42
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jun 13, 2024
This commit is an attempt to migrate from
`JamesIves/github-pages-deploy-action`, a third party action, to what
appears to be a more official action under the `github.com/actions`
organization. This was first pioneered in
bytecodealliance/wasm-tools#1597 and it looked like it would work well
over here too.
github-merge-queue Bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jun 13, 2024
This commit is an attempt to migrate from
`JamesIves/github-pages-deploy-action`, a third party action, to what
appears to be a more official action under the `github.com/actions`
organization. This was first pioneered in
bytecodealliance/wasm-tools#1597 and it looked like it would work well
over here too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Online playground

2 participants