Remove `--directory` option for `cylc install` #4823

MetRonnie · 2022-04-13T10:26:25Z

These changes close #4676 and partially address #4786

Remove --directory option for cylc install and instead allow any directory in the source arg (cylc install [SOURCE]):

Implicit relative path (e.g. foo/bar): look for source relative to each path in global.cylc[install]source dirs and install first match
Explicit relative path (e.g. ./foo/bar): source is relative to PWD
Absolute path (e.g. /home/user/foo/bar): source is at that path
No path specified: source is PWD

Rename --flow-name option to --workflow-name and add -n for short (the consensus in #4786 seemed to be --workflow-name is preferable to --id).

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg and conda-environment.yml.
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at Update workflow installation docs cylc-doc#461.

Also improve help descriptions of options

Merge behaviour into renamed arg WORKFLOW_NAME -> SOURCE

hjoliver

Looks good 👍

hjoliver · 2022-04-13T20:36:50Z

cylc/flow/pathutil.py

+    ''',
+    re.VERBOSE
+)
+"""Matches relative paths that are explicit (starts with ./)"""


Just curious, is this path format officially called "explicit relative"? If you invented the term, I quite like it 👍 Although arguably it's really an absolute path that goes through $PWD.

I can't seem to find any terminology for relative paths for differentiating between whether they start with ./ or not. Explicit and implicit seems to be something one of us (maybe Tim, as author of the issue #4676) came up with.

Maybe we should avoid calling relative paths that don't start with ./ as relative paths at all; maybe calling them workflow source names would be less confusing? The arg could then be SOURCE_NAME | PATH where PATH can be relative or absolute but must start with ./ if relative.

The trouble is that conflicts with standard unix terminology, where a relative path doesn't have to start with ./.

IMO ./blah and ../blah should really be considered absolute paths, where . is short for $PWD which expands to an absolute path. Like ~/blah is an absolute path even though it is relative to $HOME. But, that doesn't seem to be the prevailing opinion out there.

If you invented the term

MB but I think it's valid and helpful here.

The trouble is that conflicts with standard unix terminology
IMO ./blah and ../blah should really be considered absolute paths

I don't think that's correct, technically . and .. are relative paths implemented at the FS level and visible in FS listings. So . is not short for $PWD though it happens to resolve to the same thing.

$PWD/foo - the $PWD is expanded by the shell.

./foo - the ./ is resolved by the FS.

Technically true, but I doubt that matters to users.

BTW I didn't mean that . is literally short for $PWD and, like the shell variable, expanded by the shell ... just that it is short for the present working directory. A true relative path is implicit. Presumably the main reason for the existence of .. is to make changing up a level easier, and ., to force the behaviour of programs that don't interpret (implicit) relative paths properly. So I'd still argue that rel/path is a proper relative path, but ./rel/path is kind of an absolute path masquerading as a relative path.

Explicit relative and absolute paths are handled the same by the CLI so this disparity is not user facing. This is internal technical documentation in a docstring.

(Which still needs to be understood at a glance by future developers).

cylc/flow/workflow_files.py

datamel · 2022-04-14T10:08:31Z

tests/functional/cylc-install/00-simple.t

-purge_rnd_workflow
-
-
-exit


Is this exit not needed? Seems like the bulk of our tests (but not all) have it, I thought it is to ensure if an error occurs then it exits properly.
If all the bash tests are supposed to have it then I suppose we need an issue to add them to the ones that don't have it. I will wait for confirmation before opening it, it is possible some purposefully don't have it.

I'm pretty sure an exit statement with no argument at the end of a script is useless. And annoying. If no one can think of a good reason for them, let's remove them.

datamel

Thanks @MetRonnie. One small query from me.

MetRonnie · 2022-04-14T15:46:12Z

Pinging @cylc/core for any input on the unresolved reviewers' questions above

hjoliver · 2022-04-19T03:57:13Z

Two approvals, and the unresolved questions above are either now resolved or don't require changes to this branch.

oliver-sanders · 2022-04-13T15:31:47Z

cylc/flow/scripts/install.py

@@ -86,48 +101,47 @@
 def get_option_parser():
    parser = COP(
        __doc__, comms=True, prep=True,
-        argdoc=[('[WORKFLOW_NAME]', 'Workflow source name')]
+        argdoc=[('[SOURCE]', 'Path to workflow source')]


Suggested change

argdoc=[('[SOURCE]', 'Path to workflow source')]

argdoc=[('[SOURCE_DIR]', 'Path to workflow source directory')]

oliver-sanders · 2022-04-19T09:30:16Z

cylc/flow/pathutil.py

+    ''',
+    re.VERBOSE
+)
+"""Matches relative paths that are explicit (starts with ./)"""


If you invented the term

MB but I think it's valid and helpful here.

The trouble is that conflicts with standard unix terminology
IMO ./blah and ../blah should really be considered absolute paths

I don't think that's correct, technically . and .. are relative paths implemented at the FS level and visible in FS listings. So . is not short for $PWD though it happens to resolve to the same thing.

$PWD/foo - the $PWD is expanded by the shell.

./foo - the ./ is resolved by the FS.

MetRonnie added 3 commits April 13, 2022 11:17

cylc install: change --flow-name option to --workflow-name

47ec638

Also improve help descriptions of options

cylc install: remove --directory option

a53b496

Merge behaviour into renamed arg WORKFLOW_NAME -> SOURCE

Add unit tests

ece9cde

MetRonnie added this to the cylc-8.0rc3 milestone Apr 13, 2022

MetRonnie self-assigned this Apr 13, 2022

MetRonnie mentioned this pull request Apr 13, 2022

terminology: straighten out usage of "flow" #4786

Closed

4 tasks

MetRonnie requested review from oliver-sanders and datamel April 13, 2022 10:57

MetRonnie force-pushed the install-arg branch from 706f877 to ddf2763 Compare April 13, 2022 11:05

Update changelog [skip ci]

6302de7

MetRonnie force-pushed the install-arg branch from ddf2763 to 6302de7 Compare April 13, 2022 11:06

hjoliver approved these changes Apr 13, 2022

View reviewed changes

MetRonnie mentioned this pull request Apr 14, 2022

Disallow direct validation (etc.) of source FILE #4816

Merged

7 tasks

datamel reviewed Apr 14, 2022

View reviewed changes

datamel approved these changes Apr 14, 2022

View reviewed changes

hjoliver merged commit ef45ec0 into cylc:master Apr 19, 2022

oliver-sanders reviewed Apr 19, 2022

View reviewed changes

MetRonnie deleted the install-arg branch April 19, 2022 11:56

This was referenced Apr 21, 2022

Update cylc install usage text #4832

Merged

Update workflow installation docs cylc/cylc-doc#461

Merged

wxtim mentioned this pull request Apr 26, 2022

Updated tests to use the new Cylc install CLI syntax: cylc/cylc-rose#138

Merged

9 tasks

MetRonnie mentioned this pull request May 4, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

This was referenced May 8, 2022

Add a scheduled check that the tests work metomi/rose#2587

Merged

fix tests for new cylc install CLI metomi/rose#2588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `--directory` option for `cylc install` #4823

Remove `--directory` option for `cylc install` #4823

MetRonnie commented Apr 13, 2022 •

edited

Loading

hjoliver left a comment

hjoliver Apr 13, 2022 •

edited

Loading

MetRonnie Apr 14, 2022

hjoliver Apr 19, 2022

oliver-sanders Apr 19, 2022

hjoliver Apr 19, 2022

oliver-sanders Apr 19, 2022

hjoliver Apr 19, 2022

datamel Apr 14, 2022 •

edited

Loading

hjoliver Apr 19, 2022

datamel left a comment

MetRonnie commented Apr 14, 2022

hjoliver commented Apr 19, 2022

oliver-sanders Apr 13, 2022

oliver-sanders Apr 19, 2022

	argdoc=[('[SOURCE]', 'Path to workflow source')]
	argdoc=[('[SOURCE_DIR]', 'Path to workflow source directory')]

		purge_rnd_workflow


		exit

Remove --directory option for cylc install #4823

Remove --directory option for cylc install #4823

Conversation

MetRonnie commented Apr 13, 2022 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel left a comment

Choose a reason for hiding this comment

MetRonnie commented Apr 14, 2022

hjoliver commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove `--directory` option for `cylc install` #4823

Remove `--directory` option for `cylc install` #4823

MetRonnie commented Apr 13, 2022 •

edited

Loading

hjoliver Apr 13, 2022 •

edited

Loading

datamel Apr 14, 2022 •

edited

Loading