-
Notifications
You must be signed in to change notification settings - Fork 68
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
Error building building example WASM filter #161
Comments
I've seen a very similar looking error when building an extension (though not from the example) with prebuilt protobuf libraries. In my case, it was ultimately resolved by rebuilding those libraries. I took provided Dockerfile to build the image and modified sdk_container.sh by adding the following at the end of the script:
This should build WASM version of the protobuf libraries and puts them into the /sdk directory inside the docker image instead of precompiled versions. Note, I didn't test it extensively, but in the simple WASM filter I used it I didn't see any problems. |
The way the script is currently it does not work (anymore). I'm aware of two issues with the current setup: 1. emsdk underneath installs and uses Node JS and the version of Node JS is not pinned by the script and isn't tied to the version of the SDK used. As a result, emsdk moved to a newer version of Node JS than the one that was probably used when the scripts were created. This new version (it seems, the version currently being used is 18.20.3) requires a relatively fresh version of glibc which just isn't available in Ubuntu Bionic used as a base image. That results in obscure errors from CMake (see proxy-wasm#170) 2. for whatever reason, the version of the protobuf static libraries currently in the repo, don't seem to work. I don't really know how the issue happened, but there were at least 2 users that faced the problem (see proxy-wasm#161). To resolve the first issue, we have a bunch of options: 1. Fix the version of emsdk 2. Fix the version of Node JS 3. Update the version of glibc I figured I can combine options 1 and 3 together for the following reasons: 1. Fixing the version of emsdk fixes the problem 2. The problem arised from the fact that the versions of software used in the build script are a bit old, so an update might be in order, even though we have other solutions to the problem. > NOTE: It's my understanding that fixing emsdk version should also > pin Node JS version, so if we deploy option 1, option 2 seem > redundant. For the second problem, I think there is only one ultimate solution - not store binary artifacts in the repository and instead build them from the sources in the repo. It seems that in the past there was a concern that building protobuf libraries takes a long time - it's still certainly the case. However, I think we can compensate for that in two ways: 1. Drop WAVM - it does not seem like WAVM is still needed (the project itself appears to be dead and hasn't had any updates for at least 2 years), but also one of the comments in proxy-wasm#158, that removed the WAVM from the docs, also suggests that three doesn't seem to be a good reason to keep WAVM in the SDK build script. 2. Take advantage of potential hardware threads in the system when calling make - most laptops or servers these days have multiple cores, so if we have to build protobuf libraries twice, we can speed it up by using more cores. With all those changes made, the build time adds up to something like 32 minutes on my laptop, compared to the 48m without building protobuf libraries from sources (and without adding -j option to make). So I think the additional time spent on building protobuf library is compensated by other changes. Signed-off-by: Mikhail Krinkin <[email protected]>
The way the script is currently it does not work (anymore). I'm aware of two issues with the current setup: 1. emsdk underneath installs and uses Node JS and the version of Node JS is not pinned by the script and isn't tied to the version of the SDK used. As a result, emsdk moved to a newer version of Node JS than the one that was probably used when the scripts were created. This new version (it seems, the version currently being used is 18.20.3) requires a relatively fresh version of glibc which just isn't available in Ubuntu Bionic used as a base image. That results in obscure errors from CMake (see proxy-wasm#170) 2. for whatever reason, the version of the protobuf static libraries currently in the repo, don't seem to work. I don't really know how the issue happened, but there were at least 2 users that faced the problem (see proxy-wasm#161). To resolve the first issue, we have a bunch of options: 1. Fix the version of emsdk 2. Fix the version of Node JS 3. Update the version of glibc I figured I can combine options 1 and 3 together for the following reasons: 1. Fixing the version of emsdk fixes the problem 2. The problem arised from the fact that the versions of software used in the build script are a bit old, so an update might be in order, even though we have other solutions to the problem. > NOTE: It's my understanding that fixing emsdk version should also > pin Node JS version, so if we deploy option 1, option 2 seem > redundant. For the second problem, I think there is only one ultimate solution - not store binary artifacts in the repository and instead build them from the sources in the repo. It seems that in the past there was a concern that building protobuf libraries takes a long time - it's still certainly the case. However, I think we can compensate for that in two ways: 1. Drop WAVM - it does not seem like WAVM is still needed (the project itself appears to be dead and hasn't had any updates for at least 2 years), but also one of the comments in proxy-wasm#158, that removed the WAVM from the docs, also suggests that three doesn't seem to be a good reason to keep WAVM in the SDK build script. 2. Take advantage of potential hardware threads in the system when calling make - most laptops or servers these days have multiple cores, so if we have to build protobuf libraries twice, we can speed it up by using more cores. With all those changes made, the build time adds up to something like 32 minutes on my laptop, compared to the 48m without building protobuf libraries from sources (and without adding -j option to make). So I think the additional time spent on building protobuf library is compensated by other changes. Signed-off-by: Mikhail Krinkin <[email protected]>
* Refresh and fix SDK Docker image build scripts The way the script is currently it does not work (anymore). I'm aware of two issues with the current setup: 1. emsdk underneath installs and uses Node JS and the version of Node JS is not pinned by the script and isn't tied to the version of the SDK used. As a result, emsdk moved to a newer version of Node JS than the one that was probably used when the scripts were created. This new version (it seems, the version currently being used is 18.20.3) requires a relatively fresh version of glibc which just isn't available in Ubuntu Bionic used as a base image. That results in obscure errors from CMake (see #170) 2. for whatever reason, the version of the protobuf static libraries currently in the repo, don't seem to work. I don't really know how the issue happened, but there were at least 2 users that faced the problem (see #161). To resolve the first issue, we have a bunch of options: 1. Fix the version of emsdk 2. Fix the version of Node JS 3. Update the version of glibc I figured I can combine options 1 and 3 together for the following reasons: 1. Fixing the version of emsdk fixes the problem 2. The problem arised from the fact that the versions of software used in the build script are a bit old, so an update might be in order, even though we have other solutions to the problem. > NOTE: It's my understanding that fixing emsdk version should also > pin Node JS version, so if we deploy option 1, option 2 seem > redundant. For the second problem, I think there is only one ultimate solution - not store binary artifacts in the repository and instead build them from the sources in the repo. It seems that in the past there was a concern that building protobuf libraries takes a long time - it's still certainly the case. However, I think we can compensate for that in two ways: 1. Drop WAVM - it does not seem like WAVM is still needed (the project itself appears to be dead and hasn't had any updates for at least 2 years), but also one of the comments in #158, that removed the WAVM from the docs, also suggests that three doesn't seem to be a good reason to keep WAVM in the SDK build script. 2. Take advantage of potential hardware threads in the system when calling make - most laptops or servers these days have multiple cores, so if we have to build protobuf libraries twice, we can speed it up by using more cores. With all those changes made, the build time adds up to something like 32 minutes on my laptop, compared to the 48m without building protobuf libraries from sources (and without adding -j option to make). So I think the additional time spent on building protobuf library is compensated by other changes. Signed-off-by: Mikhail Krinkin <[email protected]> * Update emsdk version to 3.1.67 in Bazel configuration This CL mostly follows the instructions in https://github.com/emscripten-core/emsdk/blob/main/bazel/README.md. Even so, there are a few things that are worth mentioning: 1. I tested the changes locally and in my tests incompatible_enable_cc_toolchain_resolution bazel flag made no difference (e.g. everything builds with or without the flag); I still keep it though because instructions say that it should be there and it does not cause any harm. 2. I don't think that we still need to patch emsdk to exclude npm modules from the toolchain, because it appears that upstream already removed those as a toolchain dependency in emscripten-core/emsdk#1045. It's worth noting, that even though I don't think that emsdk patch is still needed, I actually wasn't able to reproduce the problem reported in #149 locally without the emsdk.patch even with the current version of emsdk used by C++ SDK. Signed-off-by: Mikhail Krinkin <[email protected]> * Update building.md to match updated sdk_container.sh This is a followup to the previous commits that refreshed the build scripts and emsdk. This change reconciles the docs with the current state of the build scripts. Signed-off-by: Mikhail Krinkin <[email protected]> --------- Signed-off-by: Mikhail Krinkin <[email protected]>
I think this issue is addressed by #172. @martijneken @mpwarres it does not seem that I can close this bug (since I didn't open it), but I faced the same issue and it was addressed for my be rebuilding libprotobuf, so I think the PR fixes it. |
Closing per previous comment. Thanks @krinkinmu ! |
Hi,
when I follow the steps outlined in the README for the example on how to compile a C++ WASM filter, on running
docker run -v $PWD:/work -w /work wasmsdk:v2 /build_wasm.sh
The process exits with unsuccessfully because of some linker error. The full output of above docker run execution is:
The text was updated successfully, but these errors were encountered: