Skip to content

fix: Update cargo-raze -> crate_universe #399

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

Merged
merged 9 commits into from
Aug 1, 2024

Conversation

martijneken
Copy link
Contributor

@martijneken martijneken commented Jul 26, 2024

Fixes #388.

Had to update platforms for crate_universe compatibility.

Reviewers: ignore generated files (Cargo.Bazel.lock + bazel/cargo/*/remote/)

Steps:

  • introduce crate_universe
  • get wasmsign working
    • support wasmsign2 upgrade
  • get wasmtime working
    • fix //test/fuzz:bytecode_util_fuzzer
    • fix wasmtime Windows build
  • fix .github/workflows/format.yml

Had to update platforms for crate_universe compatibility.
Had to update Abseil due to local build issues.

Wasmsign is not quite working yet (wasmtime not tried yet):
```
$ bazelisk test --verbose_failures --test_output=errors --define engine=null --config=clang -- //test/...
...
ERROR: /usr/local/google/home/mstevenson/git/proxy-wasm-cpp-host/test/test_data/BUILD:22:17: in (an implicit dependency) attribute of wasm_rust_binary_rule rule //test/test_data:abi_export.wasm: @crates_vendor__wasmsign-0.1.2//:wasmsign does not refer to a valid executable target. Since this rule was created by the macro 'wasm_rust_binary', the error might have been caused by the macro implementation
ERROR: /usr/local/google/home/mstevenson/git/proxy-wasm-cpp-host/test/test_data/BUILD:22:17: Analysis of target '//test/test_data:abi_export.wasm' failed
```

Signed-off-by: Martijn Stevenson <[email protected]>
@martijneken martijneken changed the title fix: Update cargo-raze -> crate_universe [DRAFT] fix: Update cargo-raze -> crate_universe Jul 26, 2024
@mpwarres
Copy link
Contributor

  • wasmsign is not working yet, ideas welcome:
$ bazelisk test --verbose_failures --test_output=errors --define engine=null --config=clang -- //test/...
...
ERROR: /usr/local/google/home/mstevenson/git/proxy-wasm-cpp-host/test/test_data/BUILD:22:17: in (an implicit dependency) attribute of wasm_rust_binary_rule rule //test/test_data:abi_export.wasm: @crates_vendor__wasmsign-0.1.2//:wasmsign does not refer to a valid executable target. Since this rule was created by the macro 'wasm_rust_binary', the error might have been caused by the macro implementation

I noticed that the wasm_rust_binary rule seems to want to invoke wasmsign as an executable: invocation and related setup. However the auto-generated build rules for wasmsign build it as a library here.

Maybe something needs to be tweaked to coax crate_universe to generate a binary rather than library build rule for wasm-sign? It's odd because wasm-sign appears to be defined as a binary in its source repo here.

@mpwarres
Copy link
Contributor

It's odd because wasm-sign appears to be defined as a binary in its source repo here.

Lol, ignore that comment, wasm-sign != wasmsign. wasmsign also provides an executable binary here, so I don't know why crate_universe doesn't generate a proper rule for it.

@martijneken
Copy link
Contributor Author

The windows error is Unable to start binary: Os { code: 267, kind: NotADirectory, message: "The directory name is invalid." }. This comment claims it's related to a Windows path length issue (though I don't see long paths in the command copied below). I may try to update rules_rust, which I need to do anyway.

bazel-out\x64_windows-opt-exec-2B5CBBC6\bin\external\rules_rust\cargo\cargo_build_script_runner\cargo_build_script_runner.exe
bazel-out/x64_windows-opt-exec-2B5CBBC6/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script_.exe
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.out_dir
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.env
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.flags
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.linkflags
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.linksearchpaths
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.depenv 
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.stdout.log 
bazel-out/x64_windows-fastbuild/bin/external/crates_vendor__windows_x86_64_msvc-0.48.5/windows_x86_64_msvc_build_script.stderr.log

@mpwarres
Copy link
Contributor

The windows error is Unable to start binary: Os { code: 267, kind: NotADirectory, message: "The directory name is invalid." }. This comment claims it's related to a Windows path length issue (though I don't see long paths in the command copied below). I may try to update rules_rust, which I need to do anyway.

Reading through bazelbuild/rules_rust#1120, it sounds like the issue hasn't been fixed in rules_rust yet, though there is mention of workarounds that involve patching rules_rust to shorten auto-generated workspace names. We might need to do that.

(The particular scheme used in that comment, of taking the first letter of each _ separated comment, seems a little collision-prone to me, I wonder if using a SHA hash of the path name would be safer.)

@PiotrSikora
Copy link
Member

I may try to update rules_rust, which I need to do anyway.

I'd suggest starting from that, since the current version is more than a year old, and crate_universe was still relatively new at that point.

This moves ProxyWasm past Envoy Abseil 20230802.1.

Relevant ASAN failure:
```
external/com_google_absl/absl/strings/numbers.cc:199:73: runtime error: unsigned integer overflow: 0 - 8 cannot be represented in type 'unsigned long long'
    #0 0x562c730539f9 in absl::lts_20230802::(anonymous namespace)::EncodeTenThousand(unsigned int, char*) numbers.cc
    proxy-wasm#1 0x562c73053f25 in absl::lts_20230802::numbers_internal::FastIntToBuffer(unsigned long, char*)
```

Relevant Abseil rollback: abseil/abseil-cpp@e7858c7

Signed-off-by: Martijn Stevenson <[email protected]>
@martijneken
Copy link
Contributor Author

Re: fuzzing failures, it turns out Envoy's version of Abseil (abseil-cpp-20230802.1) has a bug in an optimization that was rolled back. Upgrading to the latest LTS release (abseil-cpp-20240116.2) fixes this. CI running to confirm.

Shorten generated paths with repository_name = "cu"

- old: `@wasmtime__humantime__2_1_0//:humantime`
- new: `@crates_vendor__humantime-2.1.0//:humantime`
- fixed: `@cu__humantime-2.1.0//:humantime`

Signed-off-by: Martijn Stevenson <[email protected]>
@martijneken
Copy link
Contributor Author

Woohoo, I successfully hacked around the Windows path length issue:

  • old: @wasmtime__humantime__2_1_0//:humantime
  • new: @crates_vendor__humantime-2.1.0//:humantime
  • fixed: @cu__humantime-2.1.0//:humantime

Signed-off-by: Martijn Stevenson <[email protected]>
martijneken added a commit to martijneken/proxy-wasm-cpp-host that referenced this pull request Aug 1, 2024
Pick up this fix: abseil/abseil-cpp#1187

Bump past Envoy to pick up another fix found in fuzz tests:
proxy-wasm#399 (comment)

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

Woohoo, I successfully hacked around the Windows path length issue:

  • old: @wasmtime__humantime__2_1_0//:humantime
  • new: @crates_vendor__humantime-2.1.0//:humantime
  • fixed: @cu__humantime-2.1.0//:humantime

That's a great find/fix!

Would keeping the old prefix work on Windows? While it's probably not an issue, due to the use of maybe(), this static prefix results in multiple definitions for some crates (e.g. cu__wasi-0.11.0-wasi-snapshot-preview1 is generated by both wasmtime and wasmsign).

If you cannot use the full prefix (i.e. wasmtime & wasmsign), then perhaps a shortened versions (i.e. wt & ws) would be better than a shared prefix?

@martijneken
Copy link
Contributor Author

multiple definitions for some crates

Good point, but perhaps this is a benefit? Any reason to load the same dep twice?

@PiotrSikora
Copy link
Member

Good point, but perhaps this is a benefit? Any reason to load the same dep twice?

Debugging build issues... but having both pointing to the same dependency is probably better for the happy path, so let's leave it as-is.

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Thanks!

@martijneken martijneken changed the title [DRAFT] fix: Update cargo-raze -> crate_universe fix: Update cargo-raze -> crate_universe Aug 1, 2024
@PiotrSikora
Copy link
Member

FYI, macOS failures are due to using too old OS version on the runners (macos-11 vs macos-12, macos-13 and macos-14 supported by GitHub).

Perhaps another PR updating it to macos-13 is in order (macos-14 runs on Apple Silicon, so it might require a bit more than simply bumping the version number).

martijneken added a commit that referenced this pull request Aug 1, 2024
Bump Abseil to fix Linux build issues

Pick up this fix: abseil/abseil-cpp#1187

Bump past Envoy to pick up another fix found in fuzz tests:
#399 (comment)

Signed-off-by: Martijn Stevenson <[email protected]>
@martijneken martijneken merged commit 330c459 into proxy-wasm:main Aug 1, 2024
8 checks passed
@martijneken martijneken deleted the remove-raze branch August 1, 2024 16:19
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.

[CI] cargo-raze is uninstallable abandonware, adopt crate_universe
3 participants