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

[Chandan Singh <chandan chandansingh net>]

Hi all,

To summarize the actions based on this thread, I have created #819
[1]. I am listing the actions here as well for people who are
interested in a summary of changes:

* Add `--fetch`/`--no-fetch` as top-level options, with support for
user config, and remove similar options from existing commands.

* Add `--fetch-junctions`/`--no-fetch-junctions` as top-level options,
with support for user config.

* Add `--pull`/`--no-pull` as top-level options, with support for user
config, and remove similar options from existing commands.

* Add `--no-pull-buildtree` as top-level option (`--pull-buildtree`
already exists), with support for user config.

If something doesn't seem right, please let us know by replying to
this email, or by commenting on the GitLab issue.

Cheers,
Chandan

[1]: https://gitlab.com/BuildStream/buildstream/issues/819

On Mon, Dec 17, 2018 at 1:53 PM Chandan Singh <chandan chandansingh net> wrote:
> 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