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

[Jürg Billeter <j bitron ch>]

Hi Tristan,

Thanks for writing this up and sorry for the late reply.

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.

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.

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

> [...]
>
>   - 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?

Cheers,
Jürg