Add design draft for platform inheritance #37
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
|
||
| Currently, platforms in Bazel (declared with the [`platform` rule](https://docs.bazel.build/versions/master/be/platform.html#platform) do not have any form of inheritance. If users want to declare a platform that is semantically "other platform, with added constraint", they have to copy and paste the constraint values from the first platform to the second, and keep them in sync manually. | ||
|
|
||
| This would be useful for the execution platforms defined by remote execution systems, which are frequently a small set of base platforms, and then additional platforms which take the base and add extra capabilities, such as network access or extra memory. See the GitHub issue at: https://github.com/bazelbuild/bazel/issues/6218. |
There was a problem hiding this comment.
Nit: Clarify "This"? The current wording implies to me "currently users have to do annoying thing X. X would be useful..."
| ``` | ||
|
|
||
|
|
||
| In this example, the `extend` platform will have the constraint values `:banana` (inherited from the parent platform`) and `:clubs` (which overrides the constraint `:heart` set on the parent platform). |
There was a problem hiding this comment.
Formatting: looks like the ` past "platform" is messing with the formatting of following words?
| This would be useful for the execution platforms defined by remote execution systems, which are frequently a small set of base platforms, and then additional platforms which take the base and add extra capabilities, such as network access or extra memory. See the GitHub issue at: https://github.com/bazelbuild/bazel/issues/6218. | ||
|
|
||
|
|
||
| # Proposal |
There was a problem hiding this comment.
Optional suggestion: even though in this design it's small, I might keep the Proposal section just this top paragraph plus an API spec making it clear how the functionality is declared (i.e. with a proposed parent attribute, maybe using our standard API format?) Then shift the example into an Example section.
Again, I get in this proposal the distinction is tiny. But in general I think it's nice to be as explicit and formal about API changes as we're able.
|
|
||
| ## Remote Execution Properties | ||
|
|
||
| The `remote\_execution\_properties` attribute of a platform also needs to be inherited from the parent, but the merging behavior is slightly more complicated because the attribute is a single string. |
There was a problem hiding this comment.
Looks like the \ formatting isn't rendering correctly here and below.
There was a problem hiding this comment.
Can we also hyperlink the attribute?
|
CLAs look good, thanks! |
|
Fixed the formatting issues. I haven't sent this to the list for review yet (according to https://bazel.build/designs/index.html#announce-the-new-proposal that happens after this is merged). When I do I will update this with the link and also change the status to "In Review". |
Related to bazelbuild/bazel#6218.