From e4551024ff511b6c5d42cf645895eadb2edd9b1f Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Thu, 13 May 2021 10:11:47 +0100
Subject: [PATCH 1/9] Symlink dirs cli option

---
 cylc/flow/pathutil.py                       | 17 +++---
 cylc/flow/scripts/install.py                | 26 +++++---
 cylc/flow/workflow_files.py                 | 28 +++++++--
 tests/functional/cylc-install/01-symlinks.t | 66 +++++++++++++--------
 tests/unit/test_workflow_files.py           | 37 ++++++++++++
 5 files changed, 123 insertions(+), 51 deletions(-)

diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index 91b06d720ca..1f09d55eb84 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -28,7 +28,6 @@
 )
 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')
@@ -131,7 +130,7 @@ def make_workflow_run_tree(workflow):
 
 
 def make_localhost_symlinks(
-    rund: Union[Path, str], named_sub_dir: str
+    rund: Union[Path, str], named_sub_dir: str, symlink_conf=None
 ) -> Dict[str, Union[Path, str]]:
     """Creates symlinks for any configured symlink dirs from glbl_cfg.
     Args:
@@ -144,7 +143,9 @@ def make_localhost_symlinks(
 
     """
     dirs_to_symlink = get_dirs_to_symlink(
-        get_localhost_install_target(), named_sub_dir)
+        get_localhost_install_target(),
+        named_sub_dir, symlink_conf=symlink_conf
+    )
     symlinks_created = {}
     for key, value in dirs_to_symlink.items():
         if key == 'run':
@@ -165,14 +166,14 @@ def make_localhost_symlinks(
     return symlinks_created
 
 
-def get_dirs_to_symlink(install_target: str, flow_name: str) -> Dict[str, str]:
+def get_dirs_to_symlink(install_target: str, flow_name: str, symlink_conf=None) -> Dict[str, str]:
     """Returns dictionary of directories to symlink from glbcfg.
 
     Note the paths should remain unexpanded, to be expanded on the remote.
     """
     dirs_to_symlink: Dict[str, str] = {}
-    symlink_conf = glbl_cfg().get(['symlink dirs'])
-
+    if not symlink_conf:
+        symlink_conf = glbl_cfg().get(['install', 'symlink dirs'])
     if install_target not in symlink_conf.keys():
         return dirs_to_symlink
     base_dir = symlink_conf[install_target]['run']
@@ -213,10 +214,6 @@ def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool:
             raise WorkflowFilesError(
                 f"Error when symlinking. Failed to unlink bad symlink {path}.")
     target.mkdir(parents=True, exist_ok=True)
-    if path.exists():
-        # Trying to link to itself; no symlink needed
-        # (e.g. path's parent is symlink to target's parent)
-        return False
     path.parent.mkdir(parents=True, exist_ok=True)
     try:
         path.symlink_to(target)
diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index c139292b795..ed97058f6a4 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -75,7 +75,7 @@
 from cylc.flow.exceptions import PluginError
 from cylc.flow.option_parsers import CylcOptionParser as COP
 from cylc.flow.workflow_files import (
-    install_workflow, search_install_source_dirs
+    install_workflow, search_install_source_dirs, get_sym_dirs
 )
 from cylc.flow.terminal import cli_function
 
@@ -150,6 +150,17 @@ def get_option_parser():
         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 creating default local symlinks"
+            ", for directories run, log, work, share, share/cycle, as"
+            " configured in global.cylc."),
+        action="store",
+        default=None,
+        dest="symlink_dirs")
+
     parser.add_option(
         "--run-name",
         help="Name the run.",
@@ -165,13 +176,6 @@ def get_option_parser():
         default=False,
         dest="no_run_name")
 
-    parser.add_option(
-        "--no-symlink-dirs",
-        help="Use this option to override creating default local symlinks.",
-        action="store_true",
-        default=False,
-        dest="no_symlinks")
-
     parser = add_cylc_rose_options(parser)
 
     return parser
@@ -214,13 +218,17 @@ def install(
                 entry_point.name,
                 exc
             ) from None
+    if opts.symlink_dirs:
+        symdirs = get_sym_dirs(opts.symlink_dirs)
+    else:
+        symdirs = None
 
     source_dir, rundir, _flow_name = install_workflow(
         flow_name=flow_name,
         source=source,
         run_name=opts.run_name,
         no_run_name=opts.no_run_name,
-        no_symlinks=opts.no_symlinks
+        symlink_dirs=symdirs
     )
 
     for entry_point in iter_entry_points(
diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index a3078122354..71926a173be 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -161,7 +161,7 @@ def __init__(self, key_type, key_owner, full_key_path=None,
 
 
 class WorkflowFiles:
-    """Files and directories located in the workflow directory."""
+    """Names of files and directories located in the workflow directory."""
 
     FLOW_FILE = 'flow.cylc'
     """The workflow configuration file."""
@@ -184,6 +184,9 @@ class WorkflowFiles:
     WORK_DIR = 'work'
     """Workflow work directory."""
 
+    RUN_DIR = 'run'
+    """Workflow run directory."""
+
     class Service:
         """The directory containing Cylc system files."""
 
@@ -1243,7 +1246,7 @@ def install_workflow(
     source: Optional[Union[Path, str]] = None,
     run_name: Optional[str] = None,
     no_run_name: bool = False,
-    no_symlinks: bool = False
+    symlink_dirs: Optional[list] = None
 ) -> Tuple[Path, Path, str]:
     """Install a workflow, or renew its installation.
 
@@ -1259,7 +1262,6 @@ def install_workflow(
         rundir: for overriding the default cylc-run directory.
         no_run_name: Flag as True to install workflow into
             ~/cylc-run/<flow_name>
-        no_symlinks: Flag as True to skip making localhost symlink dirs
 
     Return:
         source: The source directory.
@@ -1299,10 +1301,10 @@ def install_workflow(
         named_run = os.path.join(named_run, run_name)
     elif run_num:
         named_run = os.path.join(named_run, f'run{run_num}')
-    if not no_symlinks:
-        symlinks_created = make_localhost_symlinks(rundir, named_run)
+    symlinks_created = make_localhost_symlinks(
+        rundir, named_run, symlink_conf=symlink_dirs)
     install_log = _get_logger(rundir, 'cylc-install')
-    if not no_symlinks and bool(symlinks_created) is True:
+    if bool(symlinks_created) is True:
         for src, dst in symlinks_created.items():
             install_log.info(f"Symlink created from {src} to {dst}")
     try:
@@ -1491,6 +1493,20 @@ def validate_source_dir(source, flow_name):
     check_flow_file(source, logger=None)
 
 
+def get_sym_dirs(symlink_dirs):
+    symdict = {'localhost': {}}
+    symlist = symlink_dirs.replace(" ", "").strip(',').split(',')
+    for pair in symlist:
+        key, val = pair.split("=")
+        if key in ['run', 'log', 'share', 'share/cycle', 'work']:
+            symdict['localhost'][key] = val
+        else:
+            raise WorkflowFilesError(
+                f"{key} not a valid entry for --symlink-dirs"
+            )
+    return symdict
+
+
 def unlink_runN(path: Union[Path, str]) -> bool:
     """Remove symlink runN if it exists.
 
diff --git a/tests/functional/cylc-install/01-symlinks.t b/tests/functional/cylc-install/01-symlinks.t
index 9a6a5cb9328..9109d3b86ff 100644
--- a/tests/functional/cylc-install/01-symlinks.t
+++ b/tests/functional/cylc-install/01-symlinks.t
@@ -24,11 +24,12 @@ if [[ -z ${TMPDIR:-} || -z ${USER:-} || $TMPDIR/$USER == "$HOME" ]]; then
     skip_all '"TMPDIR" or "USER" not defined or "TMPDIR"/"USER" is "HOME"'
 fi
 
-set_test_number 14
+set_test_number 17
 
 create_test_global_config "" "
-[symlink dirs]
-    [[localhost]]
+[install]
+[[symlink dirs]]
+    [[[localhost]]]
         run = \$TMPDIR/\$USER/test_cylc_symlink/cylctb_tmp_run_dir
         share = \$TMPDIR/\$USER/test_cylc_symlink/
         log = \$TMPDIR/\$USER/test_cylc_symlink/
@@ -53,16 +54,39 @@ else
     fail "$TEST_SYM"
 fi
 
+TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
+if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
+"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
+    ok "$TEST_SYM"
+else
+    fail "$TEST_SYM"
+# # Test "cylc install" ensure symlinks are created
+TEST_NAME="${TEST_NAME_BASE}-symlinks-created"
+make_rnd_workflow
 
+if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
+    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
+        ok "$TEST_SYM"
+else
+    fail "$TEST_SYM"
+fi
 
 TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
 if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
 "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
-    ok "$TEST_SYM"
+fi
+if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
+    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
+        ok "$TEST_SYM"
 else
     fail "$TEST_SYM"
 fi
 
+TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
+if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
+"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
+fi
+
 for DIR in 'work' 'share' 'log'; do
     TEST_SYM="${TEST_NAME_BASE}-${DIR}-symlink-exists-ok"
     if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}") == \
@@ -75,29 +99,19 @@ done
 rm -rf "${TMPDIR}/${USER}/test_cylc_symlink/"
 purge_rnd_workflow
 
+# test cli --symlink-dirs overrides the glblcfg
+VAR=$TMPDIR/$USER/test_cylc_cli_symlink/
 
-
-# Test "cylc install" --no-symlink-dirs
-TEST_NAME="${TEST_NAME_BASE}-no-symlinks-created"
+TEST_NAME="${TEST_NAME_BASE}-symlinks-cli-opt"
 make_rnd_workflow
-run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" --no-symlink-dirs --directory="${RND_WORKFLOW_SOURCE}"
+run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" \
+--directory="${RND_WORKFLOW_SOURCE}" \
+--symlink-dirs="run= ${VAR}run, log=${VAR}, share=${VAR}, work = ${VAR}, share/cycle=${VAR}cylctb_tmp_share_dir"
 contains_ok "${TEST_NAME}.stdout" <<__OUT__
 INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
 __OUT__
 
-
-TEST_SYM="${TEST_NAME_BASE}-run-symlink-exists-ok"
-
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
-    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
-        fail "$TEST_SYM"
-else
-    ok "$TEST_SYM"
-fi
-
-
-
-TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-not-exists-ok"
+TEST_SYM="${TEST_NAME_BASE}-share-cycle-symlink-cli-ok"
 if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
 "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/share/cycle" ]]; then
     fail "$TEST_SYM"
@@ -106,13 +120,13 @@ else
 fi
 
 for DIR in 'work' 'share' 'log'; do
-    TEST_SYM="${TEST_NAME_BASE}-${DIR}-symlink-not-exists-ok"
+    TEST_SYM="${TEST_NAME_BASE}-${DIR}-symlink-cli-ok"
     if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}") == \
-   "$TMPDIR/${USER}/test_cylc_symlink/cylc-run/${RND_WORKFLOW_NAME}/${DIR}" ]]; then
-        fail "$TEST_SYM"
-    else
+   "$TMPDIR/${USER}/test_cylc_cli_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}" ]]; then
         ok "$TEST_SYM"
+    else
+        fail "$TEST_SYM"
     fi
 done
-rm -rf "${TMPDIR}/${USER}/test_cylc_symlink/"
+rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
 purge_rnd_workflow
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index b2817be2373..1f36aafad84 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -31,6 +31,7 @@
     TaskRemoteMgmtError,
     WorkflowFilesError
 )
+from cylc.flow.option_parsers import Options
 from cylc.flow.pathutil import parse_rm_dirs
 from cylc.flow.scripts.clean import CleanOptions
 from cylc.flow.workflow_files import (
@@ -39,6 +40,7 @@
     _remote_clean_cmd,
     check_flow_file,
     check_nested_run_dirs,
+    get_sym_dirs,
     get_symlink_dirs,
     get_workflow_source_dir,
     glob_in_run_dir,
@@ -1441,3 +1443,38 @@ def test_check_flow_file_symlink(
                 log_msg = f'{log_msg}. Symlink created.'
         assert result == tmp_path.joinpath(expected_file)
         assert caplog.messages == [log_msg]
+
+
+@pytest.mark.parametrize(
+    'symlink_dirs, err_msg, expected',
+    [
+        ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ', 'None',
+            {'localhost': {
+                'log': "$shortbread",
+                'share': '$bourbon',
+                'share/cycle': '$digestive'
+            }}
+         ),
+        ('run=$NICE, log= $Garibaldi, share/cycle=$RichTea', 'None',
+            {'localhost': {
+                'run': '$NICE',
+                'log': '$Garibaldi',
+                'share/cycle': '$RichTea'
+            }}
+         ),
+        ('some_other_dir=$bourbon',
+         'some_other_dir not a valid entry for --symlink-dirs',
+            {'some_other_dir': '£bourbon'}
+         ),
+    ]
+)
+def test_get_sym_dirs(symlink_dirs, err_msg, expected):
+    """Test get_sym_dirs raises errors on cli symlink options"""
+    if err_msg != 'None':
+        with pytest.raises(WorkflowFilesError):
+            get_sym_dirs(symlink_dirs)
+
+    else:
+        actual = get_sym_dirs(symlink_dirs)
+
+        assert actual == expected

From 1bdb383e4f75775e3c9d9d9f90b9f378fbf8ee2c Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Tue, 18 May 2021 23:35:50 +0100
Subject: [PATCH 2/9] move symlink dirs to install glbl config section

---
 cylc/flow/cfgspec/globalcfg.py                | 109 +++++++++---------
 cylc/flow/cfgspec/workflow.py                 |   2 +-
 tests/functional/cylc-clean/00-basic.t        |  15 +--
 tests/functional/cylc-clean/01-remote.t       |  15 +--
 .../cylc-get-site-config/04-homeless.t        |   9 +-
 tests/functional/remote/04-symlink-dirs.t     |  27 ++---
 tests/unit/test_pathutil.py                   |  20 ++--
 7 files changed, 103 insertions(+), 94 deletions(-)

diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py
index 22c45fc4641..b73c9183b6e 100644
--- a/cylc/flow/cfgspec/globalcfg.py
+++ b/cylc/flow/cfgspec/globalcfg.py
@@ -369,6 +369,61 @@
                If workflow source directories of the same name exist in more
                than one of these paths, only the first one will be picked up.
         ''')
+        # Symlink Dirs
+        with Conf('symlink dirs',  # noqa: SIM117 (keep same format)
+                  desc="""
+            Define directories to be moved, symlinks from the the original
+            ``$HOME/cylc-run`` directories will be created.
+        """):
+            with Conf('<install target>'):
+                Conf('run', VDR.V_STRING, None, desc="""
+                    Specifies the directory where the workflow run directories
+                    are created. If specified, the workflow run directory will
+                    be created in ``<run dir>/cylc-run/<workflow-name>`` and a
+                    symbolic link will be created from
+                    ``$HOME/cylc-run/<workflow-name>``.
+                    If not specified the workflow run directory will be created
+                    in ``$HOME/cylc-run/<workflow-name>``.
+                    All the workflow files and the ``.service`` directory get
+                    installed into this directory.
+                """)
+                Conf('log', VDR.V_STRING, None, desc="""
+                    Specifies the directory where log directories are created.
+                    If specified the workflow log directory will be created in
+                    ``<log dir>/cylc-run/<workflow-name>/log`` and a symbolic
+                    link will be created from
+                    ``$HOME/cylc-run/<workflow-name>/log``. If not specified
+                    the workflow log directory will be created in
+                    ``$HOME/cylc-run/<workflow-name>/log``.
+                """)
+                Conf('share', VDR.V_STRING, None, desc="""
+                    Specifies the directory where share directories are
+                    created. If specified the workflow share directory will be
+                    created in ``<share dir>/cylc-run/<workflow-name>/share``
+                    and a symbolic link will be created from
+                    ``<$HOME/cylc-run/<workflow-name>/share``. If not specified
+                    the workflow share directory will be created in
+                    ``$HOME/cylc-run/<workflow-name>/share``.
+                """)
+                Conf('share/cycle', VDR.V_STRING, None, desc="""
+                    Specifies the directory where share/cycle directories are
+                    created. If specified the workflow share/cycle directory
+                    will be created in
+                    ``<share/cycle dir>/cylc-run/<workflow-name>/share/cycle``
+                    and a symbolic link will be created from
+                    ``$HOME/cylc-run/<workflow-name>/share/cycle``. If not
+                    specified the workflow share/cycle directory will be
+                    created in ``$HOME/cylc-run/<workflow-name>/share/cycle``.
+                """)
+                Conf('work', VDR.V_STRING, None, desc="""
+                    Specifies the directory where work directories are created.
+                    If specified the workflow work directory will be created in
+                    ``<work dir>/cylc-run/<workflow-name>/work`` and a symbolic
+                    link will be created from
+                    ``$HOME/cylc-run/<workflow-name>/work``. If not specified
+                    the workflow work directory will be created in
+                    ``$HOME/cylc-run/<workflow-name>/work``.
+                """)
 
     with Conf('editors', desc='''
         Choose your favourite text editor for editing workflow configurations.
@@ -711,60 +766,6 @@
     with Conf('platform groups'):  # noqa: SIM117 (keep same format)
         with Conf('<group>'):
             Conf('platforms', VDR.V_STRING_LIST)
-    # Symlink Dirs
-    with Conf('symlink dirs',   # noqa: SIM117 (keep same format)
-              desc="""
-        Define directories to be moved, symlinks from the the original
-        ``$HOME/cylc-run`` directories will be created.
-    """):
-        with Conf('<install target>'):
-            Conf('run', VDR.V_STRING, None, desc="""
-                Specifies the directory where the workflow run directories are
-                created. If specified, the workflow run directory will be
-                created in ``<run dir>/cylc-run/<workflow-name>`` and a
-                symbolic link will be created from
-                ``$HOME/cylc-run/<workflow-name>``.
-                If not specified the workflow run directory will be created in
-                ``$HOME/cylc-run/<workflow-name>``.
-                All the workflow files and the ``.service`` directory get
-                installed into this directory.
-            """)
-            Conf('log', VDR.V_STRING, None, desc="""
-                Specifies the directory where log directories are created. If
-                specified the workflow log directory will be created in
-                ``<log dir>/cylc-run/<workflow-name>/log`` and a symbolic link
-                will be created from ``$HOME/cylc-run/<workflow-name>/log``. If
-                not specified the workflow log directory will be created in
-                ``$HOME/cylc-run/<workflow-name>/log``.
-            """)
-            Conf('share', VDR.V_STRING, None, desc="""
-                Specifies the directory where share directories are created.
-                If specified the workflow share directory will be created in
-                ``<share dir>/cylc-run/<workflow-name>/share`` and a symbolic
-                link will be created from
-                ``<$HOME/cylc-run/<workflow-name>/share``. If not specified the
-                workflow share directory will be created in
-                ``$HOME/cylc-run/<workflow-name>/share``.
-            """)
-            Conf('share/cycle', VDR.V_STRING, None, desc="""
-                Specifies the directory where share/cycle directories are
-                created. If specified the workflow share/cycle directory will
-                be created in
-                ``<share/cycle dir>/cylc-run/<workflow-name>/share/cycle`` and
-                a symbolic link will be created from
-                ``$HOME/cylc-run/<workflow-name>/share/cycle``. If not
-                specified the workflow share/cycle directory will be created in
-                ``$HOME/cylc-run/<workflow-name>/share/cycle``.
-            """)
-            Conf('work', VDR.V_STRING, None, desc="""
-                Specifies the directory where work directories are created.
-                If specified the workflow work directory will be created in
-                ``<work dir>/cylc-run/<workflow-name>/work`` and a symbolic
-                link will be created from
-                ``$HOME/cylc-run/<workflow-name>/work``. If not specified the
-                workflow work directory will be created in
-                ``$HOME/cylc-run/<workflow-name>/work``.
-            """)
     # task
     with Conf('task events', desc='''
         Global site/user defaults for
diff --git a/cylc/flow/cfgspec/workflow.py b/cylc/flow/cfgspec/workflow.py
index cd8827943d9..4191ccfe6e9 100644
--- a/cylc/flow/cfgspec/workflow.py
+++ b/cylc/flow/cfgspec/workflow.py
@@ -788,7 +788,7 @@
 
                 The top level share and work directory location can be changed
                 (e.g. to a large data area) by a global config setting (see
-                :cylc:conf:`global.cylc[symlink dirs]`).
+                :cylc:conf:`global.cylc[install][symlink dirs]`).
 
                 .. note::
 
diff --git a/tests/functional/cylc-clean/00-basic.t b/tests/functional/cylc-clean/00-basic.t
index 7615a9d6804..6916c9e4f91 100644
--- a/tests/functional/cylc-clean/00-basic.t
+++ b/tests/functional/cylc-clean/00-basic.t
@@ -28,13 +28,14 @@ SYM_NAME="$(mktemp -u)"
 SYM_NAME="${SYM_NAME##*tmp.}"
 
 create_test_global_config "" "
-[symlink dirs]
-    [[localhost]]
-        run = ${TEST_DIR}/${SYM_NAME}/run
-        log = ${TEST_DIR}/${SYM_NAME}/log
-        share = ${TEST_DIR}/${SYM_NAME}/share
-        share/cycle = ${TEST_DIR}/${SYM_NAME}/cycle
-        work = ${TEST_DIR}/${SYM_NAME}/work
+[install]
+    [[symlink dirs]]
+        [[[localhost]]]
+            run = ${TEST_DIR}/${SYM_NAME}/run
+            log = ${TEST_DIR}/${SYM_NAME}/log
+            share = ${TEST_DIR}/${SYM_NAME}/share
+            share/cycle = ${TEST_DIR}/${SYM_NAME}/cycle
+            work = ${TEST_DIR}/${SYM_NAME}/work
 "
 init_workflow "${TEST_NAME_BASE}" << '__FLOW__'
 [scheduler]
diff --git a/tests/functional/cylc-clean/01-remote.t b/tests/functional/cylc-clean/01-remote.t
index 4ac43a4d126..64a01caffcb 100644
--- a/tests/functional/cylc-clean/01-remote.t
+++ b/tests/functional/cylc-clean/01-remote.t
@@ -32,13 +32,14 @@ SYM_NAME="$(mktemp -u)"
 SYM_NAME="${SYM_NAME##*tmp.}"
 
 create_test_global_config "" "
-[symlink dirs]
-    [[${CYLC_TEST_INSTALL_TARGET}]]
-        run = ${TEST_DIR}/${SYM_NAME}/run
-        log = ${TEST_DIR}/${SYM_NAME}/other
-        share = ${TEST_DIR}/${SYM_NAME}/other
-        share/cycle = ${TEST_DIR}/${SYM_NAME}/cycle
-        work = ${TEST_DIR}/${SYM_NAME}/other
+[install]
+    [[symlink dirs]]
+        [[[${CYLC_TEST_INSTALL_TARGET}]]]
+            run = ${TEST_DIR}/${SYM_NAME}/run
+            log = ${TEST_DIR}/${SYM_NAME}/other
+            share = ${TEST_DIR}/${SYM_NAME}/other
+            share/cycle = ${TEST_DIR}/${SYM_NAME}/cycle
+            work = ${TEST_DIR}/${SYM_NAME}/other
 "
 init_workflow "${TEST_NAME_BASE}" << __FLOW__
 [scheduler]
diff --git a/tests/functional/cylc-get-site-config/04-homeless.t b/tests/functional/cylc-get-site-config/04-homeless.t
index 2dc2aacde63..0afa2abe062 100644
--- a/tests/functional/cylc-get-site-config/04-homeless.t
+++ b/tests/functional/cylc-get-site-config/04-homeless.t
@@ -22,14 +22,15 @@ set_test_number 3
 
 # shellcheck disable=SC2016
 create_test_global_config '' '
-[symlink dirs]
-    [[localhost]]
-        run = $HOME/dr-malcolm
+[install]
+    [[symlink dirs]]
+        [[[localhost]]]
+            run = $HOME/dr-malcolm
 '
 
 run_ok "${TEST_NAME_BASE}" \
     env -u HOME \
-    cylc config --item='[symlink dirs][localhost]run'
+    cylc config --item='[install][symlink dirs][localhost]run'
 cmp_ok "${TEST_NAME_BASE}.stdout" <<<"\$HOME/dr-malcolm"
 cmp_ok "${TEST_NAME_BASE}.stderr" <'/dev/null'
 exit
diff --git a/tests/functional/remote/04-symlink-dirs.t b/tests/functional/remote/04-symlink-dirs.t
index b18ede331f9..9005f5419c5 100644
--- a/tests/functional/remote/04-symlink-dirs.t
+++ b/tests/functional/remote/04-symlink-dirs.t
@@ -27,19 +27,20 @@ fi
 set_test_number 12
 
 create_test_global_config "" "
-[symlink dirs]
-    [[localhost]]
-        run = \$TMPDIR/\$USER/cylctb_tmp_run_dir
-        share = \$TMPDIR/\$USER
-        log = \$TMPDIR/\$USER
-        share/cycle = \$TMPDIR/\$USER/cylctb_tmp_share_dir
-        work = \$TMPDIR/\$USER
-    [[$CYLC_TEST_INSTALL_TARGET]]
-        run = \$TMPDIR/\$USER/test_cylc_symlink/ctb_tmp_run_dir
-        share = \$TMPDIR/\$USER/test_cylc_symlink/
-        log = \$TMPDIR/\$USER/test_cylc_symlink/
-        share/cycle = \$TMPDIR/\$USER/test_cylc_symlink/ctb_tmp_share_dir
-        work = \$TMPDIR/\$USER/test_cylc_symlink/
+[install]
+    [[symlink dirs]]
+        [[[localhost]]]
+            run = \$TMPDIR/\$USER/cylctb_tmp_run_dir
+            share = \$TMPDIR/\$USER
+            log = \$TMPDIR/\$USER
+            share/cycle = \$TMPDIR/\$USER/cylctb_tmp_share_dir
+            work = \$TMPDIR/\$USER
+        [[[$CYLC_TEST_INSTALL_TARGET]]]
+            run = \$TMPDIR/\$USER/test_cylc_symlink/ctb_tmp_run_dir
+            share = \$TMPDIR/\$USER/test_cylc_symlink/
+            log = \$TMPDIR/\$USER/test_cylc_symlink/
+            share/cycle = \$TMPDIR/\$USER/test_cylc_symlink/ctb_tmp_share_dir
+            work = \$TMPDIR/\$USER/test_cylc_symlink/
 "
 
 install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}"
diff --git a/tests/unit/test_pathutil.py b/tests/unit/test_pathutil.py
index 3af92a833db..ea97ec9402c 100644
--- a/tests/unit/test_pathutil.py
+++ b/tests/unit/test_pathutil.py
@@ -174,8 +174,9 @@ def test_make_workflow_run_tree(
     [
         pytest.param(  # basic
             '''
-            [symlink dirs]
-                [[the_matrix]]
+            [install]
+            [[symlink dirs]]
+                [[[the_matrix]]]
                     run = $DEE
                     work = $DAH
                     log = $DUH
@@ -193,8 +194,9 @@ def test_make_workflow_run_tree(
         ),
         pytest.param(  # remove nested run symlinks
             '''
-            [symlink dirs]
-                [[the_matrix]]
+            [install]
+            [[symlink dirs]]
+                [[[the_matrix]]]
                     run = $DEE
                     work = $DAH
                     log = $DEE
@@ -211,8 +213,9 @@ def test_make_workflow_run_tree(
         ),
         pytest.param(  # remove only nested run symlinks
             '''
-            [symlink dirs]
-                [[the_matrix]]
+            [install]
+            [[symlink dirs]]
+                [[[the_matrix]]]
                     run = $DOH
                     log = $DEE
                     share = $DEE
@@ -226,8 +229,9 @@ def test_make_workflow_run_tree(
         ),
         pytest.param(  # blank entries
             '''
-            [symlink dirs]
-                [[the_matrix]]
+            [install]
+            [[symlink dirs]]
+                [[[the_matrix]]]
                     run =
                     log = ""
                     share =

From 556fd2fcaeed9101d60b2f768ee193456b55f914 Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Wed, 19 May 2021 14:22:38 +0100
Subject: [PATCH 3/9] Increase Symlink Dirs Test Coverage

---
 cylc/flow/pathutil.py                       |  29 +++-
 cylc/flow/scripts/install.py                |   9 +-
 cylc/flow/workflow_files.py                 |  33 ++++-
 tests/functional/cylc-install/01-symlinks.t | 143 +++++++++++---------
 tests/unit/test_workflow_files.py           |  13 +-
 5 files changed, 143 insertions(+), 84 deletions(-)

diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index 1f09d55eb84..77522419b6e 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -19,7 +19,7 @@
 from pathlib import Path
 import re
 from shutil import rmtree
-from typing import Dict, Iterable, Set, Union
+from typing import Dict, Iterable, Set, Union, Optional, OrderedDict, Any
 
 from cylc.flow import LOG
 from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
@@ -130,7 +130,9 @@ def make_workflow_run_tree(workflow):
 
 
 def make_localhost_symlinks(
-    rund: Union[Path, str], named_sub_dir: str, symlink_conf=None
+    rund: Union[Path, str],
+    named_sub_dir: str, 
+    symlink_conf: Optional[Dict[str, Dict[str, str]]] = None
 ) -> Dict[str, Union[Path, str]]:
     """Creates symlinks for any configured symlink dirs from glbl_cfg.
     Args:
@@ -166,12 +168,25 @@ def make_localhost_symlinks(
     return symlinks_created
 
 
-def get_dirs_to_symlink(install_target: str, flow_name: str, symlink_conf=None) -> Dict[str, str]:
-    """Returns dictionary of directories to symlink from glbcfg.
+def get_dirs_to_symlink(
+    install_target: str,
+    flow_name: str,
+    symlink_conf: Optional[Dict[str, Dict[str, Any]]] = None
+) -> Dict[str, Any]:
+    """Returns dictionary of directories to symlink.
 
     Note the paths should remain unexpanded, to be expanded on the remote.
+    Args:
+        install_target: Symlinks to be created on this install target
+        flow_name: full name of the run, e.g. myflow/run1
+        symlink_conf: Symlink dirs, if sent on the cli.
+            Defaults to None, in which case global config symlink dirs will
+            be applied.
+
+    Returns:
+        dirs_to_symlink: [directory: symlink_path]
     """
-    dirs_to_symlink: Dict[str, str] = {}
+    dirs_to_symlink: Dict[str, Any] = {}
     if not symlink_conf:
         symlink_conf = glbl_cfg().get(['install', 'symlink dirs'])
     if install_target not in symlink_conf.keys():
@@ -180,8 +195,8 @@ def get_dirs_to_symlink(install_target: str, flow_name: str, symlink_conf=None)
     if base_dir:
         dirs_to_symlink['run'] = os.path.join(base_dir, 'cylc-run', flow_name)
     for dir_ in ['log', 'share', 'share/cycle', 'work']:
-        link = symlink_conf[install_target][dir_]
-        if (not link) or link == base_dir:
+        link = symlink_conf[install_target].get(dir_, None)
+        if (not link) is None or link == base_dir:
             continue
         dirs_to_symlink[dir_] = os.path.join(link, 'cylc-run', flow_name, dir_)
     return dirs_to_symlink
diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index ed97058f6a4..ad033e1df92 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -69,7 +69,7 @@
 
 """
 
-from typing import Optional, TYPE_CHECKING
+from typing import Optional, TYPE_CHECKING, Dict, Any
 
 from cylc.flow import iter_entry_points
 from cylc.flow.exceptions import PluginError
@@ -218,17 +218,16 @@ def install(
                 entry_point.name,
                 exc
             ) from None
+    cli_symdirs: Optional[Dict[str, Dict[str, Any]]] = None
     if opts.symlink_dirs:
-        symdirs = get_sym_dirs(opts.symlink_dirs)
-    else:
-        symdirs = None
+        cli_symdirs = get_sym_dirs(opts.symlink_dirs)
 
     source_dir, rundir, _flow_name = install_workflow(
         flow_name=flow_name,
         source=source,
         run_name=opts.run_name,
         no_run_name=opts.no_run_name,
-        symlink_dirs=symdirs
+        cli_symlink_dirs=cli_symdirs
     )
 
     for entry_point in iter_entry_points(
diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index 71926a173be..a9f1d4d45f2 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -1246,7 +1246,7 @@ def install_workflow(
     source: Optional[Union[Path, str]] = None,
     run_name: Optional[str] = None,
     no_run_name: bool = False,
-    symlink_dirs: Optional[list] = None
+    cli_symlink_dirs: Optional[Union[Dict[str, Dict[str, Any]], None]] = None
 ) -> Tuple[Path, Path, str]:
     """Install a workflow, or renew its installation.
 
@@ -1302,7 +1302,7 @@ def install_workflow(
     elif run_num:
         named_run = os.path.join(named_run, f'run{run_num}')
     symlinks_created = make_localhost_symlinks(
-        rundir, named_run, symlink_conf=symlink_dirs)
+        rundir, named_run, symlink_conf=cli_symlink_dirs)
     install_log = _get_logger(rundir, 'cylc-install')
     if bool(symlinks_created) is True:
         for src, dst in symlinks_created.items():
@@ -1493,11 +1493,34 @@ def validate_source_dir(source, flow_name):
     check_flow_file(source, logger=None)
 
 
-def get_sym_dirs(symlink_dirs):
-    symdict = {'localhost': {}}
+def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
+    """Converts command line entered symlink dirs to a dictionary.
+
+    Args:
+        symlink_dirs (str): As entered by user on cli,
+                            e.g. [log=$DIR, share=$DIR2].
+
+    Raises:
+        WorkflowFilesError: If directory to be symlinked is not in permitted
+                            dirs: run, log, share, work, share/cycle
+
+    Returns:
+        dict: In the same form as would be returned by global config.
+            e.g. {'localhost': {'log': '$DIR',
+                                'share': '$DIR2'
+                                }
+                }
+    """
+    # Ensures the same nested dict format which is returned by the glb cfg
+    symdict: Dict[str, Dict[str, Any]] = {'localhost': {'run': None}}
+    if symlink_dirs == 'None':
+        return symdict
     symlist = symlink_dirs.replace(" ", "").strip(',').split(',')
     for pair in symlist:
-        key, val = pair.split("=")
+        try:
+            key, val = pair.split("=")
+        except ValueError:
+            return symdict
         if key in ['run', 'log', 'share', 'share/cycle', 'work']:
             symdict['localhost'][key] = val
         else:
diff --git a/tests/functional/cylc-install/01-symlinks.t b/tests/functional/cylc-install/01-symlinks.t
index 9109d3b86ff..b8c6dfac3a5 100644
--- a/tests/functional/cylc-install/01-symlinks.t
+++ b/tests/functional/cylc-install/01-symlinks.t
@@ -24,7 +24,7 @@ if [[ -z ${TMPDIR:-} || -z ${USER:-} || $TMPDIR/$USER == "$HOME" ]]; then
     skip_all '"TMPDIR" or "USER" not defined or "TMPDIR"/"USER" is "HOME"'
 fi
 
-set_test_number 17
+set_test_number 25
 
 create_test_global_config "" "
 [install]
@@ -44,89 +44,104 @@ run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" --director
 contains_ok "${TEST_NAME}.stdout" <<__OUT__
 INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
 __OUT__
+WORKFLOW_RUN_DIR="$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1"
+TEST_SYM="${TEST_NAME_BASE}-run-glblcfg"
+run_ok "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}")" \
+    = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1"
 
-TEST_SYM="${TEST_NAME_BASE}-run-symlink-exists-ok"
+TEST_SYM="${TEST_NAME_BASE}-share-cycle-glblcfg"
+run_ok "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}/share/cycle")" \
+    = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle"
 
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
-    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
-        ok "$TEST_SYM"
-else
-    fail "$TEST_SYM"
-fi
+for DIR in 'work' 'share' 'log'; do
+    TEST_SYM="${TEST_NAME_BASE}-${DIR}-glbcfg"
+    run_ok "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}/${DIR}")" \
+   = "$TMPDIR/${USER}/test_cylc_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}"
+done
+rm -rf "${TMPDIR}/${USER}/test_cylc_symlink/"
+purge_rnd_workflow
 
-TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
-"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
-    ok "$TEST_SYM"
-else
-    fail "$TEST_SYM"
-# # Test "cylc install" ensure symlinks are created
-TEST_NAME="${TEST_NAME_BASE}-symlinks-created"
+# test cli --symlink-dirs overrides the glblcfg
+SYMDIR=${TMPDIR}/${USER}/test_cylc_cli_symlink/
+
+TEST_NAME="${TEST_NAME_BASE}-cli-opt-install"
 make_rnd_workflow
+run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" \
+--directory="${RND_WORKFLOW_SOURCE}" \
+--symlink-dirs="run= ${SYMDIR}cylctb_tmp_run_dir, log=${SYMDIR}, share=${SYMDIR}, \
+work = ${SYMDIR}"
+contains_ok "${TEST_NAME}.stdout" <<__OUT__
+INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
+__OUT__
+WORKFLOW_RUN_DIR="$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1"
 
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
-    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
-        ok "$TEST_SYM"
-else
-    fail "$TEST_SYM"
-fi
+TEST_SYM="${TEST_NAME_BASE}-run-cli"
+run_ok "$TEST_SYM" test "$(readlink "${WORKFLOW_RUN_DIR}")" \
+   =  "$TMPDIR/${USER}/test_cylc_cli_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1"
 
-TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
-"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
-fi
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1") == \
-    "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1" ]]; then
-        ok "$TEST_SYM"
-else
-    fail "$TEST_SYM"
-fi
 
-TEST_SYM="${TEST_NAME_BASE}-share/cycle-symlink-exists-ok"
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
-"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle" ]]; then
-fi
+for DIR in 'work' 'share' 'log'; do
+    TEST_SYM="${TEST_NAME_BASE}-${DIR}-cli"
+    run_ok "$TEST_SYM" test "$(readlink "${WORKFLOW_RUN_DIR}/${DIR}")" \
+   = "${TMPDIR}/${USER}/test_cylc_cli_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}"
+done
 
+INSTALL_LOG="$(find "${WORKFLOW_RUN_DIR}/log/install" -type f -name '*.log')"
+ 
 for DIR in 'work' 'share' 'log'; do
-    TEST_SYM="${TEST_NAME_BASE}-${DIR}-symlink-exists-ok"
-    if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}") == \
-   "$TMPDIR/${USER}/test_cylc_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}" ]]; then
-        ok "$TEST_SYM"
-    else
-        fail "$TEST_SYM"
-    fi
+    grep_ok "${TMPDIR}/${USER}/test_cylc_cli_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}" "${INSTALL_LOG}"
 done
-rm -rf "${TMPDIR}/${USER}/test_cylc_symlink/"
+
+# test cylc play symlinks after cli opts (mapping to different directories)
+
+pushd "${WORKFLOW_RUN_DIR}" || exit 1
+cat > 'flow.cylc' << __FLOW__
+[scheduling]
+    [[graph]]
+        R1 = foo
+[runtime]
+    [[foo]]
+        script = true
+__FLOW__
+
+popd || exit 1
+
+run_ok "${TEST_NAME_BASE}-play" cylc play "${RND_WORKFLOW_NAME}/runN" --debug
+
+# test ensure symlinks, not in cli install are not created from glbl cfg.
+TEST_SYM="${TEST_NAME_BASE}-share-cycle-cli"
+run_fail "$TEST_SYM" test "$(readlink "${WORKFLOW_RUN_DIR}/share/cycle")" \
+= "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle"
+
+rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
 purge_rnd_workflow
 
-# test cli --symlink-dirs overrides the glblcfg
-VAR=$TMPDIR/$USER/test_cylc_cli_symlink/
 
-TEST_NAME="${TEST_NAME_BASE}-symlinks-cli-opt"
+# test no symlinks created with --symlink-dirs=None
+
+
+TEST_NAME="${TEST_NAME_BASE}-no-sym-dirs-cli"
 make_rnd_workflow
 run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" \
---directory="${RND_WORKFLOW_SOURCE}" \
---symlink-dirs="run= ${VAR}run, log=${VAR}, share=${VAR}, work = ${VAR}, share/cycle=${VAR}cylctb_tmp_share_dir"
+--directory="${RND_WORKFLOW_SOURCE}" --symlink-dirs=None
 contains_ok "${TEST_NAME}.stdout" <<__OUT__
 INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
 __OUT__
+WORKFLOW_RUN_DIR="$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1"
 
-TEST_SYM="${TEST_NAME_BASE}-share-cycle-symlink-cli-ok"
-if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle") == \
-"$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/share/cycle" ]]; then
-    fail "$TEST_SYM"
-else
-    ok "$TEST_SYM"
-fi
+
+TEST_SYM="${TEST_NAME}-run"
+run_fail "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}")" \
+    = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1"
+
+TEST_SYM="${TEST_NAME}-share-cycle"
+run_fail "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}/share/cycle")" \
+    = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_share_dir/cylc-run/${RND_WORKFLOW_NAME}/run1/share/cycle"
 
 for DIR in 'work' 'share' 'log'; do
-    TEST_SYM="${TEST_NAME_BASE}-${DIR}-symlink-cli-ok"
-    if [[ $(readlink "$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}") == \
-   "$TMPDIR/${USER}/test_cylc_cli_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}" ]]; then
-        ok "$TEST_SYM"
-    else
-        fail "$TEST_SYM"
-    fi
+    TEST_SYM="${TEST_NAME}-${DIR}"
+    run_fail "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}/${DIR}")" \
+   = "$TMPDIR/${USER}/test_cylc_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}"
 done
-rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
+
 purge_rnd_workflow
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index 1f36aafad84..c06245fc7ea 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -1450,6 +1450,7 @@ def test_check_flow_file_symlink(
     [
         ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ', 'None',
             {'localhost': {
+                'run': None,
                 'log': "$shortbread",
                 'share': '$bourbon',
                 'share/cycle': '$digestive'
@@ -1468,11 +1469,17 @@ def test_check_flow_file_symlink(
          ),
     ]
 )
-def test_get_sym_dirs(symlink_dirs, err_msg, expected):
-    """Test get_sym_dirs raises errors on cli symlink options"""
+def test_get_sym_dirs(
+    symlink_dirs: str,
+    err_msg: str,
+    expected: Dict[str, Dict[str, Any]]
+):
+    """Test get_sym_dirs returns dict or correctly raises errors on cli symlink
+        dir options"""
     if err_msg != 'None':
-        with pytest.raises(WorkflowFilesError):
+        with pytest.raises(WorkflowFilesError) as exc:
             get_sym_dirs(symlink_dirs)
+            assert(err_msg) in str(exc)
 
     else:
         actual = get_sym_dirs(symlink_dirs)

From d627c2263de4068fba26aba124180803e7694284 Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Fri, 28 May 2021 11:52:52 +0100
Subject: [PATCH 4/9] tidy

---
 cylc/flow/scripts/install.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index ad033e1df92..b406902cd1a 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -153,10 +153,10 @@ def get_option_parser():
     parser.add_option(
         "--symlink-dirs",
         help=(
-            "Enter a list, in the form ['log'= 'path/to/store', share = '$...'"
-            "]. Use this option to override creating default local symlinks"
-            ", for directories run, log, work, share, share/cycle, as"
-            " configured in global.cylc."),
+            "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."
+        ),
         action="store",
         default=None,
         dest="symlink_dirs")

From 95205467794791ec5a18d0e16c643aefd228b5f5 Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Wed, 9 Jun 2021 09:52:09 +0100
Subject: [PATCH 5/9] skip symlink creation if any exist

symlink-dirs=, cylc play skip local sym-dirs for installed workflows
---
 CHANGES.md                                  |  5 ++-
 cylc/flow/pathutil.py                       |  5 ++-
 cylc/flow/scripts/install.py                | 12 ++---
 cylc/flow/workflow_files.py                 | 49 +++++++++++++--------
 tests/functional/cylc-install/01-symlinks.t | 27 +++++++++---
 tests/unit/conftest.py                      |  8 +++-
 tests/unit/test_workflow_files.py           | 15 +++++++
 7 files changed, 90 insertions(+), 31 deletions(-)

diff --git a/CHANGES.md b/CHANGES.md
index 63e1baf9b46..e8238f88137 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -72,6 +72,10 @@ Add an option for displaying source workflows in `cylc scan`.
 - Expose runahead limiting to UIs; restore correct force-triggering of queued
 tasks for Cylc 8.
 
+[#4250](https://github.com/cylc/cylc-flow/pull/4250) -
+Symlink dirs localhost symlinks are now overridable with cli option
+`--symlink-dirs`.
+
 [#4103](https://github.com/cylc/cylc-flow/pull/4103) -
 Expose runahead limiting to UIs; restore correct force-triggering of queued
 tasks for Cylc 8.
@@ -123,7 +127,6 @@ when initial cycle point is not valid for the cycling type.
 [#4228](https://github.com/cylc/cylc-flow/pull/4228) - Interacting with a
 workflow on the cli using `runN` is now supported.
 
-
 [#4193](https://github.com/cylc/cylc-flow/pull/4193) - Standard `cylc install`
 now correctly installs from directories with a `.` in the name. Symlink dirs
 now correctly expands environment variables on the remote. Fixes minor cosmetic
diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index 77522419b6e..0f78a761510 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -142,14 +142,17 @@ def make_localhost_symlinks(
     Returns:
         Dictionary of symlinks with sources as keys and
         destinations as values: ``{source: destination}``
+        symlink_conf: Symlinks dirs configuration passed from cli
 
     """
+    symlinks_created = {}
     dirs_to_symlink = get_dirs_to_symlink(
         get_localhost_install_target(),
         named_sub_dir, symlink_conf=symlink_conf
     )
-    symlinks_created = {}
     for key, value in dirs_to_symlink.items():
+        if value is None:
+            continue
         if key == 'run':
             symlink_path = rund
         else:
diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index b406902cd1a..017d3f29e6a 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -155,11 +155,12 @@ def get_option_parser():
         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."
+            " log, work, share, share/cycle, as configured in global.cylc. "
+            "Enter an empty list \"\" to skip making localhost symlink dirs."
         ),
         action="store",
-        default=None,
-        dest="symlink_dirs")
+        dest="symlink_dirs"
+    )
 
     parser.add_option(
         "--run-name",
@@ -219,9 +220,10 @@ def install(
                 exc
             ) from None
     cli_symdirs: Optional[Dict[str, Dict[str, Any]]] = None
-    if opts.symlink_dirs:
+    if opts.symlink_dirs == '':
+        cli_symdirs = {}
+    elif opts.symlink_dirs:
         cli_symdirs = get_sym_dirs(opts.symlink_dirs)
-
     source_dir, rundir, _flow_name = install_workflow(
         flow_name=flow_name,
         source=source,
diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index a9f1d4d45f2..a27ce744313 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -581,26 +581,38 @@ def register(
     # flow.cylc must exist so we can detect accidentally reversed args.
     source = os.path.abspath(source)
     check_flow_file(source, symlink_suiterc=True, logger=None)
-    symlinks_created = make_localhost_symlinks(
-        get_workflow_run_dir(flow_name), flow_name)
-    if bool(symlinks_created):
-        for src, dst in symlinks_created.items():
-            LOG.info(f"Symlink created from {src} to {dst}")
+    if not is_installed(get_workflow_run_dir(flow_name)):
+        symlinks_created = make_localhost_symlinks(
+            get_workflow_run_dir(flow_name), flow_name)
+        if bool(symlinks_created):
+            for src, dst in symlinks_created.items():
+                LOG.info(f"Symlink created from {src} to {dst}")
     # Create service dir if necessary.
     srv_d = get_workflow_srv_dir(flow_name)
     os.makedirs(srv_d, exist_ok=True)
     return flow_name
 
 
-def is_installed(path: Union[Path, str]) -> bool:
+def is_installed(rund: Union[Path, str]) -> bool:
     """Check to see if the path sent contains installed flow.
 
-    Checks for valid _cylc-install directory in current folder and checks
-    source link exists.
+    Checks for valid _cylc-install directory in the two possible locations in
+    relation to the run directory.
+
+    Args:
+        rund (Union[Path, str]): run directory path to check
+
+    Returns:
+        bool: True if rund belongs to an installed workflow
     """
-    cylc_install_folder = Path(path, WorkflowFiles.Install.DIRNAME)
-    source = Path(cylc_install_folder, WorkflowFiles.Install.SOURCE)
-    return cylc_install_folder.is_dir() and source.is_symlink()
+    rund = Path(rund)
+    cylc_install_dir = Path(rund, WorkflowFiles.Install.DIRNAME)
+    alt_cylc_install_dir = Path(rund.parent, WorkflowFiles.Install.DIRNAME)
+    if cylc_install_dir.is_dir():
+        return True
+    elif alt_cylc_install_dir.is_dir():
+        return True
+    return False
 
 
 def _clean_check(reg: str, run_dir: Path) -> None:
@@ -1246,7 +1258,7 @@ def install_workflow(
     source: Optional[Union[Path, str]] = None,
     run_name: Optional[str] = None,
     no_run_name: bool = False,
-    cli_symlink_dirs: Optional[Union[Dict[str, Dict[str, Any]], None]] = None
+    cli_symlink_dirs: Optional[Dict[str, Dict[str, Any]]] = None
 ) -> Tuple[Path, Path, str]:
     """Install a workflow, or renew its installation.
 
@@ -1498,7 +1510,8 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
 
     Args:
         symlink_dirs (str): As entered by user on cli,
-                            e.g. [log=$DIR, share=$DIR2].
+                            e.g. "log=$DIR, share=$DIR2".
+        fill_syms (bool): If True, will fill missing
 
     Raises:
         WorkflowFilesError: If directory to be symlinked is not in permitted
@@ -1513,20 +1526,20 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     """
     # Ensures the same nested dict format which is returned by the glb cfg
     symdict: Dict[str, Dict[str, Any]] = {'localhost': {'run': None}}
-    if symlink_dirs == 'None':
+    if symlink_dirs == "":
         return symdict
     symlist = symlink_dirs.replace(" ", "").strip(',').split(',')
     for pair in symlist:
         try:
             key, val = pair.split("=")
         except ValueError:
-            return symdict
-        if key in ['run', 'log', 'share', 'share/cycle', 'work']:
-            symdict['localhost'][key] = val
-        else:
+            continue
+        if key not in ['run', 'log', 'share', 'share/cycle', 'work']:
             raise WorkflowFilesError(
                 f"{key} not a valid entry for --symlink-dirs"
             )
+        symdict['localhost'][key] = val.strip() or None
+
     return symdict
 
 
diff --git a/tests/functional/cylc-install/01-symlinks.t b/tests/functional/cylc-install/01-symlinks.t
index b8c6dfac3a5..49e3a781e29 100644
--- a/tests/functional/cylc-install/01-symlinks.t
+++ b/tests/functional/cylc-install/01-symlinks.t
@@ -24,7 +24,7 @@ if [[ -z ${TMPDIR:-} || -z ${USER:-} || $TMPDIR/$USER == "$HOME" ]]; then
     skip_all '"TMPDIR" or "USER" not defined or "TMPDIR"/"USER" is "HOME"'
 fi
 
-set_test_number 25
+set_test_number 27
 
 create_test_global_config "" "
 [install]
@@ -106,7 +106,7 @@ __FLOW__
 
 popd || exit 1
 
-run_ok "${TEST_NAME_BASE}-play" cylc play "${RND_WORKFLOW_NAME}/runN" --debug
+run_ok "${TEST_NAME_BASE}-play" cylc play "${RND_WORKFLOW_NAME}/runN" --debug --no-detach
 
 # test ensure symlinks, not in cli install are not created from glbl cfg.
 TEST_SYM="${TEST_NAME_BASE}-share-cycle-cli"
@@ -117,13 +117,12 @@ rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
 purge_rnd_workflow
 
 
-# test no symlinks created with --symlink-dirs=None
-
+# test no symlinks created with --symlink-dirs=""
 
 TEST_NAME="${TEST_NAME_BASE}-no-sym-dirs-cli"
 make_rnd_workflow
 run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" \
---directory="${RND_WORKFLOW_SOURCE}" --symlink-dirs=None
+--directory="${RND_WORKFLOW_SOURCE}" --symlink-dirs=""
 contains_ok "${TEST_NAME}.stdout" <<__OUT__
 INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
 __OUT__
@@ -144,4 +143,22 @@ for DIR in 'work' 'share' 'log'; do
    = "$TMPDIR/${USER}/test_cylc_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/${DIR}"
 done
 
+pushd "${WORKFLOW_RUN_DIR}" || exit 1
+cat > 'flow.cylc' << __FLOW__
+[scheduling]
+    [[graph]]
+        R1 = foo
+[runtime]
+    [[foo]]
+        script = true
+__FLOW__
+
+popd || exit 1
+
+run_ok "${TEST_NAME_BASE}-play" cylc play "${RND_WORKFLOW_NAME}/runN" --debug --no-detach
+# test ensure localhost symlink dirs skipped for installed workflows.
+TEST_SYM="${TEST_NAME_BASE}-installed-workflow-skips-symdirs"
+run_fail "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}")" \
+    = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1"
+rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
 purge_rnd_workflow
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index f50009a7e1d..22a862696d5 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -63,8 +63,9 @@ def tmp_run_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
 
     Args:
         reg: Workflow name.
+        installed: named or no-name. Creates 
     """
-    def _tmp_run_dir(reg: Optional[str] = None) -> Path:
+    def _tmp_run_dir(reg: Optional[str] = None, installed = None) -> Path:
         cylc_run_dir = tmp_path / 'cylc-run'
         cylc_run_dir.mkdir(exist_ok=True)
         monkeypatch.setattr('cylc.flow.pathutil._CYLC_RUN_DIR', cylc_run_dir)
@@ -73,6 +74,11 @@ def _tmp_run_dir(reg: Optional[str] = None) -> Path:
             run_dir.mkdir(parents=True, exist_ok=True)
             (run_dir / WorkflowFiles.FLOW_FILE).touch(exist_ok=True)
             (run_dir / WorkflowFiles.Service.DIRNAME).mkdir(exist_ok=True)
+            if installed == 'named':
+                (run_dir.parent / WorkflowFiles.Install.DIRNAME).mkdir(exist_ok=True)
+            elif installed == 'no-name':
+                (run_dir / WorkflowFiles.Install.DIRNAME).mkdir(exist_ok=True)
+
             return run_dir
         return cylc_run_dir
     return _tmp_run_dir
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index c06245fc7ea..be617409d90 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -42,6 +42,8 @@
     check_nested_run_dirs,
     get_sym_dirs,
     get_symlink_dirs,
+    is_installed,
+    get_sym_dirs,
     get_workflow_source_dir,
     glob_in_run_dir,
     reinstall_workflow,
@@ -1485,3 +1487,16 @@ def test_get_sym_dirs(
         actual = get_sym_dirs(symlink_dirs)
 
         assert actual == expected
+
+@pytest.mark.parametrize(
+    'reg, installed, expected',
+    [('reg1/run1', 'named', True),
+     ('reg2', 'no-name', True),
+     ('reg3', None, False)]
+)
+def test_is_installed(tmp_run_dir: Callable,reg, installed, expected):
+    
+
+    cylc_run_dir: Path = tmp_run_dir(reg, installed=installed)
+    actual = is_installed(cylc_run_dir)
+    assert actual == expected

From 3e45611c0ca2f2cff2f55a9ee39f56a6545dc384 Mon Sep 17 00:00:00 2001
From: Melanie Hall <37735232+datamel@users.noreply.github.com>
Date: Wed, 23 Jun 2021 09:12:24 +0100
Subject: [PATCH 6/9] Review Feedback HO

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

Rebase Tidy
---
 cylc/flow/cfgspec/globalcfg.py            | 15 ++++-------
 cylc/flow/pathutil.py                     |  8 +++---
 cylc/flow/workflow_files.py               | 32 +++++++++++++----------
 tests/functional/cylc-clean/02-targeted.t | 17 ++++++------
 tests/unit/test_workflow_files.py         | 20 ++++++++------
 5 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py
index b73c9183b6e..170d206d70c 100644
--- a/cylc/flow/cfgspec/globalcfg.py
+++ b/cylc/flow/cfgspec/globalcfg.py
@@ -372,13 +372,12 @@
         # Symlink Dirs
         with Conf('symlink dirs',  # noqa: SIM117 (keep same format)
                   desc="""
-            Define directories to be moved, symlinks from the the original
-            ``$HOME/cylc-run`` directories will be created.
+            Configure alternate workflow run directory locations. Symlinks from
+            the the standard ``$HOME/cylc-run`` locations will be created.
         """):
             with Conf('<install target>'):
                 Conf('run', VDR.V_STRING, None, desc="""
-                    Specifies the directory where the workflow run directories
-                    are created. If specified, the workflow run directory will
+                    If specified, the workflow run directory will
                     be created in ``<run dir>/cylc-run/<workflow-name>`` and a
                     symbolic link will be created from
                     ``$HOME/cylc-run/<workflow-name>``.
@@ -388,7 +387,6 @@
                     installed into this directory.
                 """)
                 Conf('log', VDR.V_STRING, None, desc="""
-                    Specifies the directory where log directories are created.
                     If specified the workflow log directory will be created in
                     ``<log dir>/cylc-run/<workflow-name>/log`` and a symbolic
                     link will be created from
@@ -397,8 +395,7 @@
                     ``$HOME/cylc-run/<workflow-name>/log``.
                 """)
                 Conf('share', VDR.V_STRING, None, desc="""
-                    Specifies the directory where share directories are
-                    created. If specified the workflow share directory will be
+                    If specified the workflow share directory will be
                     created in ``<share dir>/cylc-run/<workflow-name>/share``
                     and a symbolic link will be created from
                     ``<$HOME/cylc-run/<workflow-name>/share``. If not specified
@@ -406,8 +403,7 @@
                     ``$HOME/cylc-run/<workflow-name>/share``.
                 """)
                 Conf('share/cycle', VDR.V_STRING, None, desc="""
-                    Specifies the directory where share/cycle directories are
-                    created. If specified the workflow share/cycle directory
+                    If specified the workflow share/cycle directory
                     will be created in
                     ``<share/cycle dir>/cylc-run/<workflow-name>/share/cycle``
                     and a symbolic link will be created from
@@ -416,7 +412,6 @@
                     created in ``$HOME/cylc-run/<workflow-name>/share/cycle``.
                 """)
                 Conf('work', VDR.V_STRING, None, desc="""
-                    Specifies the directory where work directories are created.
                     If specified the workflow work directory will be created in
                     ``<work dir>/cylc-run/<workflow-name>/work`` and a symbolic
                     link will be created from
diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index 0f78a761510..94b77011658 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -19,7 +19,7 @@
 from pathlib import Path
 import re
 from shutil import rmtree
-from typing import Dict, Iterable, Set, Union, Optional, OrderedDict, Any
+from typing import Dict, Iterable, Set, Union, Optional, Any
 
 from cylc.flow import LOG
 from cylc.flow.cfgspec.glbl_cfg import glbl_cfg
@@ -131,7 +131,7 @@ def make_workflow_run_tree(workflow):
 
 def make_localhost_symlinks(
     rund: Union[Path, str],
-    named_sub_dir: str, 
+    named_sub_dir: str,
     symlink_conf: Optional[Dict[str, Dict[str, str]]] = None
 ) -> Dict[str, Union[Path, str]]:
     """Creates symlinks for any configured symlink dirs from glbl_cfg.
@@ -190,7 +190,7 @@ def get_dirs_to_symlink(
         dirs_to_symlink: [directory: symlink_path]
     """
     dirs_to_symlink: Dict[str, Any] = {}
-    if not symlink_conf:
+    if symlink_conf is None:
         symlink_conf = glbl_cfg().get(['install', 'symlink dirs'])
     if install_target not in symlink_conf.keys():
         return dirs_to_symlink
@@ -199,7 +199,7 @@ def get_dirs_to_symlink(
         dirs_to_symlink['run'] = os.path.join(base_dir, 'cylc-run', flow_name)
     for dir_ in ['log', 'share', 'share/cycle', 'work']:
         link = symlink_conf[install_target].get(dir_, None)
-        if (not link) is None or link == base_dir:
+        if (not link) or link == base_dir:
             continue
         dirs_to_symlink[dir_] = os.path.join(link, 'cylc-run', flow_name, dir_)
     return dirs_to_symlink
diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index a27ce744313..9922d0d1f6a 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -43,8 +43,9 @@
     PlatformLookupError,
     ServiceFileError,
     TaskRemoteMgmtError,
-    WorkflowFilesError,
-    handle_rmtree_err
+    handle_rmtree_err,
+    UserInputError,
+    WorkflowFilesError
 )
 from cylc.flow.pathutil import (
     expand_path,
@@ -227,7 +228,7 @@ class Install:
         SHARE_CYCLE_DIR, SHARE_DIR, LOG_DIR, WORK_DIR, ''
     ])
     """The paths of the symlink dirs that may be set in
-    global.cylc[symlink dirs], relative to the run dir
+    global.cylc[install]symlink dirs, relative to the run dir
     ('' represents the run dir)."""
 
 
@@ -584,7 +585,7 @@ def register(
     if not is_installed(get_workflow_run_dir(flow_name)):
         symlinks_created = make_localhost_symlinks(
             get_workflow_run_dir(flow_name), flow_name)
-        if bool(symlinks_created):
+        if symlinks_created:
             for src, dst in symlinks_created.items():
                 LOG.info(f"Symlink created from {src} to {dst}")
     # Create service dir if necessary.
@@ -608,11 +609,7 @@ def is_installed(rund: Union[Path, str]) -> bool:
     rund = Path(rund)
     cylc_install_dir = Path(rund, WorkflowFiles.Install.DIRNAME)
     alt_cylc_install_dir = Path(rund.parent, WorkflowFiles.Install.DIRNAME)
-    if cylc_install_dir.is_dir():
-        return True
-    elif alt_cylc_install_dir.is_dir():
-        return True
-    return False
+    return cylc_install_dir.is_dir() or alt_cylc_install_dir.is_dir()
 
 
 def _clean_check(reg: str, run_dir: Path) -> None:
@@ -1316,7 +1313,7 @@ def install_workflow(
     symlinks_created = make_localhost_symlinks(
         rundir, named_run, symlink_conf=cli_symlink_dirs)
     install_log = _get_logger(rundir, 'cylc-install')
-    if bool(symlinks_created) is True:
+    if symlinks_created:
         for src, dst in symlinks_created.items():
             install_log.info(f"Symlink created from {src} to {dst}")
     try:
@@ -1528,15 +1525,22 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     symdict: Dict[str, Dict[str, Any]] = {'localhost': {'run': None}}
     if symlink_dirs == "":
         return symdict
-    symlist = symlink_dirs.replace(" ", "").strip(',').split(',')
+    symlist = symlink_dirs.strip(',').split(',')
     for pair in symlist:
         try:
             key, val = pair.split("=")
+            key = key.strip()
         except ValueError:
-            continue
+            raise UserInputError(
+                'There is an error in --symlink-dirs option:'
+                f' {pair}. Try entering option in the form '
+                '--symlink-dirs=\'log=$DIR, share=$DIR2, ...\''
+            )
         if key not in ['run', 'log', 'share', 'share/cycle', 'work']:
-            raise WorkflowFilesError(
-                f"{key} not a valid entry for --symlink-dirs"
+            raise UserInputError(
+                f"{key} not a valid entry for --symlink-dirs. "
+                "Configurable symlink dirs are: 'run', 'log', 'share',"
+                " 'share/cycle', 'work'"
             )
         symdict['localhost'][key] = val.strip() or None
 
diff --git a/tests/functional/cylc-clean/02-targeted.t b/tests/functional/cylc-clean/02-targeted.t
index 08a7c417b5d..d50a189fd5e 100644
--- a/tests/functional/cylc-clean/02-targeted.t
+++ b/tests/functional/cylc-clean/02-targeted.t
@@ -28,14 +28,15 @@ SYM_NAME="$(mktemp -u)"
 SYM_NAME="sym-${SYM_NAME##*tmp.}"
 
 create_test_global_config "" "
-[symlink dirs]
-    [[localhost]]
-        run = ${TEST_DIR}/${SYM_NAME}/run
-        log = ${TEST_DIR}/${SYM_NAME}/other
-        share = ${TEST_DIR}/${SYM_NAME}/other
-        work = ${TEST_DIR}/${SYM_NAME}/other
-        # Need to override any symlink dirs set in global-tests.cylc:
-        share/cycle =
+[install]
+    [[symlink dirs]]
+        [[[localhost]]]
+            run = ${TEST_DIR}/${SYM_NAME}/run
+            log = ${TEST_DIR}/${SYM_NAME}/other
+            share = ${TEST_DIR}/${SYM_NAME}/other
+            work = ${TEST_DIR}/${SYM_NAME}/other
+            # Need to override any symlink dirs set in global-tests.cylc:
+            share/cycle =
 "
 init_workflow "${TEST_NAME_BASE}" << __FLOW__
 [scheduler]
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index be617409d90..0227df8e04a 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -29,6 +29,7 @@
     CylcError,
     ServiceFileError,
     TaskRemoteMgmtError,
+    UserInputError,
     WorkflowFilesError
 )
 from cylc.flow.option_parsers import Options
@@ -1450,13 +1451,16 @@ def test_check_flow_file_symlink(
 @pytest.mark.parametrize(
     'symlink_dirs, err_msg, expected',
     [
-        ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ', 'None',
-            {'localhost': {
-                'run': None,
-                'log': "$shortbread",
-                'share': '$bourbon',
-                'share/cycle': '$digestive'
-            }}
+        ('log=$shortbread, share= $bourbon,share/cycle= $digestive, ',
+         "There is an error in --symlink-dirs option:",
+            None
+         ),
+        ('log=$shortbread share= $bourbon share/cycle= $digestive ',
+         "There is an error in --symlink-dirs option:"
+         " log=$shortbread share= $bourbon share/cycle= $digestive . "
+         "Try entering option in the form --symlink-dirs="
+         "'log=$DIR, share=$DIR2, ...'",
+            None
          ),
         ('run=$NICE, log= $Garibaldi, share/cycle=$RichTea', 'None',
             {'localhost': {
@@ -1479,7 +1483,7 @@ def test_get_sym_dirs(
     """Test get_sym_dirs returns dict or correctly raises errors on cli symlink
         dir options"""
     if err_msg != 'None':
-        with pytest.raises(WorkflowFilesError) as exc:
+        with pytest.raises(UserInputError) as exc:
             get_sym_dirs(symlink_dirs)
             assert(err_msg) in str(exc)
 

From 4023cc8b070fc88927637e06eb4638321ef49302 Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Thu, 1 Jul 2021 09:43:14 +0100
Subject: [PATCH 7/9] review feedback RD

---
 cylc/flow/pathutil.py                       |  9 +++++--
 cylc/flow/scripts/install.py                |  4 +--
 cylc/flow/workflow_files.py                 | 14 ++++++----
 tests/functional/cylc-install/01-symlinks.t | 24 ++++++++++++++++-
 tests/unit/conftest.py                      | 25 ++++++++++++-----
 tests/unit/test_workflow_files.py           | 30 ++++++++++-----------
 6 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index 94b77011658..f1672fe216f 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -138,11 +138,11 @@ def make_localhost_symlinks(
     Args:
         rund: the entire run directory path
         named_sub_dir: e.g flow_name/run1
+        symlink_conf: Symlinks dirs configuration passed from cli
 
     Returns:
         Dictionary of symlinks with sources as keys and
         destinations as values: ``{source: destination}``
-        symlink_conf: Symlinks dirs configuration passed from cli
 
     """
     symlinks_created = {}
@@ -215,7 +215,7 @@ def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool:
     path = Path(path)
     target = Path(target)
     if path.exists():
-        if path.is_symlink() and path.samefile(target):
+        if path.is_symlink() and target.exists() and path.samefile(target):
             # correct symlink already exists
             return False
         # symlink name is in use by a physical file or directory
@@ -232,6 +232,11 @@ def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool:
             raise WorkflowFilesError(
                 f"Error when symlinking. Failed to unlink bad symlink {path}.")
     target.mkdir(parents=True, exist_ok=True)
+
+    # This is needed in case share and share/cycle have the same symlink dir:
+    if path.exists():
+        return False
+
     path.parent.mkdir(parents=True, exist_ok=True)
     try:
         path.symlink_to(target)
diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py
index 017d3f29e6a..f06f444b91f 100755
--- a/cylc/flow/scripts/install.py
+++ b/cylc/flow/scripts/install.py
@@ -75,7 +75,7 @@
 from cylc.flow.exceptions import PluginError
 from cylc.flow.option_parsers import CylcOptionParser as COP
 from cylc.flow.workflow_files import (
-    install_workflow, search_install_source_dirs, get_sym_dirs
+    install_workflow, search_install_source_dirs, parse_cli_sym_dirs
 )
 from cylc.flow.terminal import cli_function
 
@@ -223,7 +223,7 @@ def install(
     if opts.symlink_dirs == '':
         cli_symdirs = {}
     elif opts.symlink_dirs:
-        cli_symdirs = get_sym_dirs(opts.symlink_dirs)
+        cli_symdirs = parse_cli_sym_dirs(opts.symlink_dirs)
     source_dir, rundir, _flow_name = install_workflow(
         flow_name=flow_name,
         source=source,
diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index 9922d0d1f6a..d774683f414 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -228,7 +228,7 @@ class Install:
         SHARE_CYCLE_DIR, SHARE_DIR, LOG_DIR, WORK_DIR, ''
     ])
     """The paths of the symlink dirs that may be set in
-    global.cylc[install]symlink dirs, relative to the run dir
+    global.cylc[install][symlink dirs], relative to the run dir
     ('' represents the run dir)."""
 
 
@@ -1502,7 +1502,7 @@ def validate_source_dir(source, flow_name):
     check_flow_file(source, logger=None)
 
 
-def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
+def parse_cli_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     """Converts command line entered symlink dirs to a dictionary.
 
     Args:
@@ -1526,6 +1526,9 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     if symlink_dirs == "":
         return symdict
     symlist = symlink_dirs.strip(',').split(',')
+    possible_symlink_dirs = WorkflowFiles.SYMLINK_DIRS.symmetric_difference(
+        {'', WorkflowFiles.RUN_DIR}
+    )
     for pair in symlist:
         try:
             key, val = pair.split("=")
@@ -1536,11 +1539,12 @@ def get_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
                 f' {pair}. Try entering option in the form '
                 '--symlink-dirs=\'log=$DIR, share=$DIR2, ...\''
             )
-        if key not in ['run', 'log', 'share', 'share/cycle', 'work']:
+        if key not in possible_symlink_dirs:
+            dirs = str(
+                possible_symlink_dirs).lstrip('frozenset({').rstrip('})')
             raise UserInputError(
                 f"{key} not a valid entry for --symlink-dirs. "
-                "Configurable symlink dirs are: 'run', 'log', 'share',"
-                " 'share/cycle', 'work'"
+                f"Configurable symlink dirs are: {dirs}"
             )
         symdict['localhost'][key] = val.strip() or None
 
diff --git a/tests/functional/cylc-install/01-symlinks.t b/tests/functional/cylc-install/01-symlinks.t
index 49e3a781e29..0abb6720b3d 100644
--- a/tests/functional/cylc-install/01-symlinks.t
+++ b/tests/functional/cylc-install/01-symlinks.t
@@ -24,7 +24,7 @@ if [[ -z ${TMPDIR:-} || -z ${USER:-} || $TMPDIR/$USER == "$HOME" ]]; then
     skip_all '"TMPDIR" or "USER" not defined or "TMPDIR"/"USER" is "HOME"'
 fi
 
-set_test_number 27
+set_test_number 30
 
 create_test_global_config "" "
 [install]
@@ -162,3 +162,25 @@ run_fail "${TEST_SYM}" test "$(readlink "${WORKFLOW_RUN_DIR}")" \
     = "$TMPDIR/${USER}/test_cylc_symlink/cylctb_tmp_run_dir/cylc-run/${RND_WORKFLOW_NAME}/run1"
 rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
 purge_rnd_workflow
+
+
+# test share and share/cycle same symlinks don't error 
+SYMDIR=${TMPDIR}/${USER}/test_cylc_cli_symlink/
+
+TEST_NAME="${TEST_NAME_BASE}-share-share-cycle-same-dirs"
+make_rnd_workflow
+# check install runs without failure
+run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" \
+--directory="${RND_WORKFLOW_SOURCE}" \
+--symlink-dirs="share/cycle=${SYMDIR}, share=${SYMDIR}"
+contains_ok "${TEST_NAME}.stdout" <<__OUT__
+INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE}
+__OUT__
+WORKFLOW_RUN_DIR="$HOME/cylc-run/${RND_WORKFLOW_NAME}/run1"
+
+TEST_SYM="${TEST_NAME_BASE}-share-cli"
+run_ok "$TEST_SYM" test "$(readlink "${WORKFLOW_RUN_DIR}/share")" \
+   = "${TMPDIR}/${USER}/test_cylc_cli_symlink/cylc-run/${RND_WORKFLOW_NAME}/run1/share"
+
+rm -rf "${TMPDIR}/${USER}/test_cylc_cli_symlink/"
+purge_rnd_workflow
\ No newline at end of file
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 22a862696d5..d176e3f086e 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -63,9 +63,17 @@ def tmp_run_dir(tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
 
     Args:
         reg: Workflow name.
-        installed: named or no-name. Creates 
+        installed: If True, make it look like the workflow was installed
+            using cylc install (creates _cylc-install dir).
+        named: If True and installed is True, the _cylc-install dir will
+            be created in the parent to make it look like this is a
+            named run.
     """
-    def _tmp_run_dir(reg: Optional[str] = None, installed = None) -> Path:
+    def _tmp_run_dir(
+        reg: Optional[str] = None,
+        installed: bool = False,
+        named: bool = False
+    ) -> Path:
         cylc_run_dir = tmp_path / 'cylc-run'
         cylc_run_dir.mkdir(exist_ok=True)
         monkeypatch.setattr('cylc.flow.pathutil._CYLC_RUN_DIR', cylc_run_dir)
@@ -74,10 +82,15 @@ def _tmp_run_dir(reg: Optional[str] = None, installed = None) -> Path:
             run_dir.mkdir(parents=True, exist_ok=True)
             (run_dir / WorkflowFiles.FLOW_FILE).touch(exist_ok=True)
             (run_dir / WorkflowFiles.Service.DIRNAME).mkdir(exist_ok=True)
-            if installed == 'named':
-                (run_dir.parent / WorkflowFiles.Install.DIRNAME).mkdir(exist_ok=True)
-            elif installed == 'no-name':
-                (run_dir / WorkflowFiles.Install.DIRNAME).mkdir(exist_ok=True)
+            if installed:
+                if named:
+                    if len(Path(reg).parts) < 2:
+                        raise ValueError("Named run requires two-level reg")
+                    (run_dir.parent / WorkflowFiles.Install.DIRNAME).mkdir(
+                        exist_ok=True)
+                else:
+                    (run_dir / WorkflowFiles.Install.DIRNAME).mkdir(
+                        exist_ok=True)
 
             return run_dir
         return cylc_run_dir
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index 0227df8e04a..9c0c8948185 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -41,10 +41,10 @@
     _remote_clean_cmd,
     check_flow_file,
     check_nested_run_dirs,
-    get_sym_dirs,
+    parse_cli_sym_dirs,
     get_symlink_dirs,
     is_installed,
-    get_sym_dirs,
+    parse_cli_sym_dirs,
     get_workflow_source_dir,
     glob_in_run_dir,
     reinstall_workflow,
@@ -1475,32 +1475,32 @@ def test_check_flow_file_symlink(
          ),
     ]
 )
-def test_get_sym_dirs(
+def test_parse_cli_sym_dirs(
     symlink_dirs: str,
     err_msg: str,
     expected: Dict[str, Dict[str, Any]]
 ):
-    """Test get_sym_dirs returns dict or correctly raises errors on cli symlink
-        dir options"""
+    """Test parse_cli_sym_dirs returns dict or correctly raises errors on cli
+    symlink dir options"""
     if err_msg != 'None':
         with pytest.raises(UserInputError) as exc:
-            get_sym_dirs(symlink_dirs)
+            parse_cli_sym_dirs(symlink_dirs)
             assert(err_msg) in str(exc)
 
     else:
-        actual = get_sym_dirs(symlink_dirs)
+        actual = parse_cli_sym_dirs(symlink_dirs)
 
         assert actual == expected
 
+
 @pytest.mark.parametrize(
-    'reg, installed, expected',
-    [('reg1/run1', 'named', True),
-     ('reg2', 'no-name', True),
-     ('reg3', None, False)]
+    'reg, installed, named,  expected',
+    [('reg1/run1', True, True, True),
+     ('reg2', True, False, True),
+     ('reg3', False, False, False)]
 )
-def test_is_installed(tmp_run_dir: Callable,reg, installed, expected):
-    
-
-    cylc_run_dir: Path = tmp_run_dir(reg, installed=installed)
+def test_is_installed(tmp_run_dir: Callable, reg, installed, named, expected):
+    """Test is_installed correctly identifies presence of _cylc-install dir"""
+    cylc_run_dir: Path = tmp_run_dir(reg, installed=installed, named=named)
     actual = is_installed(cylc_run_dir)
     assert actual == expected

From 8424c70f834bb9d5c02fa137556bfcad4626a9db Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Mon, 12 Jul 2021 10:52:34 +0100
Subject: [PATCH 8/9] respond to RD's comment request

---
 cylc/flow/pathutil.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py
index f1672fe216f..e3c3ff46c23 100644
--- a/cylc/flow/pathutil.py
+++ b/cylc/flow/pathutil.py
@@ -215,6 +215,8 @@ def make_symlink(path: Union[Path, str], target: Union[Path, str]) -> bool:
     path = Path(path)
     target = Path(target)
     if path.exists():
+        # note all three checks are needed here due to case where user has set
+        # their own symlink which does not match the global config set one.
         if path.is_symlink() and target.exists() and path.samefile(target):
             # correct symlink already exists
             return False

From c14b904bffd1fb4f329d2dcb326dc6bc3f7eef67 Mon Sep 17 00:00:00 2001
From: Mel Hall <37735232+datamel@users.noreply.github.com>
Date: Tue, 20 Jul 2021 13:14:26 +0100
Subject: [PATCH 9/9] Review feedback RD

---
 cylc/flow/workflow_files.py       | 14 +++++++-------
 tests/unit/test_workflow_files.py |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py
index d774683f414..901cb5373bb 100644
--- a/cylc/flow/workflow_files.py
+++ b/cylc/flow/workflow_files.py
@@ -601,7 +601,7 @@ def is_installed(rund: Union[Path, str]) -> bool:
     relation to the run directory.
 
     Args:
-        rund (Union[Path, str]): run directory path to check
+        rund: run directory path to check
 
     Returns:
         bool: True if rund belongs to an installed workflow
@@ -1271,6 +1271,7 @@ def install_workflow(
         rundir: for overriding the default cylc-run directory.
         no_run_name: Flag as True to install workflow into
             ~/cylc-run/<flow_name>
+        cli_symlink_dirs: Symlink dirs, if entered on the cli.
 
     Return:
         source: The source directory.
@@ -1506,9 +1507,8 @@ def parse_cli_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     """Converts command line entered symlink dirs to a dictionary.
 
     Args:
-        symlink_dirs (str): As entered by user on cli,
+        symlink_dirs: As entered by user on cli,
                             e.g. "log=$DIR, share=$DIR2".
-        fill_syms (bool): If True, will fill missing
 
     Raises:
         WorkflowFilesError: If directory to be symlinked is not in permitted
@@ -1526,9 +1526,10 @@ def parse_cli_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
     if symlink_dirs == "":
         return symdict
     symlist = symlink_dirs.strip(',').split(',')
-    possible_symlink_dirs = WorkflowFiles.SYMLINK_DIRS.symmetric_difference(
-        {'', WorkflowFiles.RUN_DIR}
+    possible_symlink_dirs = set(WorkflowFiles.SYMLINK_DIRS.union(
+        {WorkflowFiles.RUN_DIR})
     )
+    possible_symlink_dirs.remove('')
     for pair in symlist:
         try:
             key, val = pair.split("=")
@@ -1540,8 +1541,7 @@ def parse_cli_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]:
                 '--symlink-dirs=\'log=$DIR, share=$DIR2, ...\''
             )
         if key not in possible_symlink_dirs:
-            dirs = str(
-                possible_symlink_dirs).lstrip('frozenset({').rstrip('})')
+            dirs = ', '.join(possible_symlink_dirs)
             raise UserInputError(
                 f"{key} not a valid entry for --symlink-dirs. "
                 f"Configurable symlink dirs are: {dirs}"
diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py
index 9c0c8948185..650e2ae8c09 100644
--- a/tests/unit/test_workflow_files.py
+++ b/tests/unit/test_workflow_files.py
@@ -1462,7 +1462,7 @@ def test_check_flow_file_symlink(
          "'log=$DIR, share=$DIR2, ...'",
             None
          ),
-        ('run=$NICE, log= $Garibaldi, share/cycle=$RichTea', 'None',
+        ('run=$NICE, log= $Garibaldi, share/cycle=$RichTea', None,
             {'localhost': {
                 'run': '$NICE',
                 'log': '$Garibaldi',
@@ -1482,7 +1482,7 @@ def test_parse_cli_sym_dirs(
 ):
     """Test parse_cli_sym_dirs returns dict or correctly raises errors on cli
     symlink dir options"""
-    if err_msg != 'None':
+    if err_msg is not None:
         with pytest.raises(UserInputError) as exc:
             parse_cli_sym_dirs(symlink_dirs)
             assert(err_msg) in str(exc)