Skip to content
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

wasmtime: update to v13.0.0. #368

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rahulchaphalkar
Copy link

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

correct rustix

Signed-off-by: rahulchaphalkar <[email protected]>
@PiotrSikora
Copy link
Member

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

@rahulchaphalkar
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

Rerunning the CI

@PiotrSikora PiotrSikora changed the title Update wasmtime to v13.0.0 wasmtime: update to v13.0.0. Sep 25, 2023
@PiotrSikora
Copy link
Member

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

@rahulchaphalkar
Copy link
Author

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
Copy link
Member

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
Copy link
Author

From what I gathered from actions/runner-images#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
Copy link
Member

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

@rahulchaphalkar
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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.

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.

3 participants