Skip to content
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

Provide facility to condition on platform during analysis phase #3510

Closed
htuch opened this issue Aug 5, 2017 · 11 comments
Closed

Provide facility to condition on platform during analysis phase #3510

htuch opened this issue Aug 5, 2017 · 11 comments
Assignees
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: other type: feature request

Comments

@htuch
Copy link

htuch commented Aug 5, 2017

With cross-platform builds such as Envoy (https://github.com/lyft/envoy/), we would benefit from the ability to condition the inclusion of different dependencies and tests via either some form of platform tag or something like select but usable inside macros.

Today, we make use of select (https://github.com/lyft/envoy/blob/master/bazel/envoy_build_system.bzl#L35), but this is not flexible enough to indicate "this test should only be applied on Linux, not OS X", see envoyproxy/envoy#1365 (comment). If Bazel natively supported a tag such as "linux_only" or the like, this would make life easier.

@zuercher

@hlopko hlopko added platform: other P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request labels Aug 9, 2017
@katre
Copy link
Member

katre commented Aug 9, 2017

This is roughly similar to #350, which is the ability to use platform() targets in select. This is planned work (and will hopefully happen in the next few months). Please track that issue to follow the work.

@katre katre closed this as completed Aug 9, 2017
@htuch
Copy link
Author

htuch commented Apr 19, 2018

Can we reopen this? I don't see a clearly documented way to condition macro definitions on platform. I'd like ultimately to have something like:

def envoy_some_macro(some_arg):
  if platform is not OS X:
    cc_binary(with some_arg)

@katre
Copy link
Member

katre commented Apr 20, 2018

No, we can't. When macros are expanded, Bazel doesn't know what the target and execution platforms are. A target might have multiple target platforms (for example, it's a tool being built to be used on several different remote workers), and it might be buildable on several different workers (once Bazel finishes analysis and decides what the best ones are).

Macros are, very literally, just macros: you can pretend like the code is mechanically expanding to produce new targets. In the example above, your macro would need to write a target that uses a select to decide, at analysis time, what the attributes are.

@htuch
Copy link
Author

htuch commented Apr 20, 2018

@katre the underlying issue is we need a clean way to condition on platform, that is simple to use and well documented. Take a look at what we had to do in https://github.com/envoyproxy/envoy/pull/3139/files; I needed to add a dummy main to make this work. Is this the best we can do in Bazel?

@katre
Copy link
Member

katre commented Apr 20, 2018

Using a select like that is the right way to go. What does dummy_main provide? Can you add sufficient deps that it works if the osx case of the select returns an empty list? One thing we are trying to add is the ability to use select a little more flexibly, in the near future (a few releases, I think) you should be able to do (heavily simplified):

deps = [ common deps ] 
  + select(
        ':osx': [], # no fuzzing
        ':default': [ ":fuzz_lib" ],
    )

@gregestren for the details on select changes.

@htuch
Copy link
Author

htuch commented Apr 20, 2018

Dummy main provide the main symbol; since the underlying cc_test is a binary target, it expects there to be at least this available.

With the select approach, we nee to condition on both deps and srcs, possibly other args. I like the improvement you sketch out above to reduce boilerplate, would it be possible to add other syntactic candy to just disable targets for specific platforms across the different args?

@katre
Copy link
Member

katre commented Apr 20, 2018

Are you saying you want to not even run the tests on OSX, instead of running the tests but without the fuzzing component?

@htuch
Copy link
Author

htuch commented Apr 20, 2018

Yep, skip the test on OS X. Basically, we have want to condition out some tests on OS X. We could use tags, but then we need to have all OS X users specify tag filters on the CLI or bazelrc. If there is a way to do it with tags and hide this behind the Bazel machinery, we would also consider that.

@gregestren
Copy link
Contributor

gregestren commented Apr 20, 2018

This sounds like https://groups.google.com/d/msg/bazel-discuss/U6sFbPWiXGM/_hjddzwdFwAJ, which discusses automatically skipping tests that aren't CPU-compatible with the build machine.

It might not be a puzzle-perfect fit, since I think it requires the test to positively declare which platforms it's compatible with vs. negatively declare those it isn't. But is this otherwise a relevant approach?

@matzipan
Copy link

matzipan commented Jan 25, 2021

Hi,

While I agree that in the particular case of tests it may make sense to use a different construct, such as select. I do still think there's some room for having constraint-based enabling/disabling of some targets (the original issue title). Some targets may simply not make any sense in some configuration. I'm unsure whether to open a new issue on this topic or raise the concern here.

I don't see any immediate violation to any Bazel principles and I don't see any way direct replacement for this behavior in the current Bazel BUILD dialect.

@katre
Copy link
Member

katre commented Jan 25, 2021

See the recent feature added to skip targets that are incompatible with the current platform: https://docs.bazel.build/versions/master/platforms.html#skipping-incompatible-targets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) platform: other type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants