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

Needing to pre-declare whether a rule is executable/test is constraining #19720

Open
illicitonion opened this issue Oct 4, 2023 · 5 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request

Comments

@illicitonion
Copy link
Contributor

illicitonion commented Oct 4, 2023

Description of the feature request:

rule has two arguments: test and executable, which are used to state whether bazel run (and other pieces of machinery) can run the rule, and whether bazel test should test it.

We have encountered a number of situations where this needing to be statically declared on the rule is limiting, and it would be nice to be able to e.g. return this in a DefaultInfo provider or similar, rather than needing to statically pre-declare it.

Some examples:

platform_transition_binary

aspect-lib contains some utility functions for transitioning, but it needs to have a separate platform_transition_binary and platform_transition_filegroup, partially because the rule you're using to transition needs to know whether it set the executable flag. Ideally we could just have one transition_target rule, and not need a separate rule depending on whether what you're transitioning happens to need to be executable. See also this discussion.

rules_go: go_non_executable_binary

This code makes two top-level rules, go_binary and go_non_executable_binary which call the exact same code, and differ only in whether executable is True or False.

A wrapping macro calls one or the other based on whether a binary or library is actually the desired output. But ideally these two rules would be one rule, and the rule implementation itself could determine this from its attrs, rather than requiring the caller to call different rules.

This require some duplication, but also makes query more error-prone. Someone may reasonably write a query like kind(go_binary, ...), and miss some targets because they are actually go_non_executable_binary targets - this feels like it's leaking an implementation detail which should be uninteresting to the user into the public interface of rules.

Which category does this issue belong to?

Configurability, Rules API

What underlying problem are you trying to solve with this feature?

Being able to wrap targets, or transition targets, in a general and flexible way without duplication of code, and in a way that makes it easier to reliably query targets.

Any other information, logs, or outputs that you want to share?

This was discussed with @comius in a recent meeting, and he asked for an issue with more details so we could discuss further.

/cc also @sluongng @fmeum @katre @gregestren who have taken part in related discussions in the past.

@iancha1992 iancha1992 added the team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts label Oct 4, 2023
@comius
Copy link
Contributor

comius commented Nov 3, 2023

There's a good reason to predeclare this. IDEs need to figure out if a target is executable/test or not. Adding anything fancy on top of this, is making this harder. The fanciest thing I know about is genrule(executable = True).

return this in a DefaultInfo provider or similar, rather than needing to statically pre-declare it.

This doesn't seem technically viable. The executable and test bits determine how the rule class looks like. Both classes have additional attributes. This needs to be defined at loading time already, so that we can correctly type check the attributes.

aspect-lib contains some utility functions for transitioning, but it needs to have a separate platform_transition_binary and platform_transition_filegroup

Both look like they should have been some kind of an alias. Those are not implementable in Starlark.

I agree there's a need for an alias or alias-like rule that does a platform transition and the body of evidence we need such a native rule is growing.

rules_go: go_non_executable_binary

This is an antipattern that also happened in java_binary. We're cleaning it up, by providing java_single_jar rule, that replaces the non-executable flavour. In Java the use-cases for non-binary java_binary are different enough that they are better supported by a second rule. The sizes of the implementations of both rules is significantly different.

The motivation for java_binary cleanup are: native rule used the same trick as genrule, Starlark rule is wrapped in macro to make the destinction. This causes problems for IDE integrations, extending the rule, generating the documentation for the rule.

differ only in whether executable is True or False

The implementation function might be the same, but it's likely there are conditionals within it that make the implementation different. If the implementation was the same, then you could probably use just one flavour all the time.

@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Nov 3, 2023
@katre
Copy link
Member

katre commented Nov 3, 2023

As far as a platform-changing alias, the decision to implement it in Starlark was made after previous changes were deemed too invasive. That was a wider set of features than just the platform_data rule we are currently implementing, but reversing back to a native rule at this point seems confusing.

Is there a reason why we don't expose an API for alias-like rules in Starlark?

@comius
Copy link
Contributor

comius commented Nov 3, 2023

Is there a reason why we don't expose an API for alias-like rules in Starlark?

One native alias gave us enough headaches already. IDE folks for example need to special case differences between query and cquery, because there are aliases with selects. Query shows them, and cquery doesn't.

Aspects need to move correctly over aliases and they need to behave as a single targets, and there were several bugs.

Also there are cases for expanding $(location //label) functions in rules that are very tricky to handle when aliases are involved. //label can be either point to an alias or to a target alias point's to.

Ah, to make it more fun, aliases can form chains.

@katre
Copy link
Member

katre commented Nov 3, 2023

I am well aware of the level of problems caused by aliases, which is why I am reluctant to add more. I agree with @illicitonion that it'd be better to fix the Starlark APIs so that this isn't a problem for rules authors.

Copy link

github-actions bot commented Jan 7, 2025

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants