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

use foreign_cc to build wee8#152

Closed
lizan wants to merge 2 commits intoenvoyproxy:masterfrom
lizan:wee8_build
Closed

use foreign_cc to build wee8#152
lizan wants to merge 2 commits intoenvoyproxy:masterfrom
lizan:wee8_build

Conversation

@lizan
Copy link
Member

@lizan lizan commented Aug 25, 2019

Signed-off-by: Lizan Zhou lizan@tetrate.io

Description:
To propagate CC context better down to gn/ninja.

Additionally, added use_glib=false so glib is no longer needed.

Risk Level:
Testing:
Docs Changes:
Release Notes:

@lizan
use foreign_cc to build wee8
7b6e520 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan lizan requested a review from PiotrSikora August 25, 2019 01:47
@lizan lizan mentioned this pull request Aug 25, 2019
Closed
@lizan
format
ffb9be1 
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@PiotrSikora
Copy link
Contributor

PiotrSikora commented Aug 25, 2019

Did you see #151? It should address all CC-related problems that you might have.

This is a nice PR, I actually didn't know you can override configure_make this way, so thanks for doing that! Having said that, I'd rather keep this hermetic and working as tested, for the time being, since:

  • This is going to be migrated to BUILD files soon, anyway (I have a working version, but I'm still trying to figure out a weird ASan error).
  • I have a few more PRs staged (pending build: support building wee8 with Clang/libc++ and GCC. #151) to hook sanitizers, update V8, etc. that I'm not sure will work with this approach,
  • I'm trying to get this shipped in 1.3 (i.e. over the next few days), so I'd prefer to avoid unnecessary changes right now,
  • I'm not sure if this solves anything that isn't already addressed by build: support building wee8 with Clang/libc++ and GCC. #151, since it seems that the make step is still executed as a single job and not distributed.

But I'm happy to discuss if you disagree.

@PiotrSikora
Copy link
Contributor

PiotrSikora commented Aug 25, 2019 •
edited
Loading

Since this is separate from this discussion, could you send a PR with use_glib=false (and revert CircleCI image to envoyproxy/envoy-build:cd8574de783791b3353579b489222bfda74888da)?

@PiotrSikora
Copy link
Contributor

PiotrSikora commented Aug 25, 2019

Also, this breaks with GCC (see #151 for how it's fixed there with is_clang).

@lizan
Copy link
Member Author

lizan commented Aug 25, 2019

I tested with libc++ and this work as is. Didn’t test gcc but I guess a couple of select should make it work.

Sanitizer flags should be already passed down in this way so I guess ASAN is already working from the CI result, though I haven’t verified it.

PiotrSikora added a commit to PiotrSikora/envoy-wasm that referenced this pull request Aug 25, 2019
@PiotrSikora
build: drop GLib2 dependency in wee8.
d3254ef 
Suggested by Lizan in:
envoyproxy#152

Since that was the only custom dependency, we can revert bakc to
upstream's build images.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora mentioned this pull request Aug 25, 2019
Merged
PiotrSikora added a commit that referenced this pull request Aug 25, 2019
@PiotrSikora
build: drop GLib2 and Ninja dependencies for wee8. (#153)
8f5993e 
Suggested by Lizan in:
#152

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
PiotrSikora
PiotrSikora reviewed Aug 25, 2019
View reviewed changes
bazel/foreign_cc/wee8.patch
# 5. Increase VSZ limit to 4TiB (allows us to start up to 370 VMs).
--- a/wee8/build/toolchain/gcc_toolchain.gni
+++ b/wee8/build/toolchain/gcc_toolchain.gni
# 6. Remove Bazel added -fuse-ld=gold if gn chose -fuse-ld=lld
Copy link
Contributor

@PiotrSikora PiotrSikora Aug 25, 2019

Choose a reason for hiding this comment

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

Please add: # 7. Disable :compiler_deterministic, because ... (or 6., really).

bazel/foreign_cc/wee8.patch
void free_data();
#else


Copy link
Contributor

@PiotrSikora PiotrSikora Aug 25, 2019

Choose a reason for hiding this comment

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

Nit: extra empty lines between patches.

PiotrSikora
PiotrSikora reviewed Aug 25, 2019
View reviewed changes
bazel/foreign_cc/BUILD
)

configure_make(
name = "wee8",
Copy link
Contributor

@PiotrSikora PiotrSikora Aug 25, 2019

Choose a reason for hiding this comment

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

Nit: sort alphabetically.

@PiotrSikora
Copy link
Contributor

PiotrSikora commented Sep 17, 2019

@lizan do you want to finish this before I start upstreaming V8, or should I upstream it as-is and migrate to BUILD files in a few weeks?

@lizan
Copy link
Member Author

lizan commented Sep 23, 2019

@PiotrSikora let's drop this and migrating to native BUILD that you have :)

@lizan lizan closed this Sep 23, 2019
bianpengyuan added a commit to bianpengyuan/envoy-wasm that referenced this pull request Feb 21, 2020
@bianpengyuan
Add x-goog-user-proj header for sts credential (envoyproxy#152)
1b1fa0e 
* add x-google-user-proj header

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>

* clean up log

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@PiotrSikora PiotrSikora PiotrSikora left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lizan @PiotrSikora