Skip to content

fix a few missing includes and ::proxy_wasm namespace pollution with std {} #365

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

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

sitano
Copy link
Contributor

@sitano sitano commented Aug 29, 2023

fixes #364

  • include: add missing dependencies
  • fix: proxy_wasm namespace polution with std defs
  • include: add missing includes to pairs_util and the test
In file included from external/proxy_wasm_cpp_host/src/null/null.cc:17:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm.h:22:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm_plugin.h:18:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/wasm_vm.h:26:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/word.h:22:
external/proxy_wasm_cpp_sdk/proxy_wasm_common.h:59:8: error: no type named 'string' in namespace 'proxy_wasm::std'; did you mean '::std::string'?
inline std::string toString(WasmResult r) {
       ^~~~~~~~~~~
       ::std::string

reproduces:

$ bazel build --config=clang --verbose_failures null_lib

Arch Linux, clang version 15.0.7, gcc version 13.2.1 20230801 (GCC)

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano sitano requested a review from PiotrSikora as a code owner August 29, 2023 14:22
@sitano sitano force-pushed the ivan_fix_includes branch from 386813d to 6f008f5 Compare August 29, 2023 14:34
@sitano
Copy link
Contributor Author

sitano commented Aug 29, 2023

other possible solutions are:

  • move includes into the global namespace but that will break existing API as clients expect now that ::proxy_wasm is populated with the enums and commons from the SDK. they can be aliased into the ::proxy_wasm but that will be a duplication.
  • change all references of std to ::std, but I think its easier just to import the required symbols than the rest garbage (std) from the those headers.

Copy link
Contributor

@mpwarres mpwarres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though can you fix the check format failure?

Also, have you verified that Envoy CI builds successfully if you draft an Envoy test PR that modifies its proxy-wasm-cpp-host dependency to fetch this modified version?

@sitano
Copy link
Contributor Author

sitano commented Sep 26, 2023

👍 will fix the format in nearest weeks 🙌

@sitano
Copy link
Contributor Author

sitano commented Oct 10, 2023

I forgot to signoff the commit that fixes the formatting. So I will have to rebase it for DCO to work...

fixes:

    In file included from external/proxy_wasm_cpp_host/src/null/null.cc:17:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm.h:22:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm_plugin.h:18:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/wasm_vm.h:26:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/word.h:22:
    external/proxy_wasm_cpp_sdk/proxy_wasm_common.h:59:8: error: no type named 'string' in namespace 'proxy_wasm::std'; did you mean '::std::string'?
    inline std::string toString(WasmResult r) {
           ^~~~~~~~~~~
           ::std::string

The headers from https://github.com/proxy-wasm/proxy-wasm-cpp-sdk
include C++ headers that pollute the current namespace with a partial
definition of std{} namespace that then interferes with the names
resolution inside of proxy_wasm as well as duplicates ::std defs.

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano sitano force-pushed the ivan_fix_includes branch from d2177d3 to 31ac2eb Compare October 10, 2023 13:20
@sitano
Copy link
Contributor Author

sitano commented Oct 10, 2023

pushed with fixed DCO

@sitano sitano requested a review from mpwarres October 10, 2023 13:27
@sitano
Copy link
Contributor Author

sitano commented Oct 10, 2023

Also, have you verified that Envoy CI builds successfully if you draft an Envoy test PR that modifies its proxy-wasm-cpp-host dependency to fetch this modified version?

@mpwarres https://github.com/sitano/envoy/tree/ivan_proxy_wasm_fix_test - sitano/envoy@2eeb0ce

$ bazel build --config=clang --verbose_failures -c opt envoy

@yanavlasov
Copy link

I have verified that this PR builds in Envoy.
@sitano @mpwarres please approve and submit, as this change blocks Envoy update of toolchain.

@mpwarres
Copy link
Contributor

Approval needed from @PiotrSikora to submit, thanks!

@PiotrSikora
Copy link
Member

@sitano could you (or @mpwarres or @kyessenov) verify that Istio Proxy builds fine with this? I'd like to verify that it doesn't break Istio's Wasm / NullVM plugins before merging this.

@sitano
Copy link
Contributor Author

sitano commented Oct 24, 2023

@PiotrSikora I can try to do it, but I don't see what to compile. Did you mean https://github.com/istio-ecosystem/wasm-extensions? the problem with this repo is that it uses very old version of the proxy-wasm-cpp-host (f383473) from Nov 18, 2021.

And the latest one (current) has an issue with Rust rules_rust/archive/0.24.1.tar.gz with missing json include at rules_rust/rust/repositories.bzl:825:9: name 'json' is not defined.

Their bazel version is too old to have json in the Starlark language.

@PiotrSikora
Copy link
Member

@PiotrSikora I can try to do it, but I don't see what to compile. Did you mean https://github.com/istio-ecosystem/wasm-extensions? the problem with this repo is that it uses very old version of the proxy-wasm-cpp-host (f383473) from Nov 18, 2021. And the latest one (current) has an issue with Rust rules_rust/archive/0.24.1.tar.gz with missing json include at rules_rust/rust/repositories.bzl:825:9: name 'json' is not defined.

I meant Istio Proxy (https://github.com/istio/proxy).

@mpwarres
Copy link
Contributor

@PiotrSikora I can try to do it, but I don't see what to compile. Did you mean https://github.com/istio-ecosystem/wasm-extensions? the problem with this repo is that it uses very old version of the proxy-wasm-cpp-host (f383473) from Nov 18, 2021. And the latest one (current) has an issue with Rust rules_rust/archive/0.24.1.tar.gz with missing json include at rules_rust/rust/repositories.bzl:825:9: name 'json' is not defined.

I meant Istio Proxy (https://github.com/istio/proxy).

I will try to build with this.

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamping this as the gatekeeper, but please resolve the remaining asks / comments before merging.

to have

    +using WasmResult = internal::WasmResult;

instead of

    -using WasmResult = internal::proxy_wasm::WasmResult;

the patchset fixes:

    In file included from external/proxy_wasm_cpp_host/src/null/null.cc:17:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm.h:22:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm_plugin.h:18:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/wasm_vm.h:26:
    In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/word.h:22:
    external/proxy_wasm_cpp_sdk/proxy_wasm_common.h:59:8: error: no type named 'string' in namespace 'proxy_wasm::std'; did you mean '::std::string'?
    inline std::string toString(WasmResult r) {
           ^~~~~~~~~~~
           ::std::string

The headers from https://github.com/proxy-wasm/proxy-wasm-cpp-sdk
include C++ headers that pollute the current namespace with a partial
definition of std{} namespace that then interferes with the names
resolution inside of proxy_wasm as well as duplicates ::std defs.

Signed-off-by: Ivan Prisyazhnyy <john.koepi@gmail.com>
@sitano
Copy link
Contributor Author

sitano commented Oct 27, 2023

@mpwarres waiting for your input on the latest change to #365 (comment). see https://github.com/sitano/envoy/tree/ivan_proxy_wasm_fix_test with

        version = "0c55e00b465cc8b2692fc1b3412d6d4bea9589c9",
        sha256 = "c276d1ab4df14ec1364d7903aedb08bdbf01e23af40737865955607513c70f0c",

sitano added a commit to sitano/envoy that referenced this pull request Nov 28, 2023
@sitano
Copy link
Contributor Author

sitano commented Nov 28, 2023

@mpwarres @PiotrSikora any desire to finish this one?

@mpwarres
Copy link
Contributor

Yes, sorry about the delay, will merge this tomorrow once I've confirmed Istio builds.

@mpwarres mpwarres merged commit e200fee into proxy-wasm:master Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add missing includes / don't include standard headers in namespaces
4 participants