Glad to see we're mostly down to choosing defaults :).
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.
[...]
> > > 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.
Purely from the directory option perspective your proposal is slightly more intuitive, and gives you the semantics to specify a path to the workspace other than generated from the element name. We can debate whether that is desirable or not.
I envision that the majority use is going to be sticking to the configured/default
directory. I derive that from the general desire to type less.
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 ?
The optional name as a path conflicts with the opening of multiple workspaces, which only works if we go with another proposal to disambiguate between path and elements.
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 ?
[...]
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`
This is what I was indeed assuming people would do when creating a project.
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 hear your argument.
> > 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,
I'm always wary about software that goes outside of the directory it was created to operate in. And that is really my only reservation with that option.
and still allow specifying "%{project}/workspaces" as default
workspace location.
However, that would need to be a user configuration.
When you want to get a consistent workflow in an organization, you have a few options:
- stick to the default
- have a site wide [system] configuration that needs to be distributed everywhere (which we currently don't support AFAIK)
- force everyone to run only patched versions of buildstream that includes a different default
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.
I could see that be run from the $(project) directory. Which then leaves element name derived directories in your project directory.
As such I'd prefer no default over ".".
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.
Right. We don't want to see the ability for projects to end up anywhere on the filesystem.
[...moving...]
> 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
I forgot to put
$ mv /old/path/to/element/workspace /new/path/to/element/workspace
> $ bst workspace close --force $element.bst
> $ bst workspace open --directory /new/path/to/element/workspace $element.bst
And the above needed the --no-checkout option.
> 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).
+1.
[...debug...]
Cheers,
-Tristan
Cheers,
Sander
