Hi,
On Sun, 2020-07-26 at 16:14 +0900, Tristan Van Berkom wrote:
> Hi all,
>
> This is a proposal for extending the `dependency` attributes on
> element declarations such that elements can extend the dependency
> attributes and participate in parsing them.
This proposal has gone through some iterations and there have been some
roadblocks which have been discussed mostly on IRC.
In this mail, I want to update the list on some of the decisions and
also raise a couple of questions to be resolved.
Updates on design and implementation
====================================
In this email thread, we decided that we want to limit plugins to only
see their build dependencies in this function. Taken a step further, we
also want to establish a new invariant:
Runtime-only dependencies cannot affect cache keys in any way,
an element's artifact output cannot be affected by it's runtime
dependencies.
This results in some churn we will have to make to the Element API,
such that Element plugins have no way of accessing their runtime
dependencies in the loaded data model. This essentially means removing
the `Scope` type from the public API and only give Element
implementations access to the `Scope.BUILD` scope of their own plugin.
New/updated APIs
----------------
The Element.dependencies() API will look like this:
# dependencies()
#
# A generator which yields an element's dependencies.
#
# Args:
# selection (List[Element]): A list of dependencies to select, or None
# recurse (bool): Whether to recurse, or to just report direct
dependencies only
#
# Yields:
# (Element): The element's dependencies, or the collective dependencies of
the
# provided selection.
#
The reason why we add `selection`, i.e. a list of elements which must
be dependencies of the `self` Element on which `Element.dependencies()`
is called, is that we need to operate on lists of elements.
For example, if we want to stage a few dependencies into a specific
path in the sandbox, then we need to iterate over a full list of a
selection's dependencies, in staging order.
Consequently, the Element.stage_dependency_artifacts() also adopts this
new semantic.
Policing dependency visibility
------------------------------
In order to ensure that an element can never see dependencies which are
not relevant to it at build time (ensure runtime-only dependencies are
invisible), we adopt a strategy where the public facing
Element.dependencies() API returns an "ElementProxy"
>From the point of view of an element, this is the same as an Element,
and is not exposed or described in the public API documentation.
Since an Element can only see itself at first, it can only obtain it's
own dependencies with `self.dependencies()`.
Once it has called `self.dependencies()`, an internal list is
maintained for the returned ElementProxy elements, so it will be easily
possible to ensure that calling Element.dependencies() on an
ElementProxy, will never return any elements which is outside of the
Scope.BUILD of the initial plugin which is performing a task.
This part is in progress at:
https://gitlab.com/BuildStream/buildstream/-/merge_requests/2042
Updated implementation plan due to breaking changes
---------------------------------------------------
In the original proposal, we had a smooth transition where we would:
o Add the new Element.configure_dependencies() API.
o Adopt the new approach in existing plugins.
o Remove Element.search() only after existing plugins are migrated.
This is no longer viable due to the necessary breakage in the
Element.dependencies() and related APIs.
As such, the strategy is changed so that:
o First we add the ElementProxy (in !2042)
o Then we introduce the breaking change of the dependency API
o This needs to happen atomically, first BuildStream will pass it's
own tests, then bst-plugins-experimental will have a branch
created which adopts the new API. The change will land in the
master branch in BuildStream when plugins are also working,
and bst-plugins-experimental will also be merged basically at the
same time.
o The Element.search() API will be maintained during this breaking
change
Note that all of this will probably happen in the context of !2042
(none of this will land before the breaking change).
After this happens, we can go back to the original plan, and add
Element.configure_dependencies(), which already has a WIP patch here:
https://gitlab.com/BuildStream/buildstream/-/merge_requests/2032
Update plugins, and finally remove Element.search().
API considerations
==================
One other snake which fell out of all of this, is that we noticed that
there is a decision to make regarding how
Element.configure_dependencies() works.
Currently, an element is never allowed to specify the same dependency
twice in the YAML file, however, with the new `config` node, it becomes
interesting to have the same dependency twice, but configured
differently (for instance, we might want to stage the same element
twice).
Alternatively, elements would need to have more complicated `config`
semantics (for instance, accepting a list of locations to stage a
dependency).
If we allow the same element to be specified multiple times, there is a
question of how `type` and `strict` attributes are resolved (probably
they would be cumulative: if it is a build dependency at least once,
then it is a build dependency, if it is `strict` at least once, then
the dependency is `strict`).
>From an API design perspective, I prefer allowing dependencies to be
specified multiple times, from an implementation perspective, I prefer
not to.
What are your thoughts on this ?
Cheers,
-Tristan
