Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Bazel build with emsdk 3.1.7 #149

Closed
martijneken opened this issue Apr 26, 2023 · 2 comments · Fixed by #152
Closed

Fix Bazel build with emsdk 3.1.7 #149

martijneken opened this issue Apr 26, 2023 · 2 comments · Fixed by #152

Comments

@martijneken
Copy link
Contributor

Errors about failure to find @emscripten_npm_linux.

Envoy has a patch that fixes it for me: https://github.com/envoyproxy/envoy/blob/main/bazel/emsdk.patch

I can send the PR to apply this patch. An upgrade of emsdk might be another fix, since there have been some changes:
@PiotrSikora
Copy link
Member

Under what conditions do you see this? I've never seen this on our CI, and I believe it was a transient issue in Envoy.

Anyway, we really should upgrade Emscripten (Enovy would benefit from this as well, since they've been chasing Emscripten issues for a while... cc @phlax).

@martijneken
Copy link
Contributor Author

martijneken commented Apr 26, 2023 •
edited
Loading

Under what conditions do you see this?

Looks like any proxy_wasm_cc_binary build (Bazel + GCC):

$ bazel build @proxy_wasm_cpp_host//test/test_data:canary_check.wasm
... spew of similar errors ...
ERROR: /usr/local/google/home/mstevenson/.cache/bazel/_bazel_mstevenson/556330a676aedc6ff4205aef0ecbb4b0/external/proxy_wasm_cpp_sdk/BUILD:32:11: Compiling proxy_wasm_intrinsics.cc failed: missing input file '@emscripten_npm_linux//:node_modules/rxjs/src/util/toSubscriber.ts'
ERROR: /usr/local/google/home/mstevenson/.cache/bazel/_bazel_mstevenson/556330a676aedc6ff4205aef0ecbb4b0/external/proxy_wasm_cpp_sdk/BUILD:32:11: Compiling proxy_wasm_intrinsics.cc failed: missing input file '@emscripten_npm_linux//:node_modules/rxjs/src/util/tryCatch.ts'
Target @proxy_wasm_cpp_host//test/test_data:canary_check.wasm failed to build

@martijneken martijneken mentioned this issue Apr 26, 2023
Closed
martijneken added a commit to martijneken/proxy-wasm-cpp-sdk that referenced this issue Apr 26, 2023
@martijneken
Patch from Envoy: exclude emsdk targets for Bazel/gcc. Fixes proxy-wa…
afe2a1b 
…sm#149.

Signed-off-by: Martijn Stevenson <mstevenson@google.com>
@PiotrSikora PiotrSikora mentioned this issue Apr 28, 2023
Merged
mpwarres pushed a commit that referenced this issue May 1, 2023
@martijneken
Patch from Envoy: exclude emsdk targets for Bazel/gcc. Fixes #149. (#152
921039a 
)

Signed-off-by: Martijn Stevenson <mstevenson@google.com>
@krinkinmu krinkinmu mentioned this issue Oct 2, 2024
Merged
krinkinmu added a commit to krinkinmu/proxy-wasm-cpp-sdk that referenced this issue Oct 4, 2024
@krinkinmu
Drop emsdk patches in attempt to reproduce proxy-wasm#149
766a3a5 
Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu added a commit to krinkinmu/proxy-wasm-cpp-host that referenced this issue Oct 4, 2024
@krinkinmu
Point cpp-host to my custom cpp-sdk repo
8ee9239 
This is done to reproduce a build error from proxy-wasm/proxy-wasm-cpp-sdk#149

Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu added a commit to krinkinmu/proxy-wasm-cpp-sdk that referenced this issue Oct 4, 2024
@krinkinmu
Update emsdk version to 3.1.67 in Bazel configuration
e6b1ca4 
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 proxy-wasm#149
locally without the emsdk.patch even with the current version of
emsdk used by C++ SDK.

Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu added a commit to krinkinmu/proxy-wasm-cpp-sdk that referenced this issue Oct 7, 2024
@krinkinmu
Update emsdk version to 3.1.67 in Bazel configuration
90d8c07 
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 proxy-wasm#149
locally without the emsdk.patch even with the current version of
emsdk used by C++ SDK.

Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
martijneken pushed a commit that referenced this issue Oct 10, 2024
@krinkinmu
Fix SDK Docker build scripts, update Emscripten (#172)
6b3dc93 
* 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 <krinkin.m.u@gmail.com>

* 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 <krinkin.m.u@gmail.com>

* 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 <krinkin.m.u@gmail.com>

---------

Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Avoid downloading already available NPM modules. martijneken/proxy-wasm-cpp-sdk
2 participants
@PiotrSikora @martijneken