Public version of Skylark Design Process.

--
MOS_MIGRATED_REVID=129092214

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).
+