Skip to content
This repository has been archived by the owner on Dec 16, 2020. It is now read-only.

platform: avoid name confliction between platform header and emscripten-libc #550

Merged
merged 7 commits into from
Jul 13, 2020

Conversation

Shikugawa
Copy link
Member

Signed-off-by: Shikugawa [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Name confliction will occur when we build envoy common library by emscripten because of emscripten-libc. We can avoid this problem with this patch.
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

# This is the 1st commit message:

platform: avoid name confliction with emscripten-libc

Signed-off-by: Shikugawa <[email protected]>

# This is the commit message envoyproxy#2:

fix

Signed-off-by: Shikugawa <[email protected]>

# This is the commit message envoyproxy#3:

fix

Signed-off-by: Shikugawa <[email protected]>
@Shikugawa Shikugawa force-pushed the emscripten_platform branch from 22077cc to 102b12c Compare June 18, 2020 15:07
@jplevyak
Copy link
Contributor

Under what circumstance are we including envoy headers in an emscripten build? This should never happen. This is one of the critical features of Wasm: having a clean API/SDK which does not depend on arbitrary envoy headers.

@Shikugawa
Copy link
Member Author

Shikugawa commented Jun 20, 2020

@jplevyak WASM API/SDK has no problem. So it works fine. But we have situations that we must use envoy headers when we build istio-proxy wasm implementation. If we don't use envoy headers in this situation, we must have a lot of duplicated implementations between istio-proxy and envoy. So I think that we should have an ability to accept portion of envoy headers, shouldn't we?

@jplevyak
Copy link
Contributor

@Shikugawa Sounds like we need some refactoring of the envoy headers. This is also the case for the service protobuf used for gRPC. See #538 This PR allows envoy specific code for envoy wasm plugins. We can refactor envoy so that the bits we need are independent and then include them there.

@Shikugawa
Copy link
Member Author

@jplevyak I think that we still need this. It is caused by platform.h and many headers depend on this. After all, we need to use envoy header (not implementation) to implement without redundancy on istio-proxy.

@lizan
Copy link
Member

lizan commented Jul 8, 2020

@jplevyak envoy/common/platform.h is / will be included in almost every header and we should fix it if it doesn't compile in a specific environment. Let's just merge this ?

Signed-off-by: Shikugawa <[email protected]>
@Shikugawa Shikugawa force-pushed the emscripten_platform branch from 0c02ec4 to cb16449 Compare July 11, 2020 10:32
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
@jplevyak
Copy link
Contributor

OK, let's merge this. Do we need the code QL ?

@jplevyak jplevyak merged commit 140d4e5 into envoyproxy:master Jul 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants