-
Notifications
You must be signed in to change notification settings - Fork 5.2k
deps: update v8 to 13.8.258.26
#40305
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
Conversation
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
agrawroh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! LGTM for deps.
|
@leonm1 Looks like Could you PTAL? |
phlax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leonm1 thanks for this
there appears to be 2 issues
missing @intel_ittapi dep - i tried adding this as an implied_untracked_dep but it appears that it is actually required so will need to be added to repository_locations.bzl etc
dependency on libatomic - there appears to be a dependency on a shared libatomic dep, which envoy cannot ship with
afaict we dont actually need it for arm64 or x86_64 so probably the easiest solution is to patch the v8 build to not include it here https://github.com/v8/v8/blob/0cc9073898540c8f1f8c515a2a3aa214ac67ba6d/bazel/defs.bzl#L184
|
I'm running into a flaky TSAN failure (building on Envoy RBE): I don't see how it's related to V8, but it doesn't flake at I will continue working on this |
|
@leonm1 Feel free to ping me here or on Slack if you are stuck and need any help. TSAN is flaky and is failing on other PRs as well. Hopefully retry would help. |
|
flake discrepancy might be just a caching issue - altho i dont recall seeing that issue previously |
|
Looks like the WASM Filter test is timing out, which could be related to this change: |
|
i think add its sharded but its got normal timeouts - so increasing sharding is another option - but more sharding means more resources etc and often doesnt help dep on the issue |
|
@phlax —
I removed the linker flag for libatomic, and it works properly on x86_64; however, it appears to be required for aarch64 (from proxy-wasm CI): We do cross-compile our aarch64 build, so it is possible it works on a native aarch64 build, but I don't have such a machine to test. |
arm64 vms are for free for OSS on github now |
|
im more concerned about the tsan fail - otoh its the only thing that is failing, otoh it looks more permafail than flake - so wondering why the v8 change would trigger it cc @ravenblackx |
|
I was able to fix the V8 TSAN flake by increasing the listeners bind timeout: Passes with |
|
so wrt patching v8 - we could do that in our repo for now i think this will be revisited for other arches - but if we can make it work for arm/x86 that works for us ideally we reduce/eliminate all patches by upstreaming, but that can be slow i was going to mention about moving the patches upstream from envoy makes sense if its more generally useful - but at least in some cases it was interaction with other deps - eg rules_python - so we might need to ~fork the patches back in future etc - which really underlines why we want to reduce patches as much as possible |
| project_name = "Intel ITT API", | ||
| project_desc = "Intel Instrumentation and Tracing Technology API", | ||
| project_url = "https://github.com/intel/ittapi", | ||
| version = "a3911fff01a775023a06af8754f9ec1e5977dd97", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from 2021. Are we sure that we want to use this version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the version set upstream
see https://github.com/v8/v8/blob/cac6de03372c25987c6cbea49b4b39d9da437978/DEPS#L297
|
cloudflare/workerd#3891 Prior art on libatomic. Workerd does build for aarch64. |
|
@leonm1 i took the liberty of pushing some commits to resolve remaining issues as we are hoping to land this asap before cutting a release i tested the wasm tests - using RBE - locally - seems like all these tests benefit from more cores - and adding for config_test got it over the timeout line im not sure if the sharding is optimal - some shards take ~2s - so probs they are not actually getting any tests - also it seems like bazel uses name based sharding - so i think it might be grouping all the slowest tests into one shard |
Includes the requisite updates to proxy_wasm_cpp_host and new v8 dependencies. Tested with `bazel test test/extensions/filters/http/wasm/... --define wasm=v8` (and `--define wasm=wasmtime` and `--define wasm=wamr` for defense in depth). Signed-off-by: Matt Leon <[email protected]>
Co-authored-by: Rohit Agrawal <[email protected]> Signed-off-by: phlax <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Matt Leon <[email protected]>
Signed-off-by: Matt Leon <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
phlax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @leonm1 - really appreciated
|
As per the comments in the original draft PR - #40235 - the OSSF Scorecard scores for a lot of these new dependencies were bad. Was it possible to address with newer versions or upstream fixes? |
|
the newer version of v8 is what dragged in a bunch of dependencies - and they seemed, at least without even more patching/complexity, required the benefit of updating is that we dont have to worry about a load of cves - so i think we needed to land, not sure if we can encourage upstreams to improve scores - i noticed that there was some history in that respect when i was looking at this today |
Commit Message: deps: update `v8` to 13.8.258.26 Additional Description: Includes the requisite updates to proxy_wasm_cpp_host and new v8 dependencies. Tested with `bazel test test/extensions/filters/http/wasm/... --define wasm=v8` (and `--define wasm=wasmtime` and `--define wasm=wamr` for defense in depth). Proxy-wasm-cpp-host patches come from proxy-wasm/proxy-wasm-cpp-host#442 Fix envoyproxy#28336 --------- Signed-off-by: Matt Leon <[email protected]> Signed-off-by: Ryan Northey <[email protected]> Signed-off-by: Rohit Agrawal <[email protected]>
Commit Message: deps: update
v8to 13.8.258.26Additional Description:
Includes the requisite updates to proxy_wasm_cpp_host and new v8 dependencies.
Tested with
bazel test test/extensions/filters/http/wasm/... --define wasm=v8(and--define wasm=wasmtimeand--define wasm=wamrfor defense in depth).Proxy-wasm-cpp-host patches come from proxy-wasm/proxy-wasm-cpp-host#442
Risk Level:
Testing: Tested with `bazel test test/extensions/filters/http/wasm/... --define
Docs Changes: None.
Release Notes: None.
Platform Specific Features: None.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]
Fix #28336