feat: Add features for dev and prod wasmer configurations #120
Conversation
ThetaSinner
force-pushed
the
add-wasmer-prod-compiler
branch
3 times, most recently
from
b0ba43a
to
384c565
Compare
| wasmer = { version = "=4.3.6", optional = true, default-feature = false } | ||
| wasmer-middlewares = { version = "=4.3.6", optional = true, default-feature = false } | ||
| wasmer = { version = "=4.3.6", optional = true, default-features = false } | ||
| wasmer-middlewares = { version = "=4.3.6", optional = true, default-features = false } |
There was a problem hiding this comment.
This took me way too long to find. This is a horrible little mistake to catch!
There was a problem hiding this comment.
Why does cargo check not catch that?
There was a problem hiding this comment.
Good question! Just put this back and ran cargo check. Doesn't find it.
I guess Cargo is ignoring unknown fields? Which is something I hate with a passion. If people want custom properties, make them use x-property-name and reject all other inputs. We've known this is a good way to design software for 50 years because you can find it in specs from the 70s... /endrant :)
There was a problem hiding this comment.
All good, I blame Cargo, not the typo :)
ThetaSinner
force-pushed
the
add-wasmer-prod-compiler
branch
from
a73e7c4
to
4d4618d
Compare
| wasmer = { version = "=4.3.6", optional = true, default-feature = false } | ||
| wasmer-middlewares = { version = "=4.3.6", optional = true, default-feature = false } | ||
| wasmer = { version = "=4.3.6", optional = true, default-features = false } | ||
| wasmer-middlewares = { version = "=4.3.6", optional = true, default-features = false } |
There was a problem hiding this comment.
Why does cargo check not catch that?
| - name: test root | ||
| run: cargo test --release --no-default-features --features error_as_host,${{ matrix.wasmer-feature }} -- --nocapture | ||
| shell: pwsh |
There was a problem hiding this comment.
Because this is running on Windows only and because its easier to work with than batch on Windows. With the distinction between traditional powershell and powershell core (pwsh), there is now a cross-platform and very capable scripting language option for Windows.
|
Should we add a changelog note describing the different flags? |
| - name: Install LLVM | ||
| env: | ||
| LLVM_DIR: .llvm | ||
| shell: bash |
There was a problem hiding this comment.
Should this also be powershell like the others?
There was a problem hiding this comment.
That would involve me figuring out the black magic bash command that I copied from the wasmer project's CI :)
Given that we're pulling their prebuilt binary, I'd prefer to just use their command to unpack it too
https://github.com/wasmerio/wasmer/blob/main/.github/workflows/build.yml#L270
| ./scripts/fuzz-wasmer_sys_prod.sh | ||
| ./scripts/fuzz-wasmer_wamr.sh |
There was a problem hiding this comment.
Is the current working directory root when running this script or should these also be without the scripts/ in the path?
There was a problem hiding this comment.
Ideally the script would locate itself but if you run ./scripts/fuzz.sh then yes these need to be referenced relative to the CWD of the caller. Given that this isn't in working order anyway, I don't fancy digging deeper into fixing this. I opened an issue on the repo about fuzzing not working without extra tools. It was automated in CI at some point but that's not maintained either.
There was a problem hiding this comment.
Okay. For calling scripts from scripts without depending on being called from a particular location it is possible to get the directory of the current file and run the scripts relative to that with
FILE_DIR=$(dirname "$(realpath "$0")")
"$FILE_DIR/some_other_script.sh"
scripts/test-wasmer_sys_prod.sh
Outdated
| export WASMER_BACKTRACE=1 | ||
|
|
||
| # static tests | ||
| cargo fmt |
There was a problem hiding this comment.
does this not also need to be
| cargo fmt | |
| cargo fmt --check |
Also, does doing it at the root level not do it for everything?
There was a problem hiding this comment.
Oh yes it should be
and you'd think, but this project isn't one workspace. It's actually 2 workspaces and one standalone project. There's the root workspace that contains the host and common crates. Then the test workspace and a standalone guest crate. It drives me potty working on this repo because you can't easily verify changes. I don't think it was done for a reason except maybe to work around historical Cargo issues. I don't think there's an issue open for this, but I'll open one now
There was a problem hiding this comment.
and not just this actually, we weren't denying warnings for Clippy... so we're just printing them in CI. That's helpful :D Sorted that out and fixed the lint issues that of course existed when we don't enforce them not going in
Co-authored-by: Callum Dunster <[email protected]>
No description provided.