You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
arm failure was due to v8's bazel build being out-of-sync with their GN file (patch 31 addresses that)
I still don't completely understand the windows build failure (example logs here), but this change (https://chromium-review.googlesource.com/c/v8/v8/+/7200625) introduced a path which MSVC presumably doesn't really handle well (or rather differently than linux libc++).. In patch 32, I changed the lambda to accept auto&& instead of std::function& and that seems to work.
@fhanau let me know if you're happy with these 2 additional patches
arm failure was due to v8's bazel build being out-of-sync with their GN file (patch 31 addresses that)
I still don't completely understand the windows build failure (example logs here), but this change (https://chromium-review.googlesource.com/c/v8/v8/+/7200625) introduced a path which MSVC presumably doesn't really handle well (or rather differently than linux libc++).. In patch 32, I changed the lambda to accept auto&& instead of std::function& and that seems to work.
@fhanau let me know if you're happy with these 2 additional patches
It appears that the first failure is due to a problem under debug builds, not arm64 builds (we just happen to have the debug CI job use arm64 since it's slightly faster).
I assume this is an issue with the source files themselves not supporting builds with debug mode on (could be one of several debug-related defines) but V8_VERIFY_WRITE_BARRIERS being off (which would be a problem with V8 itself). I think in this case it's less likely that it's an issue with the bazel configuration, which is poorly maintained and does not enable V8_VERIFY_WRITE_BARRIERS in debug builds by default whereas the GN build does, but that mode should still be supported. If this can be fixed on the source level by changing some defines, V8 would probably accept a patch (non-blocking).
Note that we use clang-cl instead of MSVC to build on Windows – MSVC is no longer supported by V8. Not sure what's causing the issue here – could be that clang-cl is not officially supported (regular clang is supported, but comes with a bunch of other headaches on Windows so it would be difficult to use that), could be a difference in compiler versions or an issue with the bazel configuration again. I think the proposed fix is fine – if const auto& would also work I think that's preferable but I'm ok with auto&& fn too.
Oh yes, they're specific to debug builds, you're right. I'll take a look at an upstream patch when I get some time..
For the windows issue - const auto&& doesn't work unfortunately, so I'm going to merge this PR as-is.
Thanks for reviewing!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Patch 29 has been upstreamed: https://chromium-review.googlesource.com/c/v8/v8/+/7124103