Re: [BuildStream] bst CLI design and consistency (UX)

[James Ennis <james ennis codethink com>]

Hi all,

On Tue, 2018-12-11 at 15:25 +0000, Sander Striker via BuildStream-list
wrote:

> Hi,
>
> On Sun, Dec 2, 2018 at 12:32 AM Chandan Singh
> <chandan chandansingh net> wrote:
>
> > Hi all,
> >
> > In another recent thread [1], I started discussion for the
> > consistency of the UI that BuildStream presents to users (via CLI).
> > In this message, I want to start a discussion about the UX, i.e. how
> > BuildStream behaves when user specifies a command. I will focus on
> > two specific topics to begin with but I would like to invite others
> > to point out any other areas of improvement, that I may have missed.
> >
> > The principles for the UX seem largely same to the ones outlined in
> > the previous thread so I won’t repeat them all. Most importantly,
> > I think it needs to do the right thing that users expect without
> > having to specify additional options. The right thing is obviously
> > hard to define, but I believe the commands should do whatever it is
> > needed to perform the specified action, except for especially
> > destructive and costly things.
>
> +1.  I would tweak this slightly to "the commands should do whatever
> is needed, _but not more_, to perform the specified action, except
> where especially destructive or costly".
>
> > Fetching sources
> > ================
> >
> > There are a few commands that need to fetch sources, either directly
> > (like `fetch`) or indirectly (like `build`, `workspace open/reset`
> > and `source-checkout`). To highlight the current state of the world,
> > `source-checkout` does not fetch the missing sources by default
> > whereas the rest of the commands do that. While `source-checkout`
> > currently does not fetch sources by default, it provides a `--fetch`
> > option to enable that. Previously there has been some discussion to
> > provide the same option to all other commands as well, and change
> > their default behavior (which will be a breaking change).

Also note that `bst show` requires a fetch if elements in the pipeline
are within a junction. Do you propose that if this is the case, the
junction is fetched implicitly?

> > As mentioned in this thread [2], that fetching the sources by
> > default may be a more natural choice. Fetching sources isn’t a
> > particularly destructive operation, and I cannot think of many
> > circumstances where fetching missing sources will not be desired.
> > Since this has been the default behavior for most commandsI think
> > our users (including me) probably are happy with the current
> > behavior of fetching missing sources by default, as we haven’t
> > recieved any complaints about that so far.
> >
> > So, I would propose that whenever user issues a command that
> > requires fetching sources, we do that unless explicitly told not to
> > do so.

I'd just like to point out that this seems to directly contradict what
we proposed after the last BuildStream gathering where we concluded that
commands that fetch sources implicitly "are doing the wrong thing" [4]
and explictily did the work to introduce the `--fetch` option in bst
source-checkout. However, FWIW, I personally prefer what has been
proposed in this thread.

> > Proposed changes
> > ~~~~~~~~~~~~~~~~
> >
> > 1. Add `--fetch` and `--no-fetch` options to the top-level `bst`
> > command.
> > 2. Add config options to specify the default behavior for fetching
> > sources.

+1, this will make it easy for users who want to retain 'full control'.

> > The reason I propose adding both `--fetch` and `--no-fetch` options
> > is so that even if change our mind about default options in future,
> > scripts and documentation that explicitly use one of these options
> > will not require any changes. They will also be helpful if user’s
> > defaults are different than our defaults.
> >
> > The reason for adding them to the top-level is that they will be
> > required by many commands and it should make it easier to make the
> > behavior consistent and configurable. As a side benefit, it would
> > also make `bst build` less special, and bring it more in line with
> > the rest of the commands. Also, this will also be a a breaking
> > change.
> >
> > Allowing users to control the default behavior further empowers the
> > users to decide how they want BuildStream to behave.
> >
> > Something similar was also proposed in this comment on !NUM. [3]
>
> +1.
>
> > Pulling artifacts
> > =================
> >
> > I think pulling artifacts is analogous to fetching sources, so
> > whatever behavior we agree on for sources should also be applied to
> > the corresponding artifact commands.
> >
> > The command that directly fetched the artifacts is `pull` and the
> > command that indirectly do so are `checkout` and `build`. Similar to
> > above, I propose the following changes for the commands that involve
> > pulling artifacts. So, when doing a checkout, BuildStream should
> > pull the artifacts if they are missing locally, with an option to
> > turn off this behavior.
> >
> > One could argue that BuildStream should also build the element if
> > the artifact if it is missing from the local and remote caches. But,
> > I believe that is taking it a step too far as building is probably
> > the most costly operation that we have and should not be assumed
> > implicitly. That said, perhaps some option could be added in future
> > to build and checkout an element. But, I am proposing that at the
> > moment as we don’t have the need for that now.
>
> Right, I think we want to keep build explicit.

+1000 to not building implicitly, but perhaps the next step would be to
consider a `--build` option for pull, but I don't think this belongs in
this proposal.

I think we also need to be aware that pulling can also be somewhat
costly/ineffective, especially if you have multiple caches declared
(including caches in your junctions), but again, by allowing the user to
configure whether they want to do implicitly pull or not, we can put
control into the users hand.
In light of this, I'd like to take this opportunity to reraise issue
#401 [5], which seems to have fallen through the cracks.
Basically, BuildStream is currently trying to pull artifact's of a
project from a junction project's remote cache.
I suspect that if these proposed changes land, this issue will really
start to become a pain.

> >
>
> > Proposed changes
> > ~~~~~~~~~~~~~~~~
> >
> > 1. Add `--pull` and `--no-pull` options to the top-level `bst`
> > command.
> > 2. Add config options to specify the default behavior for pulling
> > artifacts.
> >
> > The reasoning for these changes is same as the ones listed above for
> > fetching sources.
>
> +1.
>
> > ---
> >
> > I look forward to hearing your thoughts.
>
> Is this the full set of behaviors we are looking to make consistent or
> is there a chance we missed any?
>
> Cheers,
>
> Sander
>
> > [1]:
https://mail.gnome.org/archives/buildstream-list/2018-November/msg00106.html
> > [2]:
https://mail.gnome.org/archives/buildstream-list/2018-October/msg00074.html
> > [3]:
https://gitlab.com/BuildStream/buildstream/merge_requests/970#note_121396134
> > Cheers,
> > Chandan _______________________________________________
> > BuildStream-list mailing list
> > BuildStream-list gnome org
> > https://mail.gnome.org/mailman/listinfo/buildstream-list
> --
>
> Cheers,
>
> Sander
>
> _______________________________________________
>
> BuildStream-list mailing list
>
> BuildStream-list gnome org
>
> https://mail.gnome.org/mailman/listinfo/buildstream-list

Although I've raised some points that I think we should be aware of, the
purpose of this proposal is to make our UX consistent, and it seems that
the proposed changes would be a great way to begin achieving this.
Thanks for taking the time to write this up Chandan!

Cheers,
James

[4]:
https://gitlab.com/BuildStream/buildstream/merge_requests/820#note_110362870
[5]: https://gitlab.com/BuildStream/buildstream/issues/401