-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[TESTING/WIP] ci/coverage: Test proxy-wasm-cpp-sdk PR#157 #32432
[TESTING/WIP] ci/coverage: Test proxy-wasm-cpp-sdk PR#157 #32432
Conversation
proxy-wasm/proxy-wasm-cpp-sdk#157 Signed-off-by: Martijn Stevenson <[email protected]>
Signed-off-by: Martijn Stevenson <[email protected]>
Signed-off-by: Martijn Stevenson <[email protected]>
Signed-off-by: Martijn Stevenson <[email protected]>
Hi @martijneken, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
precheck error:
arm64 build error:
|
Signed-off-by: Martijn Stevenson <[email protected]>
The issue is that Envoy CI does a full fetch and dependency check on Lines 33 to 39 in 49425f5
But
I suspect the emsdk update in this PR also picked up a nodejs update, e.g. via this commit: @phlax: can you give advice on how to proceed? |
@phlax Friendly ping on #32432 (comment) Envoy expects all of @nodejs//... to build, but that doesn't seem true in an updated version of NodeJS (16.20.0), which doesn't contain these files/targets: Can we narrow the scope of the Envoy CI check? |
the fetch is not a "check" - its fetching what is required by the check (in a way that is retriable) this likely fetches more than it should (and perhaps not all that it requires) - but was added ~naively as without this, pulling in the nodejs toolchain tends to be very flakey the check itself (as in the build tests) need to stay the same but we can certainly adjust this if we can figure out something that is reliable (which is kinda hard to test) for testing/wip purposes just comment out or remove that line and lets see if that is the only blocker
the problem is introduced here: |
Signed-off-by: Martijn Stevenson <[email protected]>
This uncovered a clang/zlib compatibility issue. Looks like emscripten became stricter: Also seen/patched here: https://gerrit.libreoffice.org/c/core/+/160554 I am not able to repro locally. Theories:
|
This seems to be an intersection between emscripten and zlib so adding @walkingeyerobot for advice.
|
So it does know about the emscripten sysroot and it can see at least some libc-related files in there. This makes me suspect something wrong with how CMake is being invoked. If this was working before, you could try to figure out a way to pass Looking back at some of the comments here, I see cc @trybka |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Test proxy-wasm/proxy-wasm-cpp-sdk#157
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]