Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove --directory option for cylc install #4823

Merged
merged 4 commits into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ ones in. -->

Third Release Candidate for Cylc 8 suitable for acceptance testing.

### Enhancements

[#4823](https://github.com/cylc/cylc-flow/pull/4823) - Remove the `--directory`
option for `cylc install` (the functionality has been merged into the
workflow source argument), and rename the `--flow-name` option to
`--workflow-name`.

### Fixes

[#4554](https://github.com/cylc/cylc-flow/pull/4554) - Fix incorrect
Expand Down
2 changes: 1 addition & 1 deletion cylc/flow/etc/tutorial/cylc-forecasting-workflow/.validate
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
set -eux
APIKEY="$(head --lines 1 ../api-keys)"
FLOW_NAME="$(< /dev/urandom tr -dc A-Za-z | head -c6)"
cylc install --flow-name "$FLOW_NAME" --no-run-name
cylc install --workflow-name "$FLOW_NAME" --no-run-name
sed -i "s/DATAPOINT_API_KEY/$APIKEY/" "$HOME/cylc-run/$FLOW_NAME/flow.cylc"
cylc validate --check-circular --icp=2000 "$FLOW_NAME"
cylc play --no-detach --abort-if-any-task-fails "$FLOW_NAME"
Expand Down
2 changes: 1 addition & 1 deletion cylc/flow/etc/tutorial/runtime-introduction/.validate
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

set -eux
FLOW_NAME="$(< /dev/urandom tr -dc A-Za-z | head -c6)"
cylc install --flow-name "$FLOW_NAME" --no-run-name
cylc install --workflow-name "$FLOW_NAME" --no-run-name
cylc validate --check-circular --icp=2000 "$FLOW_NAME"
cylc play --no-detach --abort-if-any-task-fails "$FLOW_NAME"
cylc clean "$FLOW_NAME"
9 changes: 5 additions & 4 deletions cylc/flow/install_plugins/log_vc_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,15 +190,17 @@ def get_vc_info(path: Union[Path, str]) -> Optional[Dict[str, Any]]:


def _run_cmd(vcs: str, args: Iterable[str], cwd: Union[Path, str]) -> str:
"""Run a command, return stdout.
"""Run a VCS command, return stdout.

Args:
vcs: The version control system.
args: The args to pass to the version control command.
cwd: Directory to run the command in.

Raises:
OSError: with stderr if non-zero return code.
VCSNotInstalledError: The VCS is not found.
VCSMissingBaseError: There is no base commit in the repo.
OSError: Non-zero return code for VCS command.
"""
cmd = [vcs, *args]
try:
Expand All @@ -218,8 +220,7 @@ def _run_cmd(vcs: str, args: Iterable[str], cwd: Union[Path, str]) -> str:
ret_code = proc.wait()
out, err = proc.communicate()
if ret_code:
if any(err.lower().startswith(msg)
for msg in NO_BASE_ERRS[vcs]):
if any(err.lower().startswith(msg) for msg in NO_BASE_ERRS[vcs]):
# No base commit in repo
raise VCSMissingBaseError(vcs, cwd)
raise OSError(ret_code, err)
Expand Down
10 changes: 10 additions & 0 deletions cylc/flow/pathutil.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,20 @@
)
from cylc.flow.platforms import get_localhost_install_target


# Note: do not import this elsewhere, as it might bypass unit test
# monkeypatching:
_CYLC_RUN_DIR = os.path.join('$HOME', 'cylc-run')

EXPLICIT_RELATIVE_PATH_REGEX = re.compile(
rf'''
^({re.escape(os.curdir)}|{re.escape(os.pardir)})
({re.escape(os.sep)}|$)
''',
re.VERBOSE
)
"""Matches relative paths that are explicit (starts with ./)"""
Copy link
Member

@hjoliver hjoliver Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def expand_path(*args: Union[Path, str]) -> str:
"""Expand both vars and user in path and normalise it, joining any
Expand Down
4 changes: 2 additions & 2 deletions cylc/flow/scripts/cylc.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def get_version(long=False):
Every Installed Cylc workflow has an ID.

For example if we install a workflow like so:
$ cylc install --flow-name=foo
$ cylc install --workflow-name=foo

We will end up with a workflow with the ID "foo/run1".

Expand All @@ -107,7 +107,7 @@ def get_version(long=False):
$ cylc stop foo

Workflows can be installed hierarchically:
$ cylc install --flow-name=foo/bar/baz
$ cylc install --workflow-name=foo/bar/baz

# play the workflow with the ID "foo/bar/baz"
$ cylc play foo/bar/baz
Expand Down
115 changes: 66 additions & 49 deletions cylc/flow/scripts/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,25 @@

The workflow can then be started, stopped, and targeted by name.

Normal installation creates a directory "~/cylc-run/WORKFLOW_NAME/", with a run
directory "~/cylc-run/WORKFLOW_NAME/run1". A "_cylc-install/source" symlink to
the source directory will be created in the WORKFLOW_NAME directory.
Normal installation creates a directory "~/cylc-run/<workflow>/", with a run
directory "~/cylc-run/<workflow>/run1". A "_cylc-install/source" symlink to
the source directory will be created in the workflow directory.
Any files or directories (excluding .git, .svn) from the source directory are
copied to the new run directory.
A ".service" directory will also be created and used for server authentication
files at run time.

If the argument WORKFLOW_NAME is used, Cylc will search for the workflow in the
list of directories given by "global.cylc[install]source dirs", and install the
first match. Otherwise, the workflow in the current working directory, or the
one specified by the "--directory" option, will be installed.
The SOURCE argument accepts three types of path:
* implicit relative path (e.g. "foo/bar") - Cylc will search for the
workflow source in the list of directories given by
"global.cylc[install]source dirs", and install the first match.
* explicit relative path (e.g. "./foo/bar") - Cylc will install the workflow
from the source that is relative to the current working directory.
* absolute path (e.g. "~/foo/bar") - Cylc will install the workflow from the
source given by the path.

If the SOURCE argument is not supplied, Cylc will install the workflow from
the source in the current working directory.

Workflow names can be hierarchical, corresponding to the path under ~/cylc-run.

Expand All @@ -44,36 +51,44 @@
# this will increment)
$ cylc install dogs/fido

# Install $PWD/flow.cylc as "rabbit", if $PWD is ~/bunny/rabbit, with
# Install $PWD as "rabbit", if $PWD is ~/bunny/rabbit, with
# run directory ~/cylc-run/rabbit/run1
$ cylc install

# Install $PWD/flow.cylc as "rabbit", if $PWD is ~/bunny/rabbit, with
# Install $PWD as "rabbit", if $PWD is ~/bunny/rabbit, with
# run directory ~/cylc-run/rabbit (note: no "run1" sub-directory)
$ cylc install --no-run-name

# Install $PWD/flow.cylc as "fido", regardless of what $PWD is, with
# Install $PWD as "fido", regardless of what $PWD is called, with
# run directory ~/cylc-run/fido/run1
$ cylc install --flow-name=fido
$ cylc install --workflow-name=fido

# Install $PWD/bunny/rabbit/flow.cylc as "rabbit", with run directory
# Install $PWD/bunny/rabbit/ as "rabbit", with run directory
# ~/cylc-run/rabbit/run1
$ cylc install --directory=bunny/rabbit
$ cylc install ./bunny/rabbit

# Install /home/somewhere/badger as "badger", with run directory
# ~/cylc-run/badger/run1
$ cylc install /home/somewhere/badger

# Install $PWD/flow.cylc as "cats", if $PWD is ~/cats, overriding the
# run1, run2, run3 etc. structure with run directory ~/cylc-run/cats/paws
# Install $PWD as "cats", if $PWD is ~/cats, with run directory
# ~/cylc-run/cats/paws
$ cylc install --run-name=paws

The same workflow can be installed with multiple names; this results in
multiple workflow run directories that link to the same workflow definition.

"""

from pathlib import Path
from typing import Optional, TYPE_CHECKING, Dict, Any

from cylc.flow import iter_entry_points
from cylc.flow.exceptions import PluginError, UserInputError
from cylc.flow.option_parsers import CylcOptionParser as COP
from cylc.flow.option_parsers import (
CylcOptionParser as COP,
)
from cylc.flow.pathutil import EXPLICIT_RELATIVE_PATH_REGEX, expand_path
from cylc.flow.workflow_files import (
install_workflow, search_install_source_dirs, parse_cli_sym_dirs
)
Expand All @@ -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')]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

parser.add_option(
"--flow-name",
help="Install into ~/cylc-run/<workflow_name>/runN ",
"--workflow-name", "-n",
help="Install into ~/cylc-run/<WORKFLOW_NAME>/runN ",
action="store",
metavar="WORKFLOW_NAME",
default=None,
dest="workflow_name")

parser.add_option(
"--directory", "-C",
help="Install the workflow found in path specfied.",
action="store",
metavar="PATH/TO/FLOW",
default=None,
dest="source")

parser.add_option(
"--symlink-dirs",
help=(
"Enter a list, in the form 'log=path/to/store, share = $...'"
". Use this option to override local symlinks for directories run,"
" log, work, share, share/cycle, as configured in global.cylc. "
"Enter an empty list \"\" to skip making localhost symlink dirs."
"Enter a comma-delimited list, in the form "
"'log=path/to/store, share = $HOME/some/path, ...'. "
"Use this option to override the global.cylc configuration for "
"local symlinks for the run, log, work, share and "
"share/cycle directories. "
"Enter an empty list '' to skip making localhost symlink dirs."
),
action="store",
dest="symlink_dirs"
)

parser.add_option(
"--run-name",
help="Name the run.",
help=(
"Give the run a custom name instead of automatically numbering it."
),
action="store",
metavar="RUN_NAME",
default=None,
dest="run_name")

parser.add_option(
"--no-run-name",
help="Install the workflow directly into ~/cylc-run/<workflow_name>",
help=(
"Install the workflow directly into ~/cylc-run/<workflow_name>, "
"without an automatic run number or custom run name."
),
action="store_true",
default=False,
dest="no_run_name")
Expand All @@ -137,6 +151,22 @@ def get_option_parser():
return parser


def get_source_location(path: Optional[str]) -> Path:
"""Return the workflow source location as an absolute path.

Note: does not check that the source actually exists.
"""
if path is None:
return Path.cwd()
path = path.strip()
expanded_path = Path(expand_path(path))
if expanded_path.is_absolute():
return expanded_path
if EXPLICIT_RELATIVE_PATH_REGEX.match(path):
return Path.cwd() / expanded_path
return search_install_source_dirs(expanded_path)


@cli_function(get_option_parser)
def main(parser, opts, reg=None):
install(parser, opts, reg)
Expand All @@ -150,25 +180,12 @@ def install(
"options --no-run-name and --run-name are mutually exclusive."
)

if reg is None:
source = opts.source
else:
if opts.source:
raise UserInputError(
"WORKFLOW_NAME and --directory are mutually exclusive."
)
source = search_install_source_dirs(reg)
workflow_name = opts.workflow_name or reg

source = get_source_location(reg)
for entry_point in iter_entry_points(
'cylc.pre_configure'
):
try:
if source:
entry_point.resolve()(srcdir=source, opts=opts)
else:
from pathlib import Path
entry_point.resolve()(srcdir=Path().cwd(), opts=opts)
entry_point.resolve()(srcdir=source, opts=opts)
except Exception as exc:
# NOTE: except Exception (purposefully vague)
# this is to separate plugin from core Cylc errors
Expand All @@ -185,8 +202,8 @@ def install(
cli_symdirs = parse_cli_sym_dirs(opts.symlink_dirs)

source_dir, rundir, _workflow_name = install_workflow(
workflow_name=workflow_name,
source=source,
workflow_name=opts.workflow_name,
run_name=opts.run_name,
no_run_name=opts.no_run_name,
cli_symlink_dirs=cli_symdirs
Expand Down
2 changes: 1 addition & 1 deletion cylc/flow/scripts/reinstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,9 @@ def main(
) from None

reinstall_workflow(
source=Path(source),
named_run=workflow_id,
rundir=run_dir,
source=source,
dry_run=False # TODO: ready for dry run implementation
)

Expand Down
Loading