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

Allow multiple rules to be output from a rule definition #8898

Closed
shs96c opened this issue Jul 15, 2019 · 7 comments
Closed

Allow multiple rules to be output from a rule definition #8898

shs96c opened this issue Jul 15, 2019 · 7 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@shs96c
Copy link
Contributor

shs96c commented Jul 15, 2019

Description of the problem / feature request:

In the selenium project, we want to generate a set of tests from a single set source file. These tests are "one per browser", where the set of browsers is platform dependent. Put another way, a rule definition like this:

selenium_test(
  name = "FooTest",
  srcs = ["FooTest.java"],
  deps = [...],
  browsers = ["chrome", "firefox"] + select({
    "@bazel_tools//platforms:osx": ["safari"],
    "//capabilities:default": [],
  ],
)

should generate FooTest-chrome, FooTest-firefox, and (on macOS) FooTest-safari. Generating the safari tests on linux is clearly never going to work, causing bazel test //... to fail for users unaware of the quirks of the build.

The hack suggested in this comment allows us to pass a platform-specific set of browsers to a rule. However, a rule can only generate one executable output, and we want one executable (that is, test) per browser, meaning that isn't a solution that's viable.

Similarly, if select worked in a macro, then it would be a simple matter of looping, and we'd be done. But select does not work in a macro, so that isn't a viable solution.

If there was an environment declared per browser type, then we could restrict the tests to those environments. The docs for defining environments don't really help, and issues such as #3780 indicate that there's still ongoing work. So that isn't a viable solution either.

If the java_test's restricted_to attribute would accept a constraint (or set of constraints) then this would work as well (since we could use the @bazel_tools//platforms:osx constraint for the example above). This has the added benefit that all rules are fully expanded for every platform. Of course, a constraint is not an environment, so this is not a viable solution either.

Feature requests: what underlying problem are you trying to solve with this feature?

I want bazel test //... to run green, no matter which host platform someone is running on.

Have you found anything relevant by searching the web?

Issue #287 seems relevant, but not quite there. Issue #3780 also seems relevant, but doesn't feature the expansion of the initial set of attributes into a suite of rules.

@shs96c shs96c changed the title Allow multiple rules Allow multiple rules to be output from a rule definition Jul 15, 2019
@laurentlb
Copy link
Contributor

@gregestren, any recommendation for this?

@gregestren
Copy link
Contributor

In my reading using a macro and integrating #3780 would solve this.

The macro would separately instantiate FooTest-chrome, FooTest-firefox, etc. for every supported browser, each with appropriate constraint_values. Then bazel test //:... would automatically exclude the tests that don't match the target platform.

Aside from the fact that #3780 is a work in progress, does this approach miss something?

@shs96c
Copy link
Contributor Author

shs96c commented Jul 18, 2019

That would work provided we can "or" constraints. That is, FooTest-chrome would run if the current platform is linux | windows | macos, but FooTest-safari would have the single constraint of the platform needing to be macos

Generating FooTest-chrome-windows, FooTest-chome-linux, and FooTest-chrome-macos would be possible, but would be hard to follow when using bazel query --output=build on the build file, as well as exploding the number of targets that would need to be generated and built (because each rule would compile the same srcs separately 😱)

@laurentlb laurentlb added team-Configurability platforms, toolchains, cquery, select(), config transitions and removed team-Starlark labels Jul 24, 2019
@gregestren
Copy link
Contributor

Note that @AustinSchuh and I are actively collaborating on a proposal for #3780 right now. We've got a chicken scratch Google doc that we'll share more widely when it's.. a little less chicken scratch

@katre
Copy link
Member

katre commented May 20, 2020

@gregestren: Can you update this and remove the "untriaged" label? Thanks.
Closing or re-naming so that the issue description matches the work we plan to do would be very helpful.

@gregestren
Copy link
Contributor

I still believe we can get existing mechanisms to work with #8898 (comment), and that's preferable to adding native support for selectively combining a single rule's outputs.

As I'm reading, we have these requirements:

  1. Rule designer needs to be able to declare that chrome is compatible with platforms X, Y, and Z. Can that be an explicit list, or would we need something like all platforms or all platforms except V or more complicated logic?
  2. The top-level rule can read the host platform to determine which browser tests run
  3. All tests consume the same source file. I assume what distinguishes them is the command line built around that source file?
  4. bazel test //... never fails due to non-sensical platform incompatibilities.
  5. Should be query-friendly.

While I can think of a few variations, if all tests consume the same source that suggests they're principally part of the same rule.

I wonder if the rule can generate inner actions (actions that aren't declared executable rule outputs) for each browser variation? Then the "executable" is a trivial script that takes the browser-specific outputs as inputs and returns a good status code if each of them also returns a good status code.

If this sounds off, please clarify. If not I can try to mock up an idea of what I'm thinking about.

@gregestren gregestren added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels May 26, 2020
@sgowroji sgowroji added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 15, 2023
@sgowroji
Copy link
Member

Hi there! We're doing a clean up of old issues and will be closing this one. Please reopen if you’d like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

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) stale Issues or PRs that are stale (no activity for 30 days) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

No branches or pull requests

7 participants