Fixes to build proxy #141
Fixes to build proxy #141
Conversation
We don't use it. It saves ~500MB on disk / ~50MB on tarball. Cherry-pick of f635b86
We now rely on emsdk package from the host
Signed-off-by: giantcroc <changran.wang@intel.com>
jwendell
force-pushed
the
2.2-build-issues
branch
from
c265221
to
06f4ade
Compare
|
/retest |
There was a problem hiding this comment.
This is awesome, and LGTM overall.
I left some comments/nits/asks; I will leave it up to you to decide if you want to follow up, and if so, if you want to do this in this PR or mop them up via a follow up.
Approving, but I added a label to keep it on hold to give you an opportunity to make a decision with respect to the review comments I left.
| @@ -29,3 +29,8 @@ new_local_repository( | |||
| build_file = "openssl.BUILD", | |||
| path = "/usr/lib64/", | |||
| ) | |||
| new_local_repository( | |||
There was a problem hiding this comment.
nit: add newline (though perhaps we should start using Envoy's fix-format tool and enforce that in CI?).
There was a problem hiding this comment.
+1 for enforcing this in CI.
| @@ -11,7 +11,7 @@ load("@proxy_wasm_rust_sdk//bazel:dependencies.bzl", "proxy_wasm_rust_sdk_depend | |||
| load("@rules_cc//cc:repositories.bzl", "rules_cc_dependencies", "rules_cc_toolchains") | |||
|
|
|||
| # go version for rules_go | |||
| GO_VERSION = "1.15.5" | |||
| GO_VERSION = "host" | |||
There was a problem hiding this comment.
Can we add a one line doc comment to explain the rationale for this change?
There was a problem hiding this comment.
This is a port from https://github.com/maistra/proxy/blob/maistra-2.1/maistra/scripts/update-deps.sh#L97 - I removed that patch from proxy in 2.2.
We use the host's golang (as we use java, gcc, etc from the host)
| @@ -253,106 +253,6 @@ envoy_cmake( | |||
| }), | |||
| ) | |||
|
|
|||
| envoy_cmake( | |||
There was a problem hiding this comment.
perhaps a one line comment about why we don't need this would be helpful?
Also - I wonder - can we figure out a way to minimize this change? E.g. for windows this has tags = ["skip_on_windows"], maybe we would benefit from something similar. In that case this change would fold to a single line + a comment on why we do that. This might make life easier when cutting a branch for a major new release (when we need to rebase/merge)? Just some thoughts.
There was a problem hiding this comment.
This - and other changes in this PR - got lost in the rebase. We did that in previous versions. The reason I did that this way back in 2.1 or 2.0? Lack of knowledge in Bazel. I'd love to simplify things where it's possible. Let's do this in a follow up though.
No description provided.