Re: [BuildStream] Proposal: Be explicit when overriding junction configuration, or else warn/error

[Tristan Van Berkom <tristan vanberkom codethink co uk>]

Hi Jürg,

On Tue, 2020-04-21 at 11:24 +0200, Jürg Billeter wrote:
> Hi Tristan,
>
> Thanks for writing this up and sorry for the late reply.

Thanks for putting your effort into this complex problem :)

> On Mon, 2020-04-06 at 19:35 +0900, Tristan Van Berkom wrote:
> > [...]
> >
> > Proposed warning/failure
> > ~~~~~~~~~~~~~~~~~~~~~~~~
> > I basically propose that junctions themselves not implicitly override
> > all junctions in the project implicitly, but that they be explicit
> > about which junction configurations they override.
> >
> > If a junction to a common project arises over time which is unaccounted
> > for, then we get an error (or at least a warning).
>
> I agree, I think it's better to be explicit about this. However, there
> are use cases where it's ok to use two different versions or
> configurations of a project in a single pipeline, so we also need a way
> to disable the warning/error for certain junctions. And we might want
> to support such a flag both in the top-level project as well as in the
> inner project.
>
> E.g., a BuildStream project for an application might use a junction to
> get access to static libraries or build-only tools, neither of which
> should be able to cause any conflicts in a potential higher level
> project that includes the application project via junction. The
> application project should thus be able to flag its library junction as
> private/internal and this would then disable the conflict warning/error
> in higher level projects.

Agreed, I think there are good arguments for supporting multiple
junctions into the same project, I can envision this happening with the
same ref/version of the subproject, but with different project options
selected (e.g.: machine architectures).

> However, it should also be possible for a top-level project to
> explicitly state that a particular junction in a subproject should be
> left as is, disabling the conflict warning/error. This should likely be
> possible in the same place where overrides are configured, see below.
>
> With this in place I'd be in favor of making it a fatal error, not just
> a warning. This would allow us to completely drop implicit coalescing.

I think we're talking about coalescing on junction element name, in
which case I agree... see my comments at the end.

> > Proposed Implementation
> > ~~~~~~~~~~~~~~~~~~~~~~~
> > After discussing this on IRC, I am still tending towards my initial
> > proposal of adding an 'overrides' list type option to the junction
> > configuration.
> >
> > This overrides list would specifically address the junctions in
> > subprojects which it overrides, if there are other occurrences of the
> > same project which are not explicitly overridden, then an error (or
> > configurable warning) is raised.
> >
> > Here is an example of project 'foo's declaration of the 'baz'
> > junction,
> > if it were to explicitly override 'bar's junction to 'baz':
> >
> >     kind: junction
> >     sources:
> >     - kind: git
> >       url: ...
> >       ref: ...
> >
> >     config:
> >       overrides:
> >       - bar.bst:baz.bst
>
> I would reverse this such that, in your example, the override is
> configured in project `foo`'s declaration of `bar` (not `baz`). My
> reasoning is that we effectively modify the `bar` subproject, not the
> `baz` subproject.
>
> Also, this change should make it easier to extend the syntax to
> explicitly mark a junction in a subproject to be left as is, i.e.,
> disabling the conflict warning/error without override.

Ok it took me a moment (and a quick IRC ping) to figure this out, I'll
rewrite my understanding of it here to make sure.

Basically, instead of the statement:

   "This junction configuration of baz.bst overwrites this other
    junction configuration"

You would prefer the statement:

   "This junction configuration bar.bst overrides aspects of the bar
    project, by overriding it's baz.bst configuration with this locally
    defined junction"

Sure, I can get behind that :)

> > [...]
> >
> >   - Coalesce junctions by project names
> >
> >     We are still coalescing projects by their junction names, Jürg
> >     pointed out on IRC that he wished we could stop doing this.
> >
> >     This is probably the most confusing part of the junction API, and I
> >     think we should really be using the project name itself rather than
> >     the name of the element which junctions the project.
> >
> >     This may be hard to achieve in the current load process but is
> >     probably ideal, and again I think needs to be set aside as a
> >     separate issue to the one I'm trying to address in this proposal.
>
> If we raise a fatal error, we no longer need any name-based coalescing,
> or am I forgetting something?
>
> We still need a name-based check to determine whether we need to raise
> an error. However, I think for this check alone, it might be simple
> enough to switch to a project name-based check without significant
> additional changes in the load process. Or do you see an issue with
> this?

We definitely need some name to determine that two projects are indeed
the same, in order to ensure that we never accidentally have two of the
same project in the same pipeline with separate configurations.

I think what would happen to the loader code is, while descending
through projects, a junction declaration contains all the context it
needs to explicitly override configurations from the top down (thanks
to your switcheroo above, this is a bit more straight forward
implementation wise).

This allows us to coalesce elements loaded into the pipeline using
explicit declarations rather than the junction name (as the override
statements effectively means "reuse these same elements").

Then, once the load process is complete, we need to add some traversal
of loaded projects which checks if loaded projects which have the same
name, are either explicitly coalesced, or explicitly loaded as separate
dependency chains.

A couple of general questions:

  * Do we need to have support for loading the same project twice in
    the pipeline right away ?

    Or can we leave this as an extensible second step, where initially
    any project that is loaded twice without an explicit override is
    effectively an error ?

  * When thinking about loading the same project twice intentionally,
    I wonder if we can use element dependency semantics in some way.

    If a subproject does not propagate any runtime dependencies forward
    from it's subproject, then perhaps this is a sign that double
    loading of a same project is not problematic ?

Cheers,
    -Tristan