This sounds like a great idea.
In the example, the new config attribute was used under the 'depends:'
key. I was wondering how it would work 'build-depends:' and
'runtime-depends:'.
I'm assuming it could be used under 'build-depends' in exactly the same
way as the regular 'depends'?
But I don't think there would be a meaningful way to use it under
'runtime-depends'. Is that right?
Douglas Winship
On 26/07/2020 08:14, 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 is a proposal aimed at reconciling a few topics which we've been
looking into recently.
* Plugins accessing private API[0]
In Chandan's announcement we underlined the importance of being
able to reduce redundancy in element declarations which stage
elements into different sysroots.
Any element which needs to parse an element name in it's
configuration and later use Element.search() to find that element
and treat it specially, is likely causing the user to enter
redundant information by requiring the user to specify the element
both as a dependency and also in the element configuration.
* Dependencies in a sysroot[1]
This is a proposal from Valentin from two years ago, which
underlines the usefulness of being able to stage elements in
multiple locations (which BuildStream does already, except that it
appears to not be an obvious thing for users).
This proposal would not solve Valentin's proposal by itself, but it
would provide a more convenient API for Valentin to go ahead with
extending the BuildElement with a feature to satisfy his
requirements.
The current way to implement this in BuildStream would be to add
new configuration to BuildElement allowing BuildElement to pick and
choose which dependencies it decides to stage at which paths.
After this proposal, it would allow the BuildElement abstract class
to add syntactic sugar to the dependency objects in the `depends`,
`build-depends` and `runtime-depends` lists, making the final user
facing feature a bit more user friendly.
* Replace Element.search() completely.
The Element.search() API has been broken[2] for a very long time,
as it does not support addressing elements across junction
boundaries.
I created this merge request[3] this weekend to address this, since
we now have a clearer story about project relative element paths,
however there are some problems with the patch.
Namely, in order to make Element.search() performant and avoid
redundantly reloading .bst files from the disk later on, we need to
cache a lot of data which we usually discard from the loader,
increasing our memory footprint by around 12%.
Considering that we've been wanting to provide a solution that is
more tightly coupled to the dependency lists, I think it is best if
we just let Element.search() die completely, as any utility it
provides can be replaced by this proposal.
Proposed format changes
=======================
The Dependency dictionary[4] would gain a single new `config`
attribute, which would be a dictionary who's semantics would be defined
by the owning element.
kind: foo
build-depends:
- filename: payload.bst
config:
#
# The plugin loaded as the `foo` kind can
# now be told anything specific about this
# dependency `payload.bst`.
#
frob-me: True
# Configuration that is parsed by the plugin
# loaded as the `foo` kind in this project
# is specified already on the toplevel `config`
# dictionary.
#
config:
some-config: "Yes please"
Proposed plugin API changes
===========================
The element would gain a new method for parsing dependency
configurations with a similar contract as Plugin.configure().
* This new method would be called after Plugin.configure() and before
Plugin.preflight()
* This new method would be given tuples of (Element, MappingNode) for
each direct dependency, regardless of whether it had specified a
`config` node (so the MappingNode is allowed to be `None` for some
entries).
This allows the plugin to make a judgement about dependencies which
no configuration was specified for.
For example: an element like x86Image might require 2 build
dependencies, one for the tooling to stage at `/` and another for
the payload to deploy at `%{build-root}`, so it could raise an
error if there is a third dependency specified which would be
ignored.
* This new method would be optional to implement, however it will be
mandatory if the user ever specifies a `config` dictionary for any
of the given plugin instance's dependencies.
This is to say that BuildStream core would take care of simply
raising a nice LoadError informing the user that `config` is an
invalid configuration for the said element, if the said element
does not implement any special parsing of dependency
configurations.
Example of Valentin's BuildElement proposal[1]
==============================================
I thought it would be good to include a small example of what the
BuildElement sysroot example might look like in YAML, based on the
`readline.bst` example in his original email:
depends:
- filename: glibc.bst
config:
sysroot: "%{sysroot}"
- filename: ncurses.bst
config:
sysroot: "%{sysroot}"
- filename: system-image.bst
type: build
- filename: sysroot-compiler.bst
type: build
In this example, the BuildElement class would implement
Element.configure_dependencies() in such a way as to collect a list of
all of the dependencies which are supposed to go in specific sysroots,
and stage those in those sysroots instead of `/`.
Implementation plan
===================
To implement this change I would go through 3 phases of implementation.
* Implement the new format and method
This would be done in the BuildStream core repository and
would introduce bare bones plugins in the `tests/` directory
for the purpose of testing the correct behaviors of the
feature.
At the same time, this would result in deprecation of
Element.search().
* Migrate existing plugins
In this phase, we would update the ScriptElement, base
class, the `script` convenience element, and any plugins
in `bst-plugins-experimental` which use `Element.search()`
to use the new `Element.configure_dependencies()` instead.
* Remove Element.search()
Once plugins have been migrated, we would remove
Element.search() entirely.
Any thoughts ? Please discuss :)
Cheers,
-Tristan
[0]:
https://lists.apache.org/thread.html/r057a6fa1ed275fcbc8d92972753bf32ce1e16ab3740580da69be3f3c%40%3Cdev.buildstream.apache.org%3E
[1]: https://mail.gnome.org/archives/buildstream-list/2018-August/msg00009.html
[2]: https://gitlab.com/BuildStream/buildstream/-/issues/931
[3]: https://gitlab.com/BuildStream/buildstream/-/merge_requests/1957
[4]:
https://docs.buildstream.build/master/format_declaring.html#expressing-dependencies
