proxy-wasm / proxy-wasm-cpp-host

WebAssembly for Proxies (C++ host implementation)
Apache License 2.0
84 stars 69 forks source link

wasmtime: update to v13.0.0. #368

Open rahulchaphalkar opened 1 year ago

rahulchaphalkar commented 1 year ago

Updated wasmtime to v13.0.0, resolved duplicate dependency issues caused by cargo raze.

PiotrSikora commented 1 year ago

Ugh, I suspect that you need to update rules_rust to pull more recent version of Rust first.

rahulchaphalkar commented 1 year ago

I just updated wasmtime in repositories.bzl , and it ran the failing test successfully. I suspect the failures here were due to that. Let me take a look at rules_rust as well.

rahulchaphalkar commented 1 year ago

Ugh, I suspect that you need to update rules_rust to pull more recent version of Rust first.

Without updating rules_rust (after updating wasmtime in above commit), the tests complete when I run them with bazel test --verbose_failures --test_output=errors --define engine=wasmtime --config=clang -c opt -- //test/...

I updated rules_rust to latest release v0.27.0 , which in turn required updating bazel version to 6.3.0, but seemingly bazel 6.x.x is causing some failures, similar to discussed here https://groups.google.com/g/bazel-discuss/c/iQyt08ZaNek

I can work on resolving these issues, but just want to understand if that's fine to do. Or I can use the latest head from rules_rust, where the requirement for bazel 6.3.0 has been reverted.

I'm still not sure if updated rules_rust is required, so if the CI can be rerun to check if the failures still exist, would be helpful.

mpwarres commented 1 year ago

Rerunning the CI

PiotrSikora commented 1 year ago

I'll re-run it after other tests finish, but the failure on Windows looks real.

rahulchaphalkar commented 1 year ago

The windows failure seems to be related to a newly added crate in wasmtime v13.0.0, versioned_export_macros. I'm having a hard time trying to repro it as I don't have a windows system properly set up for development, currently proxy_wasm build fails on my windows system with following error -

ERROR: Traceback (most recent call last):
        File "C:/users/abc/_bazel_rschapha/s5hcskdl/external/rules_rust/rust/private/rustfmt.bzl", line 121, column 24, in <toplevel>
                rustfmt_aspect = aspect(
Error in aspect: aspect() got unexpected keyword argument 'required_providers'
PiotrSikora commented 1 year ago

I'm wondering if this is hitting Windows's Maximum Path Length Limitation, since C:\users\runneradmin\_bazel_runneradmin\dwxiuyix\execroot\proxy_wasm_cpp_host\bazel-out\x64_windows-opt-exec-2B5CBBC6\bin\external\wasmtime__wasmtime_versioned_export_macros__13_0_0\wasmtime_versioned_export_macros-3083915575.wasmtime_versioned_export_macros.cf0192ed-cgu.0.rcgu.o is 280 characters long.

Perhaps updating runner image to windows-2022 (see: #372) would fix it?

rahulchaphalkar commented 1 year ago

From what I gathered from https://github.com/actions/runner-images/issues/4913 windows-2019 image should already have enabled long paths. Can test it if needed by adding this snippet -

- name: Check LongPathsEnabled
  run: |
           (Get-ItemProperty "HKLM:System\CurrentControlSet\Control\FileSystem").LongPathsEnabled
PiotrSikora commented 1 year ago

FWIW, updating CI to windows-2022 in this PR should be a trivial way to see if it fixes the issue.

rahulchaphalkar commented 1 year ago

I've updated CI to use the newer windows image. I suspect git config --system core.longpaths true might be required to enable this, I have not added that yet in CI.

PiotrSikora commented 1 year ago

I've updated CI to use the newer windows image.

Thanks!

I suspect git config --system core.longpaths true might be required to enable this, I have not added that yet in CI.

I don't think this is related, since that file isn't checked into git.

rahulchaphalkar commented 1 year ago

The failures seem to be due to bazel being unable to find msvc tools. Perhaps simply installing the 2022 tools on the system would be sufficient.

mpwarres commented 1 year ago

The failures seem to be due to bazel being unable to find msvc tools. Perhaps simply installing the 2022 tools on the system would be sufficient.

That might not be sufficient on its own, I'm running into the same thing with #375 , using updated runners.

rahulchaphalkar commented 1 year ago

I think I've figured out the issue, and somewhat of a workaround for the Windows CI failure which was occurring on windows-2019 image (not the latest CI update to windows-2022) This is indeed related to the max windows limit of 260 characters, but indirectly. Bazel seems to shorten all paths that are >260 chars to short paths. So a >260 char path like

C:\Users\rschapha\_bazel_rschapha\s5hcskdl\execroot\proxy_wasm_cpp_host\bazel-out\x64_windows-opt-exec-2B5CBBC6\bin\external\wasmtime__wasmtime_versioned_export_macros__13_0_0\wasmtime_versioned_export_macros-3083915575.wasmtime_versioned_export_macros.cf0192ed-cgu.0.rcgu.o

is shortened by bazel to

C:\Users\rschapha\_BAZEL~1\s5hcskdl\execroot\PROXY_~1\BAZEL-~1\X64_WI~2\bin\external\WA973C~1\wasmtime_versioned_export_macros-3083915575.wasmtime_versioned_export_macros.cf0192ed-cgu.0.rcgu.o

It seems the linker and even win utilities like dir are not able to recognize files in this shortened path in my local testing. Bazel previously had this shortening behind a flag, but now is default on.

The workaround is to use bazel startup option --output_user_root (probably in a bazelrc file) to reduce total path length. So, the following works -

bazel --output_user_root=C:\tmp test --verbose_failures --sandbox_debug --define engine=wasmtime  -- //test/... -//test/fuzz/...

and the this passed all tests on my local system. However, I needed to manually delete the C:\tmp directory for rerunning.

Planning on opening a bazel issue as well, and still looking at what is the best way to resolve this.