Bug fixes

CHANGES.md

@@ -49,6 +49,12 @@ ones in. -->
 -------------------------------------------------------------------------------
 ## __cylc-8.0b2 (<span actions:bind='release-date'>Released 2021-??-??</span>)__
 
+### Fixes
+
+[#4180](https://github.com/cylc/cylc-flow/pull/4180) - Fix bug where installing
+a workflow that uses the deprecated `suite.rc` filename would symlink `flow.cylc`
+to the `suite.rc` in the source dir instead of the run dir. Also fixes a couple
+of other, small bugs.
 
 -------------------------------------------------------------------------------
 ## __cylc-8.0b1 (<span actions:bind='release-date'>Released 2021-04-21</span>)__
@@ -68,7 +74,7 @@ Replace the job "host" field with "platform" in the GraphQL schema.
 Fix a host ⇒ platform upgrade bug where host names were being popped from task
 configs causing subsequent tasks to run on localhost.
 
-[#4173](https://github.com/cylc/cylc-flow/pull/4173)
+[#4173](https://github.com/cylc/cylc-flow/pull/4173) -
 Fix the state totals shown in both the UI and TUI, including incorrect counts
 during workflow run and post pause.
 

cylc/flow/cfgspec/suite.py

@@ -18,6 +18,7 @@
 import re
 
 from itertools import product
+from typing import Any, Dict, Set
 
 from metomi.isodatetime.data import Calendar
 
@@ -1534,7 +1535,7 @@ def upg(cfg, descr):
                 )
 
 
-def upgrade_graph_section(cfg, descr):
+def upgrade_graph_section(cfg: Dict[str, Any], descr: str) -> None:
     """Upgrade Cylc 7 `[scheduling][dependencies][X]graph` format to
     `[scheduling][graph]X`."""
     # Parsec upgrader cannot do this type of move
@@ -1548,23 +1549,25 @@ def upgrade_graph_section(cfg, descr):
                     f'because {msg_new[:-1]} already exists.'
                 )
             else:
-                keys = set()
+                keys: Set[str] = set()
                 cfg['scheduling'].setdefault('graph', {})
                 cfg['scheduling']['graph'].update(
                     cfg['scheduling'].pop('dependencies')
                 )
-                graphdict = cfg['scheduling']['graph']
+                graphdict: Dict[str, Any] = cfg['scheduling']['graph']
                 for key, value in graphdict.copy().items():
                     if isinstance(value, dict) and 'graph' in value:
                         graphdict[key] = value['graph']
                         keys.add(key)
+                    elif key == 'graph' and isinstance(value, str):
+                        graphdict[key] = value
+                        keys.add(key)
                 if keys:
-                    keys = ', '.join(sorted(keys))
                     LOG.warning(
                         'deprecated graph items were automatically upgraded '
                         f'in "{descr}":\n'
                         f' * (8.0.0) {msg_old} -> {msg_new} - for X in:\n'
-                        f'       {keys}'
+                        f"       {', '.join(sorted(keys))}"
                     )
     except KeyError:
         pass

cylc/flow/scheduler.py

@@ -42,9 +42,7 @@
 from cylc.flow.config import SuiteConfig
 from cylc.flow.cycling.loader import get_point
 from cylc.flow.data_store_mgr import DataStoreMgr, parse_job_item
-from cylc.flow.exceptions import (
-    CyclingError, CylcError, SuiteConfigError, PlatformLookupError
-)
+from cylc.flow.exceptions import CyclingError, CylcError
 import cylc.flow.flags
 from cylc.flow.host_select import select_suite_host
 from cylc.flow.hostuserutil import (
@@ -616,7 +614,6 @@ async def run(self):
 
         """
         try:
-            await self.install()
             await self.initialise()
             await self.configure()
             await self.start_servers()
@@ -1619,11 +1616,13 @@ async def shutdown(self, reason: Exception) -> None:
         except (KeyboardInterrupt, asyncio.CancelledError, Exception) as exc:
             # In case of exception in the shutdown method itself.
             LOG.error("Error during shutdown")
+            # Suppress the reason for shutdown, which is logged separately
+            exc.__suppress_context__ = True
             if isinstance(exc, CylcError):
                 LOG.error(f"{exc.__class__.__name__}: {exc}")
+                if cylc.flow.flags.debug:
+                    LOG.exception(exc)
             else:
-                # Suppress the reason for shutdown, which is logged separately
-                exc.__suppress_context__ = True
                 LOG.exception(exc)
             # Re-raise exception to be caught higher up (sets the exit code)
             raise exc from None
@@ -1637,10 +1636,11 @@ async def _shutdown(self, reason: Exception) -> None:
                 self.resume_workflow(quiet=True)
         elif isinstance(reason, SchedulerError):
             LOG.error(f'Suite shutting down - {reason}')
-        elif isinstance(reason, SuiteConfigError):
-            LOG.error(f'{SuiteConfigError.__name__}: {reason}')
-        elif isinstance(reason, PlatformLookupError):
-            LOG.error(f'{PlatformLookupError.__name__}: {reason}')
+        elif isinstance(reason, CylcError):
+            LOG.error(
+                f"Suite shutting down - {reason.__class__.__name__}: {reason}")
+            if cylc.flow.flags.debug:
+                LOG.exception(reason)
         else:
             LOG.exception(reason)
             if str(reason):

cylc/flow/scheduler_cli.py

@@ -291,7 +291,7 @@ def scheduler_cli(parser, options, reg):
     #       then shutdown async generators and closes the event loop
     scheduler = Scheduler(reg, options)
     asyncio.run(
-        _setup(parser, options, reg, scheduler)
+        _setup(scheduler)
     )
 
     # daemonize if requested
@@ -306,7 +306,7 @@ def scheduler_cli(parser, options, reg):
 
     # run the workflow
     ret = asyncio.run(
-        _run(parser, options, reg, scheduler)
+        _run(scheduler)
     )
 
     # exit
@@ -331,15 +331,15 @@ def _distribute(host):
         sys.exit(0)
 
 
-async def _setup(parser, options, reg, scheduler):
+async def _setup(scheduler: Scheduler) -> None:
     """Initialise the scheduler."""
     try:
         await scheduler.install()
     except SuiteServiceFileError as exc:
         sys.exit(exc)
 
 
-async def _run(parser, options, reg, scheduler):
+async def _run(scheduler: Scheduler) -> int:
     """Run the workflow and handle exceptions."""
     # run cylc run
     ret = 0

cylc/flow/suite_db_mgr.py

@@ -559,7 +559,7 @@ def recover_pub_from_pri(self):
                 f"{self.pri_dao.db_file_name}")
             self.pub_dao.n_tries = 0
 
-    def restart_check(self):
+    def restart_check(self) -> bool:
         """Check & vacuum the runtime DB for a restart.
 
         Raises SuiteServiceFileError if DB is incompatible.
@@ -571,8 +571,7 @@ def restart_check(self):
         except FileNotFoundError:
             return False
         except SuiteServiceFileError as exc:
-            raise SuiteServiceFileError(
-                f"Cannot restart - {exc}")
+            raise SuiteServiceFileError(f"Cannot restart - {exc}")
         pri_dao = self.get_pri_dao()
         try:
             pri_dao.vacuum()
@@ -582,7 +581,7 @@ def restart_check(self):
             pri_dao.close()
         return True
 
-    def check_suite_db_compatibility(self):
+    def check_suite_db_compatibility(self) -> None:
         """Raises SuiteServiceFileError if the existing suite database is
         incompatible with the current version of Cylc."""
         if not os.path.isfile(self.pri_path):

cylc/flow/suite_files.py

@@ -503,7 +503,7 @@ def parse_suite_arg(options, arg):
             if os.path.isdir(arg):
                 path = os.path.join(arg, SuiteFiles.FLOW_FILE)
                 name = os.path.basename(arg)
-                check_flow_file(arg)
+                check_flow_file(arg, logger=None)
             else:
                 path = arg
                 name = os.path.basename(os.path.dirname(arg))
@@ -545,7 +545,7 @@ def register(
         source = os.getcwd()
     # flow.cylc must exist so we can detect accidentally reversed args.
     source = os.path.abspath(source)
-    check_flow_file(source, symlink_suiterc=True)
+    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):
@@ -1142,9 +1142,8 @@ def install_workflow(
             INSTALL_LOG.info(f"Symlink created from {src} to {dst}")
     try:
         rundir.mkdir(exist_ok=True)
-    except OSError as e:
-        if e.strerror == "File exists":
-            raise WorkflowFilesError(f"Run directory already exists : {e}")
+    except FileExistsError:
+        raise WorkflowFilesError("Run directory already exists")
     if relink:
         link_runN(rundir)
     create_workflow_srv_dir(rundir)
@@ -1153,7 +1152,7 @@ def install_workflow(
     stdout, stderr = proc.communicate()
     INSTALL_LOG.info(f"Copying files from {source} to {rundir}")
     INSTALL_LOG.info(f"{stdout}")
-    if not proc.returncode == 0:
+    if proc.returncode != 0:
         INSTALL_LOG.warning(
             f"An error occurred when copying files from {source} to {rundir}")
         INSTALL_LOG.warning(f" Error: {stderr}")
@@ -1245,30 +1244,44 @@ def detect_flow_exists(run_path_base, numbered):
 def check_flow_file(
     path: Union[Path, str],
     symlink_suiterc: bool = False,
-    logger: 'Logger' = LOG
+    logger: Optional['Logger'] = LOG
 ) -> Path:
     """Raises WorkflowFilesError if no flow file in path sent.
 
     Args:
         path: Path to check for a flow.cylc and/or suite.rc file.
-        symlink_suiterc: If True and suite.rc exists but not flow.cylc, create
-            flow.cylc as a symlink to suite.rc.
+        symlink_suiterc: If True and suite.rc exists, create flow.cylc as a
+            symlink to suite.rc. If a flow.cylc symlink already exists but
+            points elsewhere, it will be replaced.
         logger: A custom logger to use to log warnings.
 
     Returns the path of the flow file if present.
     """
     flow_file_path = Path(expand_path(path), SuiteFiles.FLOW_FILE)
+    suite_rc_path = Path(expand_path(path), SuiteFiles.SUITE_RC)
+    depr_msg = (
+        f'The filename "{SuiteFiles.SUITE_RC}" is deprecated '
+        f'in favour of "{SuiteFiles.FLOW_FILE}"')
     if flow_file_path.is_file():
-        # Note: this includes if flow.cylc is a symlink
-        return flow_file_path
-    suite_rc_path = Path(path, SuiteFiles.SUITE_RC)
+        if not flow_file_path.is_symlink():
+            return flow_file_path
+        if flow_file_path.resolve() == suite_rc_path.resolve():
+            # A symlink that points to *existing* suite.rc
+            if logger:
+                logger.warning(depr_msg)
+            return flow_file_path
     if suite_rc_path.is_file():
-        if symlink_suiterc:
-            flow_file_path.symlink_to(suite_rc_path)
-            logger.warning(
-                f'The filename "{SuiteFiles.SUITE_RC}" is deprecated in '
-                f'favour of "{SuiteFiles.FLOW_FILE}". Symlink created.')
-        return suite_rc_path
+        if not symlink_suiterc:
+            if logger:
+                logger.warning(depr_msg)
+            return suite_rc_path
+        if flow_file_path.is_symlink():
+            # Symlink broken or points elsewhere - replace
+            flow_file_path.unlink()
+        flow_file_path.symlink_to(suite_rc_path)
+        if logger:
+            logger.warning(f'{depr_msg}. Symlink created.')
+        return flow_file_path
     raise WorkflowFilesError(
         f"no {SuiteFiles.FLOW_FILE} or {SuiteFiles.SUITE_RC} in {path}")
 
@@ -1303,7 +1316,7 @@ def validate_source_dir(source, flow_name):
         raise WorkflowFilesError(
             f'{flow_name} installation failed. Source directory should not be '
             f'in {cylc_run_dir}')
-    check_flow_file(source)
+    check_flow_file(source, logger=None)
 
 
 def unlink_runN(path: Union[Path, str]) -> bool:
@@ -1339,7 +1352,7 @@ def search_install_source_dirs(flow_name: str) -> Path:
             "does not contain any paths")
     for path in search_path:
         try:
-            flow_file = check_flow_file(Path(path, flow_name))
+            flow_file = check_flow_file(Path(path, flow_name), logger=None)
             return flow_file.parent
         except WorkflowFilesError:
             continue

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

@@ -66,7 +66,8 @@ purge_rnd_suite
 
 # -----------------------------------------------------------------------------
 # Test cylc install succeeds if suite.rc file in source dir
-TEST_NAME="${TEST_NAME_BASE}-no-flow-file"
+# See also tests/functional/deprecations/03-suiterc.t
+TEST_NAME="${TEST_NAME_BASE}-suite.rc"
 make_rnd_suite
 rm -f "${RND_SUITE_SOURCE}/flow.cylc"
 touch "${RND_SUITE_SOURCE}/suite.rc"
@@ -80,11 +81,10 @@ exists_fail "flow.cylc"
 # test symlink correctly made in run dir
 pushd "${RND_SUITE_RUNDIR}/run1" || exit 1
 exists_ok "flow.cylc"
-if [[ $(readlink "${RND_SUITE_RUNDIR}/run1/flow.cylc") == "${RND_SUITE_RUNDIR}/run1/suite.rc" ]] ; then
-    ok "symlink.suite.rc"
-else
-    fail "symlink.suite.rc"
-fi
+
+TEST_NAME="${TEST_NAME_BASE}-suite.rc-flow.cylc-readlink"
+readlink "flow.cylc" > "${TEST_NAME}.out"
+cmp_ok "${TEST_NAME}.out" <<< "${RND_SUITE_RUNDIR}/run1/suite.rc"
 
 INSTALL_LOG="$(find "${RND_SUITE_RUNDIR}/run1/log/install" -type f -name '*.log')"
 grep_ok "The filename \"suite.rc\" is deprecated in favour of \"flow.cylc\". Symlink created." "${INSTALL_LOG}"

tests/functional/deprecations/01-cylc8-basic/flow.cylc

@@ -34,8 +34,9 @@
 
     initial cycle point = 20150808T00
     final cycle point = 20150808T00
-    [[graph]]
-        P1D = foo => cat & dog
+    [[dependencies]]
+        [[[P1D]]]
+            graph = foo => cat & dog
 
 [runtime]
     [[foo, cat, dog]]

tests/functional/deprecations/01-cylc8-basic/validation.stderr

@@ -30,6 +30,9 @@ WARNING -  * (8.0.0) [runtime][foo, cat, dog][job][execution time limit] -> [run
 WARNING -  * (8.0.0) [runtime][foo, cat, dog][job][submission polling intervals] -> [runtime][foo, cat, dog][submission polling intervals] - value unchanged
 WARNING -  * (8.0.0) [runtime][foo, cat, dog][job][submission retry delays] -> [runtime][foo, cat, dog][submission retry delays] - value unchanged
 WARNING -  * (8.0.0) [cylc] -> [scheduler] - value unchanged
+WARNING - deprecated graph items were automatically upgraded in "suite definition":
+	 * (8.0.0) [scheduling][dependencies][X]graph -> [scheduling][graph]X - for X in:
+	       P1D
 WARNING - * (8.0.0) '[runtime][foo, cat, dog]execution polling intervals' - this setting is deprecated; use 'global.cylc[platforms][<platform name>]execution polling intervals' instead. Currently, this item will override the corresponding item in global.cylc, but support for this will be removed in Cylc 9.
 WARNING - * (8.0.0) '[runtime][foo, cat, dog]submission polling intervals' - this setting is deprecated; use 'global.cylc[platforms][<platform name>]submission polling intervals' instead. Currently, this item will override the corresponding item in global.cylc, but support for this will be removed in Cylc 9.
 WARNING - * (8.0.0) '[runtime][foo, cat, dog]submission retry delays' - this setting is deprecated; use 'global.cylc[platforms][<platform name>]submission retry delays' instead. Currently, this item will override the corresponding item in global.cylc, but support for this will be removed in Cylc 9.