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 setting a Skylark rule kind rather than using "magic" assigned identifier #5078

Open
shahms opened this issue Apr 23, 2018 · 7 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: documentation (cleanup) type: feature request

Comments

@shahms
Copy link

shahms commented Apr 23, 2018

Description of the problem / feature request:

Currently, when defining a new rule in Skylark the "kind" is essentially part of the public API. It is user-visible through bazel query and BEP but is taken exclusively from the local identifier on the left hand side of the assignment from the rule() function. This (and some documentation examples) easily leads to the mistaken assumption that the rule kind is an internal implementation detail.

Ideally, there would either be another way of setting the rule kind, possibly through an attribute. Barring that the documentation should be much more explicit about the visibility of this name. For example:

_not_really_internal = rule( ... )
my_lang_library = _not_really_internal # Rule kind is "_not_really_internal" which is public.

def my_lang_binary(...): # Also Rule kind "_not_really_internal"
_not_really_internal(..., kind="binary")

Developers (reasonably) assume that they have control over the exposed API based on the "public" identifiers, but this assumption is false when applied to the return value of rule() and this is non-obvious.

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

Make the visibility of the rule name more explicit and obvious to developers and maintainers.

@meteorcloudy
Copy link
Member

@laurentlb Can you take a look here?

@laurentlb
Copy link
Contributor

cc @c-parsons

@laurentlb laurentlb added P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark and removed category: extensibility > skylark labels Nov 21, 2018
@brandjon
Copy link
Member

Ideally, there would either be another way of setting the rule kind, possibly through an attribute.

The kind is a property of the rule, not the target, so it would be a parameter to rule(), possibly kind=, or even name= for consistency with targets.

This should be straightforward to implement (modulo a little refactoring) -- we'd basically immediately "export" the rule with the given kind/name, instead of waiting for the post-assign hook.

But the real question is whether it's worth the conceptual complexity to add this feature, and whether there's a demand for it.

@brandjon brandjon added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) team-Build-Language and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark labels Feb 16, 2021
@shahms
Copy link
Author

shahms commented Feb 17, 2021

Yes, by "attribute" I meant parameter to the rule() function :-)

This confusion over the fairly common source of breakage when rules are refactored (there are a fair few number of bugs and corresponding # Don't change this name or it breaks <things> b/NNNNNN. comments internally.

I suspect making the rule kind explicit would help reduce the conceptual complexity a bit, rather than adding to it. In particular, it provides a very accessible location from which to hang documentation noting that "kind" is used by bazel query and similar facilities along with noting that if omitted it takes the default from the variable assignment. Right now, this fact is kind of buried in an offhand sentence in https://docs.bazel.build/versions/master/skylark/rules.html#rule-creation rather than being visible in the reference documentation for the rule() function, where all of the other configurable parameters are documented.

@brandjon
Copy link
Member

See also dup #8640.

@brandjon brandjon added P1 I'll work on this now. (Assignee required) and removed P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Jul 14, 2021
@brandjon
Copy link
Member

Prioritized because this was identified as useful for migration of native rules to Starlark.

@shahms
Copy link
Author

shahms commented Mar 21, 2022

Any update's here? It would be great to at least have the caveats around rule kind more loudly documented :-)

@brandjon brandjon added untriaged team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-Build-Language labels Nov 4, 2022
@comius comius added P2 We'll consider working on this in future. (Assignee optional) and removed P1 I'll work on this now. (Assignee required) untriaged labels Dec 29, 2022
@sgowroji sgowroji added the team-Documentation Documentation improvements that cannot be directly linked to other team labels label Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: documentation (cleanup) type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants