build: Support tags[] arg for more specific build control#8233
build: Support tags[] arg for more specific build control#8233htuch merged 2 commits intoenvoyproxy:masterfrom greenhouse-org:add-tags-arg
Conversation
Where the underlying bazel primitives support tags[], envoy_() should support them. Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: William Rowe <wrowe@pivotal.io>
|
@lizan do you mind taking a look at this? |
| visibility = ["//visibility:private"], | ||
| srcs = [], | ||
| deps = [], | ||
| tags = [], |
There was a problem hiding this comment.
why do you need tags even for api_proto_library? If you're not building the library/binary depends on them they're not built anyway.
There was a problem hiding this comment.
You raise a good point, we introduced tags[] wherever we observed that the underlying bazel API's supported them, but this case may not be necessary for our purpose of excluding from Windows. It might be best to support them where they are valid, though?
There was a problem hiding this comment.
We do omit some parameter as "opinionated" wrapper of underlying bazel API and add tags/options for Envoy specific. I'm not feeling strongly on this so either way is fine.
| visibility = ["//visibility:private"], | ||
| srcs = [], | ||
| deps = [], | ||
| tags = [], |
There was a problem hiding this comment.
We do omit some parameter as "opinionated" wrapper of underlying bazel API and add tags/options for Envoy specific. I'm not feeling strongly on this so either way is fine.
bazel/envoy_test.bzl
Outdated
| repository + "//test:main", | ||
| ], | ||
| deps = select({ | ||
| "@envoy//bazel:windows_x86_64": [repository + "//test:dummy_main"], |
There was a problem hiding this comment.
This is not needed at the end, and shouldn't included in this PR?
There was a problem hiding this comment.
We saw this exclusion for Apple so we added it for Windows.
We don't have a strong opinion either way and we can remove this change.
There was a problem hiding this comment.
@achasveachas the exclusion you mentioned is for fuzz I think? No platform should be excluded from normal cc_test.
Signed-off-by: William Rowe <wrowe@pivotal.io> Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io> Signed-off-by: William Rowe <wrowe@pivotal.io>
…#8233) Support tags[] arg for more specific build control. Where the underlying bazel primitives support tags[], envoy_() should support them. Risk Level: Low Testing: Local on Windows and Linux CI Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
…#8233) Support tags[] arg for more specific build control. Where the underlying bazel primitives support tags[], envoy_() should support them. Risk Level: Low Testing: Local on Windows and Linux CI Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
…#8233) Support tags[] arg for more specific build control. Where the underlying bazel primitives support tags[], envoy_() should support them. Risk Level: Low Testing: Local on Windows and Linux CI Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
Description:
Support tags[] arg for more specific build control.
Where the underlying bazel primitives support tags[], envoy_() should support them.
Risk Level: Low
Testing: Local on Windows and Linux CI
Docs Changes: None
Release Notes: None