GNOME.org
 

Re: [BuildStream] Proposal: Workspace related DX features & design

Hi all,

I like Tristan's idea of --open better than --multi but I don't think
it is very intuitive from users' point of view. It definitely allows
us to provide a very feature-rich CLI but that might come at the cost
of user experience.

As a user, I would probably expect to run `bst workspace open foo.bst
bar.bst` and not end up with a workspace for foo.bst in a directory
called bar.bst. In my opinion, it is intuitive for this command to
open workspaces for both elements since that is exactly how the other
two workspace commands (close and reset) work, i.e. by accepting a
list of elements as arguments. --open would introduce a weird
asymmetry between `open` and other workspace commands. (open is
already different from the other two commands, but I think we should
try to close the gap instead.)

If we are allowed to break API once, I would like to propose the
following syntax:

    bst workspace open ELEMENT [ELEMENT ...] [--directory DIR]

This approach has a downside that users will not be able to specify
different locations for different elements in the same bst invocation.
But I don't think that that is a very common use-case and that the
directory argument, in general, might see less use once we allow
configuring default locations for workspaces. Even though it will be a
bit restrictive, I think it will still do what it says on the tin,
i.e. open workspaces for all elements in the directory specified by
--directory option, which can default to '.' ($CWD).

Just my 2 cents. Thoughts?

Cheers,
Chandan

On Tue, Sep 4, 2018 at 12:09 PM Tristan Van Berkom via
BuildStream-list <buildstream-list gnome org> wrote:


Hi all,

Ok so let's pick this thread up at the end... I will prefix some
answers from the previous mails not in this one and then continue to
respond inline.

  "We propose using a `fragment` file that we will create in a
   .bst/project.yml of a workspace's root directory. This is similar to
   how git submodules identify which directories in the main project
   are subdirectories."

+1 agree.

Having this state recorded in the workspace itself is more desirable
than having a system wide or user wide cache.

  "Support for multiple projects pulling a workspace is not allowed by
   default but it can be forced with known side-effects. Default
   behaviour is to say a workspace already exists at a given location
   with a given name. If forced to be a workspace for multiple projects
   then the original parent project takes precedence (i.e. is the
   default resolution unless overridden) (bst commands again need to be
   forced & warn the user that it may affect other projects consuming
   the shared workspace)."

This sounds sensible enough to me as well.

However, note that I expect there to be a fair amount of edge cases
around this, as well as relative workspace paths in general.

Basically this boils down to failing well in the case that something
went wrong, and informing the user when something goes missing, holding
their hand and assisting them to recover when they have moved things
around (so it means lots of tests, and not too much supporting code).


On Mon, 2018-09-03 at 09:43 -0400, Sander Striker wrote:

Hi,

On Mon, Sep 3, 2018 at 2:59 PM William Salmon
<will salmon codethink co uk> wrote:

I think myself and Tom agreed with Sanders last remarks so we have
been a bit slow in responding.

As Tristan has been quite on this I amuse he is similarly minded,
if not if he could reply sooner rather than later then it will
avoid myself and tom codeing this up and then having a too lengthy
MR rework.

I have made some notes further down as well.



Responses inline.


Thanks All
Will

PS. Tristan i know you are supper busy wit 1.2 atm but if you could
fine moment this week, as we will probs start next week that would
be really appreciated.


On 28/08/18 23:11, Sander Striker wrote:

Hi,

On Tue, Aug 28, 2018 at 1:12 PM Tom Pollard <tom pollard codethink co uk> wrote:

Hi Sander,



[...]


    When opening a single element then you can:

    bst workspace open --directory /path/to/workspaces --multi
    elementA.bst elementB.bst


Why do we need the --multi argument?  That doesn't seem intuitive.  If
it is to get around the fact that otherwise elementB.bst would be
interpreted as the workspacename, then I don't think we should do that.
Otherwise you end up with an open workspace carrying the name of the
second element you wanted a workspace open for.


Yes it is to mitigate the situation you've mentioned. Will has some
thoughts about handling this with click in alternative ways, however as
it stands the most sane thing seemed to be the use of an additional flag.


I'm opposed to the --multi flag.  I don't think it's intuitive.

I am definitely concerned about this invocation in the current proposal resulting in:
$ bst workspace open elementA.bst elementB.bst
... opens up a workspace for elementA.bst in a directory called /path/to/workspaces/elementB.bst...

I think it makes more sense to in this case make a backward
incompatible change to the CLI, for the benefit of a better user
experience.


I read this as: we all dislike the --multi flag but we can think of a better way?
So I think we are agreed to go with the --multi/-m flag.



It's beyond dislike.  I really don't think we should do that based on
the example I've given above.


I concur with Sander here that it is beyond dislike, however
acknowledge that there is a need for disambiguation.

This might or might not be possible without breaking existing API, the
disambiguation is important regardless.

Remember that we are designing a CLI, and that click is just a vehicle
that we use for convenience - if it does not provide a feature we need,
then this limitation only makes things a bit more difficult to work
around, but we should not consider the library as a blocker to us
designing semantics which it does not support natively.

Further than the `-multi` argument being discussed, I will also add
that I very much dislike the added `--directory` argument.

I think that it is desirable to continue supporting paths for the
workspace name argument following the element name.

  o This avoids breaking the API (I accept if we need to break
    it once, but not breaking it is still preferable).

  o When implementing a default directory for opening a workspace,
    the workspace name argument following the element name can
    still be a path.

    - The specified path can be an absolute path, which disregards
      the configured directory.

    - If the specified path is a relative path, it can be considered
      relative to the default workspace directory, if one has been
      configured.

    - If the path is not specified, we simply generate the name from
      the element name and place it in the default workspace directory.

I think this would be a simpler and more comprehensive CLI api.

Am I missing something ? Is there a concrete need for adding this
additional `--directory` option and not treating the optional name as a
path ?


With that said, I have a counter proposal for `bst workspace open`
semantics to support opening multiple workspaces at once.

Here is the idea:

  Single workspace:
  ~~~~~~~~~~~~~~~~~
  bst workspace open [open options] element.bst [directory]


  Multiple workspaces:
  ~~~~~~~~~~~~~~~~~~~~
  bst workspace open [open options] element1.bst [directory] \
          --open [open options] element2.bst [directory2] \
          --open [open options] element2.bst [directory3] \
          ...

Basically, when scanning the argv, we need to treat every set of
arguments between consecutive `--open` option as a unique set of
arguments for opening a given workspace.

In the case that `--open` is specified, the initial set of options for
the first workspace can be optional, one can equally just specify a
list of `--open` statements to the `bst workspace open` command.

  o This allows maximum flexibility.

    Anything can be specified for each workspace being opened
    respectively.

  o There is no room for ambiguity.

    Any set of options between `--open` separators are distinct
    sets, there are no "rules" about whether you must or must not
    specify the [directory] optional argument for each workspace.

  o I think it's elegant.

    That might just be a matter of taste, but I think the flow
    of opening many workspaces at once is nice this way.

  o It is easily scriptable.

    Given a set of input elements, one can easily and reliably
    construct a list of `--open` statements and feed them directly
    to `bst workspace open`.

  o The limitation is that none of the workspace opening options may
    ever be named `--open`, as this would conflict with the separator.

    Not a huge limitation.


As mentioned above, this will be *tricky* to implement, it will require
adding some custom hooks to how click parses the arguments for this
command specifically.

But semantically it makes good sense I think.

Thoughts ?


[...]

    The directory option specifies the directory, in which the folder
    containing/(the root of) the workspace will be created. The folder
    containing/(the root of) the workspace is either the "workspacename"
    from the
    command line or if not given then it is the element name without the
    trailing
    ".bst".

    eg. `bst workspace open --directory /path/to/workspaces elementA.bst`

    would create:
    path/to/workspaces/elementA/
    as the root or the workspace

    and `bst workspace open --directory /path/to/workspaces element.bst
    workspacename`

    would create
    path/to/workspaces/workspacename/




[...]

What is the sensible default location used when no workspace location is
defined?  'workspaces' as a subdirectory of the project directory?  Or
will not having it set result in a helpful failure message?


My preference would for be the command to behave as is to not break any
compatibility with any existing automated workspaces. As such I'd
propose to keep the existing helpful failure message, all be it reworded
to include the availability of defaults. Having workspaces as
sub-directories of a project is also a workflow in which Tristan has
been an advocate of not recommending (see issue #191), so making this
default behavior would be somewhat challenging that.



I'm ok with challenging Tristan on this one as I am still not
convinced that it's that bad (Hi Tristan ;)).  However I am not
necessarily opposed to have the helpful failure message with
clear instructions how to configure.  What would result in the
best user experience?


Hi Sander :)

So, I don't feel that strongly as to disallow workspaces in the project
directory, but I prefer not encouraging it.

The project directory represents the input of BuildStream itself, and
is presumably usually stored as a part of some VCS. The workspace
directory itself belongs to the user who opened the workspace, and is
not project data.

Where there is a risk that people can hurt themselves, we maybe should
not disallow it, but we probably shouldn't recommend it.

In the BuildStream project itself, I have seen various merge requests
which add some random stuff to the merge request, like the cache or
preferences from some random contributor's random IDE of choice, which
decided to store their stuff directly in the BuildStream repository
checkout instead of somewhere sensible, like "~/.cache/${ide}/..." ,
and the contributor accidentally committed and submitted this stuff.

If it happened to us, I have no doubt that it will happen that people
accidentally submit merge requests to BuildStream project repositories
which propose the addition of a workspace to the BuildStream project.

Conversely, it is altogether plausible that the user chose a simple
name for their workspace and stored it in the project directory - and
then the user is hurt when they `git pull` from the upstream project
and it fails to update because the remote added a file which conflicts
with their workspace.

Arguably, this could be done sensibly with cooperation from the
upstream project maintainers, e.g.:

  o The convention might be to put workspaces into ${project}/workspaces

  o The upstream project should then add `workspaces/` to their `.gitignore`

And then things should work out well in general; but we should not
assume people will follow rules or do recommended things, as the
upstream we should probably encourage a workflow that is safest for
everyone, and let power users do things the way they want, at their own
risk.


I think I agree with Tristan on preference but I agree with sander
that it doesn't make scene to disable the feature if the user wants
it. It would be nice if this was something you could force but not
default to tho.. however i think we are all agreed to have the
default location if a default has not been set to be the location
the command was run. eg, the project root or one of its sub
folders.



I'm fine with the default of not being configured, and requiring a
one-time action from the user - on the premise that the error message
gives clear instructions on how to configure.  I'm assuming this is
not too controversial?


I've given some rationale for my stance on why we shouldn't recommend
the project directory as a place for storing workspaces, but I don't
think this needs to be controversial.

Rather, I wouldn't be against a format for the default workspace
directory including the project directory as a reference, e.g. one
could configure:

   workspace-directory: "%{project}/../"

To specify that a workspace should always be opened *beside* the
project, and still allow specifying "%{project}/workspaces" as default
workspace location.

I think that simply specifying ".", should indicate the $CWD at the
time `bst workspace open` was called.

Also I think that "." would be a sensible default, allowing us to avoid
the questions of what happens when there is no default - This is also
fairly inline with the current default behavior, so it could allow us
to avoid breaking API as well.


As a final point on the topic of defaults, I've seen the words
system/user/project config, thrown around haphazardly, let's be clear
on this point:

  The location where a user chooses to store their workspaces is a user
  choice, not a project choice.

  If there is a setting, it is set in `~/.config/buildstream.conf`, and
  the default for it is set in BuildStream's `data/userconfig.yaml`
  file.



[...]

What is the recovery mode for a moved workspace that can't find its
project?  Can it be re-registered?


Will mentioned that a user could 'fudge' the path given in a workspace
fragment which would allow for manual intervention in the case of
re-registering. Ultimately that's a bit messy so going forward with a
utility (a new flag for bst workspace?) for the re-association of
workspaces to projects is probably the right thing to do.


+1.  I don't really like the idea that we end up with an FAQ that tells you to edit the 'fragment'.


How 'smart'
this command would be, especially when trying to generate a list of
potential parent projects, is something that would need to be more
fleshed out if desired.



I think we'll be ok if we ask for confirmation to replace all
associations with just the association to this single project.


I feel like this is a lot of effort when we could just say "we
don't support moving projects", just clone/copy the project with
out the .bst folder and set up new workspaces your self.

That said if we want a mover utility function/tidier its not a lot
of effort so we could easily right it.



I prefer stating that we don't support moving over suggesting someone
"...could 'fudge' the path given in a workspace fragment...".

I imagine from the user perspective this could look something like:
$ cd $project_dir
$ bst workspace close --force $element.bst
$ bst workspace open --directory /new/path/to/element/workspace $element.bst

which I think is acceptable.  The individual workspace commands should already be supported, unless I'm 
missing something?


I think you are correct here, just force close and reopen pointing the
to newly relocated directory.

The key here is to ensure that when we fail, we fail with the correct
hand holding and messaging to the user, informing them what happened
and providing them a path to recovery (hopefully well in advance of,
god forbid, having to resort to searching through an FAQ).


[...]

I feel that half of the proposal is non-contentious and the other half
is around Debug.  From the above I am not clear there are going to be
any conflicting workspace CLI changes needed, it seems not.  Can we
separate out Debug on that basis?


I agree here that the majority of cli changes needed (or as we currently
see as needed) here are for bst shell. However there is definitely
overlaps & the point of using cached build trees (& especially if they
are to become by default what is currently checked out into a workspace)
are places that we feel benefit from being in the overarching design
proposal. If deemed necessary I'd take no issue with the core design &
implementation of extending debugging with bst shell to be separated out.



That would be my preference if there are no objections.


Again I think we all agree but we all needed to have a think about it, and we have, so lets move on.



Good to see we have consensus.


Yes +1 here too, let's have considerations for thing which might be
needed, but avoid scope creep in this proposal.

Cheers,
    -Tristan

_______________________________________________
BuildStream-list mailing list
BuildStream-list gnome org
https://mail.gnome.org/mailman/listinfo/buildstream-list
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]