Use the web-time polyfill #527
Conversation
|
This works for me. I do take @stephanemagnenat's point about the dangers of using a timeout: alas, the "perfect" alternative would be to track memory usage and bork if it gets too high, but that's probably tricky for other reasons. It would be good to revisit that at some point, though, because we might be able to do something that's a little less sucky than the current timeout. |
| vob.workspace = true | ||
| syn.workspace = true | ||
| prettyplease.workspace = true | ||
| web-time = "1.1.0" |
There was a problem hiding this comment.
In theory we could also make this an optional dependency, and only conditionally depend on it if a feature is enabled.
That duplicates the conditional compilation in the crate, but I get that some people might want to minimize dependencies.
There was a problem hiding this comment.
I must admit, I was thinking exactly the same. Personally I do have a mild preference for only compiling it on wasm targets, because then non-wasm users don't have to think about it as something they need to audit. But I appreciate that's extra work.
There was a problem hiding this comment.
What I was thinking was slightly different actually than compiling it conditionally per-platform
But using a feature like the following.
#[cfg(feature = "webpolyfills")]
use web_time::{Instant, Duration}
#[cfg(not(feature = "webpolyfills"))]
use std::{Instant, Duration};
While the name doesn't imply it you could enable webpolyfills on non-web platforms.
Then you could use the same build options to target either web/non-web.
I think we need this kind of feature anyways, just to enable the library from cargo.toml.
At the very least, I don't know about using target specific cfg options that involve dependencies,
that may very well be a thing though.
There was a problem hiding this comment.
I think we can say something like:
[target.'cfg(target_arch = "wasm32")'.dependencies]
web-time = "0.1.0"and then:
#[cfg(target_arch = "wasm32")]
use web_time::{...};But I haven't thought about whether that's nicer than a feature detected in build.rs or not TBH. I'm easy either way.
There was a problem hiding this comment.
I suppose making it work by default without feature flags does seem convenient
|
I'm going to mark this one as draft, until I figure out how to test it. because just doing the naive thing and installing I think the actual main requires some bindgen attributes, and it may well be that more is needed than just this minimal patch. I don't know. |
|
@ratmice thank you for looking into this. I am currently not using grmtools any more, as I switched to lalrpop in the mean time. |
|
Nevermind.. I seem to have figured out the problem with my testing.... |
|
So, I'm pretty uncertain about the The example is very much not pretty, but from what I can tell it does actually work. But I'm going to mark as ready for review because it seems like it's working as intended. |
| Licensed under the Apache License, Version 2.0 (the "License"); you may not use | ||
| this file except in compliance with the License. You may obtain a copy of the | ||
| License at | ||
|
|
There was a problem hiding this comment.
wasm-pack is pretty adamant that LICENSE files exist.
There was a problem hiding this comment.
If we really have to do this, we'll need to add the LICENSE-MIT too. But is there any way to shut wasm-pack up on this? Or, maybe, we just symlink back to the top-level files, to at least avoid bloating the repository?
There was a problem hiding this comment.
I'd thought about symlinks, but IIRC git doesn't do symlinks well on windows.
I forget what it does exactly (either it puts the destination path of the symlink in the contents, or just bloats the repository how we'd be trying to avoid anyways) but I don't think it had a helpful behavior.
I'll look and see if I can't somehow get wasm-pack to pipe down.
There was a problem hiding this comment.
There doesn't seem to be any workaround, there has been issues for 6 years, and PRs for at least 3.
I'll look and see though if there is anything I can do that is more similar to what we did with wasmtime,
setting a runner, so we can just avoid wasm-pack all together, as we aren't really using it for it's pack into a tarball for distribution behavior (where checking licenses makes sense).
I'm not certain this is going to be doable, e.g. the trunkrs project implements a lot of wasm-pack's packing behavior, but still doesn't seem to have it's own testing strategy afaict.
There was a problem hiding this comment.
Strangely and unexpectedly this appears to have been fixed by moving it into cttests in c5a4efd.
There must be something different about the Cargo.toml that makes it not run the check or maybe being one directory closer to the ../Cargo.toml instead of ../../Cargo.toml, I would guess that is it.
There was a problem hiding this comment.
I did manage to get rid of wasm-pack fully in da72a4d
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
There was a problem hiding this comment.
I don't think i've ever actually used a test folder but I don't see any way to specify a crate type for them.
Because of how wasm32-unknown-unknown works, this seems to need to be a cdylib crate.
the #[cfg(test)] module is used for wasm32-wasip2 and native targets,
while the other section is used only for wasm32-unknown-unknown.
There was a problem hiding this comment.
Should be fixed by moving it into cttests in c5a4efd
lrpar/examples/calc_wasm/README.md
Outdated
| * wasm32-unknown-unknown using wasm-bindgen | ||
| * wasm32-wasip2 without wasm-bindgen | ||
| * native targets. | ||
|
|
There was a problem hiding this comment.
This should probably give instructions, and requirements.
| ), | ||
| wasm_bindgen | ||
| )] | ||
| pub fn calculate(l: &str) -> Result<u64, String> { |
There was a problem hiding this comment.
A big reason this is as ugly as it is is that on wasm32-unknown-unknown the Err variant must be able to be converted into a JsonValue, hence the String error.
Uncertain about wasm32-wasip2.
lrpar/examples/calc_wasm/src/lib.rs
Outdated
| test | ||
| ))] | ||
| mod wasm_bindgen_test { | ||
| use super::calculate; |
There was a problem hiding this comment.
Making these separate modules seemed more clear than the alternative of:
#[cfg(any(all(target_arch = "wasm32", target_os = "unknown", target_vendor = "unknown", test), test))]
mod test {
then
#[cfg_attr(not(all(...)),test)]
#[cfg_attr(all(...), wasm_bindgen_test])
fn test() {...}
wasm_bindgen here kind of wants to take over #[wasm_bindgen_test(unsupported=test)] for tests that should also run on other targets.
But that means bringing in wasm_bindgen as a dependency even for targets where it is unnecessary.
I guess it is worth trying to clean this up via a dev-dependency on cfg_aliases, it is an absolutely tiny macro_rules only crate which seems to be about the same compilation speed as an empty build.rs from my testing.
It seems worth attempting anyways.
There was a problem hiding this comment.
Cleaned up with cfg_aliases in 2a6e491, I personally think the dep pulls it's weight.
considering the duplication of either modules or attributes that we'd otherwise have.
|
I have a very dumb question: what is |
|
It isn't a dumb question at all, The thing it is really trying to show is that bug #461 is actually fixed, i.e. it is more of a So yeah the entirety of
So you are not wrong, it just seems to be the madness of |
Ah, I see! Right, I think I now understand your comment about a |
|
I'll try to see if I can get it working by putting it in Edit:
|
.buildbot.sh
Outdated
| touch .fake_profile | ||
| export WASMTIME_HOME="`pwd`/.wasmtime" | ||
| rustup target add wasm32-wasip2 | ||
| PROFILE=.fake_profile curl https://wasmtime.dev/install.sh -sSf | bash |
There was a problem hiding this comment.
This one doesn't seem to check whether the current install is sufficient before
downloading/going through the install process, so it redownloads wasmtime every time.
I guess we could just check which wasmtime and if we ever need specific versions we can start comparing the output of wasmtime -v
There was a problem hiding this comment.
Kind of a hack, but 99cfb93 checks the PATH for wasmtime before install.
There was a problem hiding this comment.
If/when an appropriate wasmtime is available on Debian, we can edit the docker file in the repo (which buildbot uses). I guess it's either not present or too old?
There was a problem hiding this comment.
I feel it is worth trying, there are versions of rust-wasm and nodejs on debian.
I don't know where this docker file is however.
I'm actually not certain which versions are required, but since wasm-pack has been around and stable for a long time, I would hope the version of nodejs installed there is sufficient.
I don't know what the necessary wasmtime version is, because I've only ever run with the latest version.
https://packages.debian.org/search?searchon=sourcenames&keywords=rust-wasmtime
https://packages.debian.org/search?searchon=sourcenames&keywords=nodejs
There doesn't appear to be a package for wasm-pack.
There was a problem hiding this comment.
The docker file is .buildbot_dockerfile_debian. You'd need to add the appropriate package(s) to the apt-get -y install line. I suggest we give it a go, remove the wasmtime manual installation stuff, and only add that back in if we find we have to. If the version in Debian is good enough, it'll save us some long term maintenance (it also makes CI a bit quicker!).
There was a problem hiding this comment.
Hmm, I don't see a .buildbot_dockerfile_debian could it be just not checked into the repo?
There was a problem hiding this comment.
Thanks if it's alright, I'll rebase onto master once that goes through,
There are some other ci changes like the license check on master which will also help.
There was a problem hiding this comment.
Agreed, if that PR goes in first, it should make this PR easier (I hope).
.buildbot_dockerfile_debian
Outdated
| ARG CI_UID | ||
| RUN useradd -m -u ${CI_UID} ci | ||
| RUN apt-get update && \ | ||
| apt-get -y install build-essential curl procps file |
There was a problem hiding this comment.
I think you need to end this line with \ and remove apt-get -y install from the following line. [I remember this format being fussy, and I think that's what works.]
There was a problem hiding this comment.
Doh, I see it now.
should be fixed in 949e713
.buildbot.sh
Outdated
| wasm-pack test --node | ||
| cargo test --target wasm32-unknown-unknown | ||
| cargo test --target wasm32-wasip2 | ||
| cargo test |
There was a problem hiding this comment.
These would ideally run cargo test --target ... just from the root rather than within this subdir.
However that doesn't seem to work, building the tempfile crate needed for lrpar::ctbuilder.
I don't have a complete understanding of what is going on but it seems like that tries to run the codegen process from within wasi on wasip2, and that runs into some issue with nightly features in the std library.
Thus currently we can only build the targets at top-level but only test them on this cttests.
There was a problem hiding this comment.
I think this was improved in 96f24cb
By disabling those tests that required tempfile.
|
Alright, well I think this now ticks all the boxes for me, assuming the debian packaged versions work out. Edit: I guess one thing is that 96f24cb adds a Perhaps it would be better to move the rustup install away from .cargo to something like |
lrpar/cttests/build.rs
Outdated
| } | ||
|
|
||
| CTLexerBuilder::new() | ||
| .rust_edition(lrlex::RustEdition::Rust2021) |
There was a problem hiding this comment.
One last thought is that I don't see any reason we can't move this build.rs, calc_wasm.l, and calc_wasm.y into a calc_wasm.test file.
|
Please squash. |
|
Squashed. |
|
I think the debian packages didn't actually work, but somehow it still succeeded. Eventually producing the message: Looking at the output here: Now that I'm looking at that, I see that the debian packages Edit: Curiously I'm not familiar with the rest of the output of the |
|
Also, i'm assuming that because trixie isn't released we actually want to do the install via |
|
Since |
|
That is part of the weird part though, I don't actually see the output of that, |
3 participants
This adds a poly-fill crate to fix #461
I don't really know anything about wasm etc, but web_time is a complete drop in replacement for
std::time,On platforms where std::time is available, it is simply just a re-export of std::time as shown in the link.
@stephanemagnenat Curious if you are still interested in testing whether this fixes things for you?