Re: [BuildStream] [Proposal] 1.4 release (and potential further 1.x releases)

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

Hi Abderrahim,

On Fri, 2019-05-24 at 11:25 +0100, Abderrahim Kitouni wrote:
> Hi all,
>
> As discussed in the meeting, I'd like to backport some features to the 
> stable branch, which means a 1.4 release.

Thanks for compiling this exhaustive list !

As discussed in the last team meeting:

    https://meetbot.gnome.org/buildstream-meetings/2019/buildstream-meetings.2019-05-14-14.00.log.html

I think there are good justifications to rolling a highly conservative
1.4 release.

There are some roadblocks which are starting to be painful in 1.2
(especially hard coded max-jobs), which require action on the one hand,
and on the other hand there are some conveniences introduced in master
that are extremely unlikely to change (e.g. 'build-depends' and cross
junction colon notation for dependencies).

Judging from the meeting, I think there is also large consensus that we
should not be backporting things that are at all likely to change (e.g.
the CLI interfaces).

While technically BuildStream 2 is a separate beast and it will
certainly differ from BuildStream 1 in some ways, it is highly
desirable that their paths do not diverge - for example, if we backport
things from BuildStream 2 and those APIs change for whatever reason,
then we will be stuck with the ugly story of having more than one
porting guide for porting to BuildStream 2, I think we would do well to
stay extremely conservative and avoid any such situation.

I'll comment inline on the proposed backports you've listed below...

[...]
> * Features we want/need: These are either simple, non-intrusive 
> features or features we really want. These are the priority for 1.4
>
>  - Allow source plugins to access previous sources 
> https://gitlab.com/BuildStream/buildstream/merge_requests/568/
>    - followup: Bst track can't track when multiple sources are on the 
> same element without ref 
> https://gitlab.com/BuildStream/buildstream/issues/1010

+1

For some context that others may be missing: We have to deal with some
rust projects in the freedesktop-sdk and gnome-build-meta projects.

For this, we have the excruciating experience of using external scripts
to update the element files which have rust/cargo dependencies, this
happens whenever `bst track` updates a rust project to a version with a
changed Cargo.toml or Cargo.lock file:

    https://gitlab.gnome.org/GNOME/gnome-build-meta/blob/master/utils/generate_cargo_dependencies.py

This is solved in BuildStream 2 for a very long time now and I think it
is the right design and works well, I was about to go ahead and writeup
a `cargo` source plugin (similar to our `pip` source plugin) to remedy
the painful updates, but then noticed that we don't have this API in
BuildStream 1.

>  - Resolve "Make dependency type default to build" 
> https://gitlab.com/BuildStream/buildstream/merge_requests/633

+1

This adds the new `build-depends` and `runtime-depends` conveniences
for expressing dependencies.

>  - loader: Allow dependencies to use ":" to refer to junctioned 
> elements https://gitlab.com/BuildStream/buildstream/merge_requests/998

+1

I'm not sure if you've captured all of the related merge requests, but
there are 2 sides of this which I think are worthwhile "accepting" (but
not "blocking") in a 1.4 release.

  A.) Element files can now refer to cross junction elements using
      colon notation.

  B.) Users can express elements with colon notation on the command
      line.

While we mentioned that CLI changes are more delicate and likely to
change, I think this is entirely safe and uncontroversial; the CLI may
change in other ways in BuildStream 2 but I'm quite certain that it
will still accept colon notation for any element arguments.

>  - git.py: Make `ref` human readable 
> https://gitlab.com/BuildStream/buildstream/merge_requests/291

+1

This doesn't appear to break anything, and people seem to enjoy these
refs to be expressed in long git describe form.

>  - Expose base class for Git source plugins 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1019

>  - Expose _GitMirror as part of plugin author facing API 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1022

-1 for the above two (which are essentially the same).

That merge request is only part of a larger mission, to expose a public
convenience abstract class for git based source plugins to use.

Backporting that "as is" doesnt solve anything, it would just backport
a private class that external plugin authors are still not allowed to
subclass. Further, seeing as it is still a private component, I cannot
speak much towards it's API stability going forward.

>  - Allow customisation of max-jobs instead hardcore to a maximum of 8 
> https://gitlab.com/BuildStream/buildstream/issues/1033

+1

First, we need to get this into master - probably this needs to be
documented such that it only affects local execution, as the rules for
%{max-jobs} needs to be different in remote execution setups.

In the IRC team meeting[0], Emmet (persia) pointed out that such a
setting may be naive, and wanted to discuss more exotic solutions.

I agree that we can do much better than a user configurable max-jobs, I
also think that there is a lot of work and discovery needed before we
can solve this more elegantly.

People are doing very bad things right now (patching BuildStream
checkouts and overriding autotools/cmake/meson elements at the
project.conf level to make builds unrestricted, causing some build
servers to build a bit faster, and causing BuildStream to crash and
burn on other machines).

We need an immediate solution to this, and I think that solution needs
to be a very simple and straight forward user configuration/CLI option
to directly control what %{max-jobs} resolves to.

This is frankly the number one pressing issue for BuildStream 1, and we
need this solution yesterday.


> * Minor incopatibilities: These are features/fixes that bring in small 
> incompatiblities (including breaking cache keys). I believe these are 
> okay, but we should have them in as soon as possible to define the 
> cache keys of 1.4.

To clarify two things:

  * It is not okay to break any API in BuildStream 1.x at all.

    The only exception to this would be for example, if we have not
    bulletproofed the API enough in previous releases and we need to
    add restrictions to avoid people sneaking around and abusing API.

    I.e. an example of this is how we ensured that:

    - People cannot force override automatic variables set by
      BuildStream
    - People cannot refer to local file imports which escape the
      BuildStream project directory

    These are examples of things we did in BuildStream 1.2, while they
    did technically break some projects which were getting away with
    murder in 1.0, these are not API breaks.

  * Cache keys are not API

    If cache keys change, it implies that artifacts need to be rebuilt,
    it does not however imply that an author of a project or plugin
    needs to change anything in order to upgrade to a new version of
    BuildStream, so it is not an API break.

    We do have a desire to also make cache keys stable long term in
    addition to API stability, but even in master we have not yet
    achieved this.

    So far, the only guarantees we have in place for cache key
    stability is that they will not change in a minor point release
    (i.e. 1.2.x cache keys are all the same).

That said, I would like to avoid a cache key bump in 1.4 unless there
is a very good reason to bump it.

> - Execution environment reqs 
> https://gitlab.com/BuildStream/buildstream/merge_requests/969
>  - followup: BuildStream doesn't hit the cache when building foreign 
> arches https://gitlab.com/BuildStream/buildstream/issues/523

On the fence on this one.

 * This certainly breaks cache keys

 * I'm not 100% certain about the stability of these APIs going
   forward, specifically arch names and os names are pretty young,
   IIRC this was part of an initiative to ensure that we can also
   place OS/arch constraints on remote workers in a remote execution
   setting.

Before seriously considering this, can you elaborate as to weather this
is really a pain point for current use of BuildStream 1, and how ?


> - Symlink fixes 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1138

-1

This is a clear API break. If we absolutely need a solution for this in
BuildStream 1, then it needs to be buy in and not API breaking.

> - Do not resolve or mangle symlinks during staging 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1140

-1

This also appears to also be a clear API break, not only at the plugin
level but I suspect also changes how BuildStream creates output.

Either we need a buy-in solution for this, or live with current symlink
behavior until BuildStream 2 - I would probably prefer to just live
with it until BuildStream 2.

> - Update cache keys to use JSON 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1151

On the fence on this one.

As I recall:

  * Some, but not all code changes *might* cause cache keys to
    change due to this bug.

  * Changing this alone, only breaks cache keys.

It might not be needed, and probably not worth it if it is the only
thing which will change cache keys in a given release - if there is
justification for other cache key breaking things, then including this
fix is obviously a good idea.

> - Allow absolute paths in whitelist 
> https://gitlab.com/BuildStream/buildstream/merge_requests/968

+1

As long as this does not *mandate* a leading slash, and as the merge
request says, only additionally *supports* the leading slash, then this
is very obviously the right thing to do and we should backport it.

> * Nice to have: These are mostly UI additions, I'd like to have them in 
> 1.4 if they are believed to be stable, but they can wait for 1.6 if 
> time doesn't permit.

I think it's definitely premature to consider most of this list, and
would prefer to have a completely separate conversation about these
issues waaaay down the road when we start considering a 1.6.

That said, cherry picking from the list below, I think there are a few
things that are safe and could be fair game for a 1.4:

> - Add `--deps build` option to `bst checkout` 
> https://gitlab.com/BuildStream/buildstream/merge_requests/819
> - Add --remote, -r option to bst build, inline with pull & push 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1119
> - _frontend: Allow printing dependencies using `bst show` 
> https://gitlab.com/BuildStream/buildstream/merge_requests/1121

That said, to be honest I would prefer not backporting these at all
unless they are really justified by a user request for a concrete
reason.

I.e. is it problematic for us to work without these features ? If yes,
then let's consider it - let's not consider it just because the feature
looks nice to have and is available in BuildStream 2.

Cheers,
    -Tristan