diff options
| -rw-r--r-- | site/designs/index.md | 9 | ||||
| -rw-r--r-- | site/designs/skylark-design-process.md | 93 | ||||
| -rw-r--r-- | site/designs/skylark-parameterized-aspects.md | 304 |
3 files changed, 406 insertions, 0 deletions
diff --git a/site/designs/index.md b/site/designs/index.md index f65368ea76..fc00c38479 100644 --- a/site/designs/index.md +++ b/site/designs/index.md @@ -11,3 +11,12 @@ title: Design Documents {{ doc.date | date_to_long_string }}</a></li> {% endfor %} </ul> + + + +## Skylark Design Documents + +Changes to the Bazel build and extension language (Skylark) should go +through the [Skylark Design Process](/designs/skylark-design-process.md). + +1. [Parameterized Skylark Aspects](/designs/skylark-parameterized-aspects.md). diff --git a/site/designs/skylark-design-process.md b/site/designs/skylark-design-process.md new file mode 100644 index 0000000000..a6c46cb7e2 --- /dev/null +++ b/site/designs/skylark-design-process.md @@ -0,0 +1,93 @@ +--- +layout: documentation +title: Skylark Design Review Process +--- + +# Skylark Design Review Process + +Authors: [Dmitry Lomov](mailto:dslomov@google.com), +[Laurent Le Brun](mailto:laurentlb@google.com), [Florian Weikert](mailto:fwe@google.com) + + +## Motivation + +As Bazel adoption grows, we are seeing both more requests for adding +features to Skylark as a language, and for exposing more and more +functionality available in Bazel internally to Skylark extensions. +These requests originate both within and outside the Bazel team. As +their number grows (and we expect them to grow more and more over time), +addressing those requests meets several challenges: + +* We need to keep Skylark as a language (and a set of +associated APIs/libraries) concise and consistent, easy to learn +and use, and documented. + +* Any APIs or solutions we adopt will be painful to change +down the road, so the more consistent, orthogonal, and open +to future extensions they are, the less pain we and our users +will encounter. + +* It is difficult for engineers - both within the team and outside - +to approach making changes to Skylark. As people attempt to do so, +they experience a lot of friction: patches get written and the discussion +starts where reviewers and the author attempt to solve Skylark +API issues, code design issues, implementation issues, compatibility +issues and so on and so forth all in one code review thread. +The result is friction and frustration, and the quality of the end result +is not guaranteed. + +This document proposes an informal but orderly and well-defined process +for Skylark language changes to address the above challenges. + +## Goals + +* Facilitate healthy growth of Skylark as a language + +* Ensure that Skylark the language has a clear set of experts caring for its development + +* Reduce friction for engineers exposing APIs to Skylark or proposing new Skylark features + +## Non-goals + +* Replace general design review process in Bazel team. +The process described here is strictly about the Skylark language and APIs. +In particular, changes to native rules that do not affect Skylark as a +language much, such as adding/removing an attribute, or adding a new +native rule, are not required to go through this process +(whereas exposing a new provider to Skylark from a native rule is). + +## The Process + +Changes to Skylark, both the language and the exposed APIs, +are evaluated by the Skylark Reviewers Panel. + + +1. The **author** of the proposed change sends the design document to the +[bazel-dev@googlegroups.com] mailing list with a subject containing +"SKYLARK DESIGN REVIEW" + +1. Design doc can be either of: + 1. A universally-commentable Google Doc + 1. A Gerrit code review request with a design doc in + [Markdown](https://guides.github.com/features/mastering-markdown/) + format. + +1. The design doc should include: + 1. Motivation for the change (a GitHub issue link is acceptable) + 2. An example of the usage of the proposed API/language feature + 3. Description of the proposed change + + [A model example design doc](/docs/designs/skylark-parameterized-aspects.md) + (although that is probably an overkill). + +1. **Skylark Reviewers** respond to a document within *2 business days* + +1. **Skylark Reviewers** are responsible for bringing in + **subject-matter experts** as needed (for example, a change involving + exposing a certain provider from cc_library rule should be reviewed by + C++ rules expert as well) + +1. **Skylark Reviewers** facilitate the discussion and aim to reach +a "looks good/does not look good" decision within *10 business days*. + +1. **Skylark Reviewers** operate by consensus. diff --git a/site/designs/skylark-parameterized-aspects.md b/site/designs/skylark-parameterized-aspects.md new file mode 100644 index 0000000000..b99af3f94d --- /dev/null +++ b/site/designs/skylark-parameterized-aspects.md @@ -0,0 +1,304 @@ +--- +layout: documentation +title: Parameterized Skylark Aspects +--- + +# Parameterized Skylark Aspects + +Author: [Dmitry Lomov](mailto:dslomov@google.com), +[Lindley French](mailto:lindleyf@google.com) + +Date: 2016-04-18 + +Status: Approved (Proposal #2), Stage 1 implemented. + + +**Proposal #2 is approved ** + +# Motivation + +When rules apply aspects to their dependencies, they often need to +parameterize these aspects with certain values that depend on rule +instances. Typical example: + +* `python_proto_library` rule (just like other `*_proto_library` rules) + need to generate code for different API versions depending on the + attribute py_api_version in the rule instance + + +In general, a different set of parameters for aspects means not only +different actions that aspects generate, but also a different set of +extra dependencies that aspects introduce (for example, depending on +the value of py_api_version, python proto aspect will depend on +different versions of python protobuf runtime library). + +This functionality is already available for native implementations of + aspects. Native aspects can be parameterized with + [AspectParameters](https://github.com/bazelbuild/bazel/blob/72229431c24ad08f0546b03ede9737b633034e30/src/main/java/com/google/devtools/build/lib/packages/AspectParameters.java): (key,value)-dictionaries, where keys and values are simple strings: + +1. AspectParameters are produced by *parameter extractor*: a function + that works on rule instance and produces an aspect parameter dictionary + based on rule instance attribute values + +2. AspectParameters affect both the aspect definitions ([aspect +definition of a particular aspect class depends on AspectParameters](https://github.com/bazelbuild/bazel/blob/f64730fcff20b7d9428e6bd8471ac057ae1bb3b1/src/main/java/com/google/devtools/build/lib/packages/NativeAspectClass.java)) +and aspect implementations (AspectParameters are available to [ConfiguredAspectFactory.create](https://github.com/bazelbuild/bazel/blob/0773430188885e075121ebf720c82bb05a39db21/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspectFactory.java#L31)) + +This document describes how to expose this + functionality to Skylark in safe and powerful way. + +# Non-solutions and concerns + +*Too much parameterization is bad*. Consider the following strawman: +why cannot we make the entire originating rule instance available to the +propagating aspect? This is very powerful, but it introduces a +_M\*N work problem_: every rule instance originating an aspect will +generate a completely different aspect! Effectively, every rule +originating an aspect will generate an entirely new graph of transitive +dependencies. + +In the same vein, it is desirable to always limit the parameter space +across which the aspect parameters might vary. +The good design of Skylark aspect parameterization needs to account for that. + +*Using different instances of aspects/rules instead of parameters is +unworkable*. It could be argued that, for example, instead of having +a api_version on python_proto_library, we should have several different +rule classes, py_proto_library_<api version>. This is quite unergonomic. +It is barely bearable for *_proto_library case, and completely +impossible for ndk_cc_library where the potential parameter space is +large (for every parameter combination, a new rule class needs to be +introduced; Skylark macros cannot help here, as Skylark macros cannot +introduce new names into global namespace). + +*Increased potential of action conflict.* As it stands now, aspects +output their artifacts to the output directories of the targets they +apply to. This is fragile as unrelated aspects can generate conflicting +actions, and with introduction of parameters the possibility of that +increases (we now have the possibility of the same aspect with different +parameters being applied to a target; aspect author might forget to +disambiguate carefully, leading to subtle and hard to find bugs). + + +# Solutions + +The primary idea for solving the M*N problem is forcing the aspect +author to limit the parameter space and prohibit its accidental +expansion. Instead of having a direct function RI -> AI (where RI is +a rule instance, AI is an aspect instance), we will have (possibly +indirectly) two functions, RI -> P and P -> AI, where P is a finite set +of possible parameter values defined in advance. + +## Proposal #1 + +We introduce the proposal by example (the discussion is below): + +```python +SUPPORTED_API_VERSIONS = ["1","2","3"] + +def _py_aspect_attrs(api_version): + if api_version = "1": + return { '_protoc' : attr.label(default = "//tools/protoc:v1") } + else if api_version == "2": +…. + +def _py_aspect_impl(target, ctx, params): + if params.api_version == "1": …. +py_proto_aspect = aspect(implementation = _py_aspect_impl, + # params declare all aspect parameters with all possible values + params = { 'api_version' : set(SUPPORTED_API_VERSIONS) }, + attr_aspects = ['deps'], + # rhs of attrs can still be dictionary if no dependencies on params + attrs = _py_aspect_attrs, +) +# Can be omitted, see below. +def _py_proto_library_parameter_extractor(py_api_version, some_other_attr): + return { 'api_version' : str(py_api_version), } +py_proto_library = rule(implementation = _py_proto_library_impl, + attrs = { + 'py_api_version' : attr.int() + 'deps': attr.label_list(aspect = py_proto_aspect, + # Can be omitted: the default extractor + # just strs all rule attributes with the same + # names as aspect parameters. + aspect_parameter_extractor = _py_proto_library_parameter_extractor, + ), + 'some_other_attr' : attr.string(), + } +) +``` + + + +Here are the elements we introduce: + +1. Aspects declare their parameters by means of `params` argument to + `aspect` function. The value of that argument is a dictionary from + parameter name to the set of possible values for that parameter. + We require that the parameter space for an aspect is defined upfront. + We reject any parameter values that are not declared in advance. + In this way we address the M*N work problem: we force the aspect + author to limit the parameter space and prohibit its accidental + expansion. + Note: the better expression for this would have been to require params + to always be of certain enumeration type, but we do not have + enumeration types in Skylark. + +2. We allow aspect attributes (essentially the extra dependencies that + aspects introduce) to depend on aspect parameters. To this end, we + allow functions as values of `attrs` argument for `aspect` function. + If the `attrs` argument is a function it is called with aspect + parameters to obtain the attributes dictionary (the parameters are + guaranteed to be within their specified range i.e. set of values). + If `attrs` argument is a dictionary, it is used as is (compatible + with current behavior). + Note: it is possible to extend `attr_aspects` argument in the same way + as well, if needed. + +3. Parameter dictionary is passed as a third parameter to aspect + implementation function. + +4. When rules specify an aspect to apply to their attribute, they can + optionally specify *a parameter extractor* - a Skylark function that + produces a parameter dictionary based on values of rule attributes. + It is an error when a value of parameter produced by a parameter + extractor is not within its specified range. The default parameter + extractor just stores the values of rule attributes with the same name + as parameters of an aspect in question. + +### Implementation stages for proposal #1 + +*Stage 1.* Make the params available to aspect implementation function. This includes: + +1. Adding `params` argument to `aspect` function. + Declared parameters and their ranges become a part of `SkylarkAspect`. + +2. Adding appropriate parameter extractor (just the default one, + str-ing all the relevant attribute values) and introduce the validation + when creating an aspect in `Attribute.SkylarkRuleAspect` + +3. Passing parameter dictionary to aspect implementation function: + see `SkylarkAspectFactory`. + +*Stage 2.* Parameterize Skylark aspect attributes with aspect +parameters. This involves straightforward changes to `aspect` Skylark +function and to `Attribute.SkylarkRuleAspect`. The only tricky thing +there is handling evaluation exceptions from Skylark. + +*Stage 3.* Implement custom parameter extractors: a straightforward +change to `Attribute.SkylarkRuleAspect` (most of error handling should be +in place by that stage). + +## Proposal #2 (alternative to #1) + +In this proposal, aspect parameters are just aspect’s *explicit* +attributes. We restrict the parameter space by requiring all aspect +explicit attributes to have `values` declaration. + +Here is how the pervious example will look like in this proposal: + +```python +SUPPORTED_API_VERSIONS = ["1","2","3"] + +# For rules, configured default function has access to cfg as well, we +# do not support it in aspects +def _py_aspect_protoc(attr_map): + if attr_map.api_version = "1": + return Label("//tools/protoc:v1") + else if attr_map.api_version "2": + … + +def _py_aspect_impl(target, ctx): + if ctx.attrs.api_version == "1": …. + +py_proto_aspect = aspect(implementation = _py_aspect_impl, + attr_aspects = ['deps'], + attrs = { + # For aspect implicit attributes, we allow computed defaults. + # We still require defaults for all implicit attributes + '_protoc' : attr.label(default = _py_aspect_protoc) + # We allow non-implicit attributes. They MUST declare a range of + # possible values, and they MUST be of a limited set of types + # (initially just strings) + 'api_version' : attr.string(values = SUPPORTED_API_VERSIONS) + } +) + + +# Can be omitted, see below. +def _py_proto_library_parameter_extractor(py_api_version, some_other_attr): + return { 'api_version' : str(py_api_version), } +py_proto_library = rule(implementation = _py_proto_library_impl, + attrs = { + 'py_api_version' : attr.int() + 'deps': attr.label_list(aspect = py_proto_aspect, + # Can be omitted: the default extractor + # just passes all rule attributes with the same + # names as aspect non-implicit attributes + # (aka "parameters"). + aspect_parameter_extractor = _py_proto_library_parameter_extractor, + ), + 'some_other_attr' : attr.string(), + } +) +``` + +Here are the elements we introduce: + +1. We limit the types of explicit aspect attributes to "primitive" values + (strings, ints, booleans). + Note: initially those attributes should just be strings in line with + AspectParameters; if we want more types here, we can extend + AspectParameters to support more types. + +2. To facilitate parameterizing aspect dependencies, we allow *implicit* + aspect attributes to have computed defaults, exposed in the same way + computed defaults are exposed to Skylark rules: "default value" of + an attribute can be a function that computes the value given + an attribute map. + Note: computed default functions for Skylark rules have access to + configuration information as well. We cannot support this for aspects + at the moment; we need to clarify the relationship between aspects and + configurations, so this is TBD. + +3. When rules specify an aspect to apply to their attribute, they can + optionally specify *a parameter extractor* - a Skylark function that + produces a parameter dictionary based on values of rule attributes. + The keys of the computed dictionary must match the names of all + non-explicit attributes on the aspect. It is an error when a value of + parameter produced by a parameter extractor is not within its specified + range. The default parameter extractor just passes values of rule + attributes with the same name as explicit attributes of an aspect + in question. + +### Implementation stages for proposal #2 + +(Those stages correspond to implementation stages for proposal #1: at their completion, the same functionality becomes available) + +*Stage 1.* Allow explicit attributes with values restriction on aspects: + +1. Modify `aspect` value. + +2. Add appropriate parameter extractor (just the default one, + passing through all the relevant attribute values) and introduce the + validation when creating an aspect in `Attribute.SkylarkRuleAspect`. + +3. Ensure that explicit attribute values are passed through to aspect + implementation function: see `SkylarkAspectFactory` + +Stage 1 is [impemented](https://github.com/bazelbuild/bazel/commit/74558fcc8953dec64c2ba5920c8f7a7e3ada36ab). + +*Stage 2.* Allow computed defaults for aspect’s implicit attributes. +This involves changes to `aspect` Skylark function and to +`Attribute.SkylarkRuleAspect`. There are two non-obvious parts: + +1. we should not allow computed defaults to be default values of + attributes after AspectDefintion is computed + (i.e. `SkylarkAspect.getDefinition`) + +2. proper error handling is needed here. + +*Stage 3.* Implement custom parameter extractors: a straightforward +change to Attribute.SkylarkRuleAspect (most of error handling should +be in place by that stage). + |