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

[rule] Allow deleting attributes from Starlark rule() function #24151

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Yannic
Copy link
Contributor

@Yannic Yannic commented Oct 31, 2024

This is useful for rules like rules_license's license() rule that are typically used via `package(default_package_metadata = [":license"]).

Since there's an implicit dependency from every target in the package to what's declared in default_package_metadata, rules intended for this purpose need to be wrapped in a macro today.

_license = rule(
  # ...
)

def license(**kwargs):
  _license(package_metadata = [], **kwargs)

With this change, wrapping the rule is no longer necessary.

license = rule(
  # ...
  deleted_attrs = ["package_metadata"],
)

This is useful for rules like `rules_license`'s `license()` rule
that are typically used via `package(default_package_metadata = [":license"]).

Since there's an implicit dependency from every target in the package to
what's declared in `default_package_metadata`, rules intended for
this purpose need to be wrapped in a macro today.

```starlark
_license = rule(
  # ...
)

def license(**kwargs):
  _license(package_metadata = [], **kwargs)
```

With this change, wrapping the rule is no longer necessary.

```starlark
license = rule(
  # ...
  deleted_attrs = ["package_metadata"],
)
```
@Yannic Yannic requested review from a team and lberki as code owners October 31, 2024 00:23
@Yannic Yannic requested review from aranguyen and removed request for a team October 31, 2024 00:23
@Yannic Yannic marked this pull request as draft October 31, 2024 00:23
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Oct 31, 2024
@Yannic
Copy link
Contributor Author

Yannic commented Oct 31, 2024

Converting to draft for now since this is purely a POC whether something like this would work at all.

@lberki do you see any obvious issues with exposing something like this to Starlark before I send time creating a design doc?

@fmeum
Copy link
Collaborator

fmeum commented Oct 31, 2024

The macro approach could be "inlined" via the new initializer attribute of rule. Maybe that's sufficient and a new attribute isn't needed?

@lberki
Copy link
Contributor

lberki commented Oct 31, 2024

Not on the implementation side. In fact, RuleClass.Builder.removeAttribute() is used quite a few times in built-in rules. So as far as the implementation goes, it's sound. I think the bigger issue is how to use this: presumably, you'll want to delegate to the implementation of the rule you inherit from in some way, but that implementation will expect the attribute you delete.

I do understand what you want to use this for in license(), but it seems to be a pretty niche use case and if it really is a niche, it's probably better off implemented as a macro as you propose above.

@Yannic
Copy link
Contributor Author

Yannic commented Oct 31, 2024

The macro approach could be "inlined" via the new initializer attribute of rule. Maybe that's sufficient and a new attribute isn't needed?

Mhm I tried that yesterday, but my naive approach of filtering out ["package_metadata", "applicable_licenses"] didn't work, so it seems like propagating attributes from package() to the target happens later than the initializer.. Let me try again with setting them to [] explicitly and report back.

Do I understand correctly that initializer is available in 8.x, but not in 7.x? If so, is this something we could potentially backport?

@Yannic
Copy link
Contributor Author

Yannic commented Oct 31, 2024

Nope initializer doesn't work: Error in license: Initializer can only set Starlark defined attributes, not 'package_metadata'

def _set_package_metadata(**kwargs):
    args = {k: v for k, v in kwargs.items()}
    args["package_metadata"] = []

    return args

@Yannic
Copy link
Contributor Author

Yannic commented Nov 1, 2024

I can see that this feature becomes a foot-gun when extending an existing rule via subrules. Maybe we should only allow it only when there's no parent?

I don't have a usecase outside on license() or package metadata in mind right now. We are working on making this more extendible right now to account for things other than licenses or allow organizations to override / extend what's available in the bzlmod dependency, so I do expect there to be more rules with a need to remove package_metadata. And having this would allow rules_license (or its successor we plan to create in the next week) to create an abstract "parent" rule with the attributes removed to make it easy for users (like we would if it was a native rule)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants