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

[Chandan Singh <chandan chandansingh net>]

Hi James, Sander,

On Fri, Dec 14, 2018 at 10:45 AM James Ennis <james ennis codethink com> wrote:
> 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".

Definitely agree with your amendment.

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

That's a valid point. I had missed this bit when originally writing
this message, but I think that an option like
`--fetch-junctions`/`--no-fetch-junctions` would be useful. We can
debate whether or not should be set by default. But, if we make the
rest of the fetching on by default, I would argue for this to be on by
default as well. However, it should still be configurable by user to
make it on/off by default and tweak it using the CLI when a different
behavior is required.

> > > 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 agree, something `--build` can be interesting for future. Since we
don't have that option anywhere at present, we don't have to worry
about making it consistent just yet. But, if/when we do implement
this, I would suggest that it be implemented in the same way as rest
of the similar options, i.e. with both `--build` and `--no-build`
options, and make it configurable.

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

Thanks for raising this, #401 probably deserves more love :) and the
performance, by default, may suffer with this change, but at least
users will be able to change that (if they wish so) by turning it off
via a config file.

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

Sander, the list has grown slightly since the original message. So, I
would like to add the following new items:

a.) `--pull-buildtrees` was recently added, I would propose that we
add its counterpart `--no-pull-buildtrees` and make it configurable
like the rest of the options proposed.
b.) As mentioned earlier in the message, add an option about fetching
junctions by default.

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

Cheers,
Chandan