From 9f401fe3c4c6d64081aefe49f4630b3c138115c5 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 9 Nov 2021 11:14:27 +0000 Subject: [PATCH 01/11] Remove the suite.rc flow.cylc symlink --- CHANGES.md | 3 + cylc/flow/workflow_files.py | 64 ++++++------------- tests/functional/cylc-install/00-simple.t | 17 +---- tests/functional/cylc-install/02-failures.t | 4 +- tests/functional/cylc-reinstall/00-simple.t | 24 +------ .../cylc-reinstall/01-file-transfer.t | 18 +----- tests/functional/cylc-reinstall/02-failures.t | 4 +- tests/functional/deprecations/03-suiterc.t | 18 +----- .../optional-outputs/03-c7backcompat.t | 3 +- tests/unit/test_workflow_files.py | 4 +- 10 files changed, 35 insertions(+), 124 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index d92f219b869..470cb3c7ebe 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -56,6 +56,9 @@ First Release Candidate for Cylc 8. ### Enhancements +[#](https://github.com/cylc/cylc-flow/pull/) - +Cylc no longer creates a `flow.cylc` symlink to a `suite.rc` file. + [#4526](https://github.com/cylc/cylc-flow/pull/4526) - Prevent runN and run\d+ being allowed as installation target names. diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index 9b312247171..cdd09d40dc7 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -84,7 +84,6 @@ if TYPE_CHECKING: from optparse import Values - from logging import Logger class KeyType(Enum): @@ -551,8 +550,8 @@ def get_flow_file(reg: str) -> str: Creates a flow.cylc symlink to suite.rc if only suite.rc exists. """ run_dir = get_workflow_run_dir(reg) - check_flow_file(run_dir, symlink_suiterc=True) - return os.path.join(run_dir, WorkflowFiles.FLOW_FILE) + path = check_flow_file(run_dir) + return path def get_workflow_source_dir( @@ -678,7 +677,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, logger=None) + check_flow_file(source) if not is_installed(get_workflow_run_dir(workflow_name)): symlinks_created = make_localhost_symlinks( get_workflow_run_dir(workflow_name), workflow_name) @@ -1297,7 +1296,7 @@ def _parse_src_reg(reg: Path, cur_dir_only: bool = False) -> Tuple[Path, Path]: )) else: try: - abs_path = check_flow_file(abs_path, logger=None) + abs_path = check_flow_file(abs_path) except WorkflowFilesError: return (run_dir_reg, run_dir_path) LOG.warning(REG_CLASH_MSG.format( @@ -1587,7 +1586,7 @@ def reinstall_workflow(named_run, rundir, source, dry_run=False): reinstall_log.warning( f"An error occurred when copying files from {source} to {rundir}") reinstall_log.warning(f" Error: {stderr}") - check_flow_file(rundir, symlink_suiterc=True, logger=reinstall_log) + check_flow_file(rundir) reinstall_log.info(f'REINSTALLED {named_run} from {source}') print(f'REINSTALLED {named_run} from {source}') close_log(reinstall_log) @@ -1691,7 +1690,7 @@ def install_workflow( install_log.warning(f" Warning: {stderr}") cylc_install = Path(rundir.parent, WorkflowFiles.Install.DIRNAME) check_deprecation( - check_flow_file(rundir, symlink_suiterc=True, logger=install_log) + check_flow_file(rundir) ) if no_run_name: cylc_install = Path(rundir, WorkflowFiles.Install.DIRNAME) @@ -1761,11 +1760,13 @@ def get_run_dir_info( def detect_both_flow_and_suite(path): """Detects if both suite.rc and flow.cylc are in directory. + + Permits flow.cylc to be a symlink. Raises: WorkflowFilesError: If both flow.cylc and suite.rc are in directory """ msg = (f"Both {WorkflowFiles.FLOW_FILE} and {WorkflowFiles.SUITE_RC} " - "files are present in the run directory. Please remove one and" + f"files are present in {path}. Please remove one and" " try again. For more information visit: https://cylc.github.io/" "cylc-doc/latest/html/7-to-8/summary.html#backward-compatibility") if path.resolve().name == WorkflowFiles.SUITE_RC: @@ -1806,53 +1807,30 @@ def detect_flow_exists( def check_flow_file( path: Union[Path, str], - symlink_suiterc: bool = False, - logger: Optional['Logger'] = LOG ) -> Path: - """Raises WorkflowFilesError if no flow file in path sent. + """Checks the path for a suite.rc or flow.cylc file. + + Raises: + WorkflowFilesError + - if no flow file in path sent + - both suite.rc and flow.cylc in path sent. Args: path: Absolute path to check for a flow.cylc and/or suite.rc file. - 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), WorkflowFiles.FLOW_FILE) suite_rc_path = Path(expand_path(path), WorkflowFiles.SUITE_RC) if flow_file_path.is_file(): - if not flow_file_path.is_symlink(): - if not suite_rc_path.is_file(): - return flow_file_path - raise WorkflowFilesError( - f"Both {WorkflowFiles.FLOW_FILE} and " - f"{WorkflowFiles.SUITE_RC} files are present in the source " - "directory. Please remove one and try again. For more " - "information visit: https://cylc.github.io/cylc-doc/latest/" - "html/7-to-8/summary.html#backward-compatibility" - ) - if flow_file_path.resolve() == suite_rc_path.resolve(): - # A symlink that points to existing suite.rc - return flow_file_path - if suite_rc_path.is_file(): - if not symlink_suiterc: - 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(WorkflowFiles.SUITE_RC) - if logger: - logger.warning( - f"Symlink created: " - f"{WorkflowFiles.FLOW_FILE} -> {WorkflowFiles.SUITE_RC}" - ) + detect_both_flow_and_suite(Path(path)) return flow_file_path + if suite_rc_path.is_file(): + return suite_rc_path raise WorkflowFilesError(NO_FLOW_FILE_MSG.format(path)) -def create_workflow_srv_dir(rundir=None, source=None): +def create_workflow_srv_dir(rundir=None): """Create workflow service directory""" workflow_srv_d = rundir.joinpath(WorkflowFiles.Service.DIRNAME) @@ -1882,7 +1860,7 @@ def validate_source_dir(source, workflow_name): raise WorkflowFilesError( f"{workflow_name} installation failed. Source directory " f"should not be in {cylc_run_dir}.") - check_flow_file(source, logger=None) + check_flow_file(source) def parse_cli_sym_dirs(symlink_dirs: str) -> Dict[str, Dict[str, Any]]: @@ -1964,7 +1942,7 @@ def search_install_source_dirs(workflow_name: str) -> Path: "does not contain any paths") for path in search_path: try: - flow_file = check_flow_file(Path(path, workflow_name), logger=None) + flow_file = check_flow_file(Path(path, workflow_name)) return flow_file.parent except WorkflowFilesError: continue diff --git a/tests/functional/cylc-install/00-simple.t b/tests/functional/cylc-install/00-simple.t index ddbf44312da..48e171f3192 100755 --- a/tests/functional/cylc-install/00-simple.t +++ b/tests/functional/cylc-install/00-simple.t @@ -18,7 +18,7 @@ #------------------------------------------------------------------------------ # Test workflow installation . "$(dirname "$0")/test_header" -set_test_number 29 +set_test_number 25 create_test_global_config "" " [install] @@ -66,7 +66,6 @@ purge_rnd_workflow # ----------------------------------------------------------------------------- # Test cylc install succeeds if suite.rc file in source dir -# See also tests/functional/deprecations/03-suiterc.t TEST_NAME="${TEST_NAME_BASE}-suite.rc" make_rnd_workflow rm -f "${RND_WORKFLOW_SOURCE}/flow.cylc" @@ -76,20 +75,6 @@ run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" -C "${RND_ contains_ok "${TEST_NAME}.stdout" <<__OUT__ INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE} __OUT__ -# test symlink not made in source dir -exists_fail "flow.cylc" -# test symlink correctly made in run dir -pushd "${RND_WORKFLOW_RUNDIR}/run1" || exit 1 -exists_ok "flow.cylc" - -TEST_NAME="${TEST_NAME_BASE}-suite.rc-flow.cylc-readlink" -readlink "flow.cylc" > "${TEST_NAME}.out" -cmp_ok "${TEST_NAME}.out" <<< "suite.rc" - -INSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*.log')" -grep_ok "Symlink created: flow.cylc -> suite.rc" "${INSTALL_LOG}" -popd || exit 1 - purge_rnd_workflow # ----------------------------------------------------------------------------- diff --git a/tests/functional/cylc-install/02-failures.t b/tests/functional/cylc-install/02-failures.t index 0887191dede..af6c5ad3877 100644 --- a/tests/functional/cylc-install/02-failures.t +++ b/tests/functional/cylc-install/02-failures.t @@ -64,8 +64,8 @@ TEST_NAME="${TEST_NAME_BASE}-both-suite-and-flow-file" touch "${RND_WORKFLOW_SOURCE}/suite.rc" run_fail "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" -C "${RND_WORKFLOW_SOURCE}" contains_ok "${TEST_NAME}.stderr" <<__ERR__ -WorkflowFilesError: Both flow.cylc and suite.rc files are present in the source \ -directory. Please remove one and try again. For more information visit: \ +WorkflowFilesError: Both flow.cylc and suite.rc files are present in ${RND_WORKFLOW_SOURCE}. \ +Please remove one and try again. For more information visit: \ https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html#backward-compatibility __ERR__ diff --git a/tests/functional/cylc-reinstall/00-simple.t b/tests/functional/cylc-reinstall/00-simple.t index f945aad238e..b951e7b4f9e 100644 --- a/tests/functional/cylc-reinstall/00-simple.t +++ b/tests/functional/cylc-reinstall/00-simple.t @@ -18,7 +18,7 @@ #------------------------------------------------------------------------------ # Test workflow re-installation . "$(dirname "$0")/test_header" -set_test_number 28 +set_test_number 21 # Test basic cylc reinstall, named run given TEST_NAME="${TEST_NAME_BASE}-basic-named-run" @@ -60,30 +60,8 @@ run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" -C "${RND_ cmp_ok "${TEST_NAME}.stdout" <<__OUT__ INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE} __OUT__ -# test symlink not made in source dir -exists_fail "flow.cylc" -# test symlink correctly made in run dir -pushd "${RND_WORKFLOW_RUNDIR}/run1" || exit 1 -exists_ok "flow.cylc" -if [[ $(readlink "${RND_WORKFLOW_RUNDIR}/run1/flow.cylc") == "suite.rc" ]] ; then - ok "symlink.suite.rc" -else - fail "symlink.suite.rc" -fi -INSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*.log')" -grep_ok "Symlink created: flow.cylc -> suite.rc" "${INSTALL_LOG}" -rm -rf flow.cylc run_ok "${TEST_NAME}-reinstall" cylc reinstall "${RND_WORKFLOW_NAME}/run1" -exists_ok "${RND_WORKFLOW_RUNDIR}/run1/flow.cylc" -if [[ $(readlink "${RND_WORKFLOW_RUNDIR}/run1/flow.cylc") == "suite.rc" ]] ; then - ok "symlink.suite.rc" -else - fail "symlink.suite.rc" -fi -REINSTALL_LOG="$(find "${RND_WORKFLOW_RUNDIR}/run1/log/install" -type f -name '*reinstall.log')" -grep_ok "Symlink created: flow.cylc -> suite.rc" "${INSTALL_LOG}" -popd || exit 1 purge_rnd_workflow # Test cylc reinstall from within rundir, no args given diff --git a/tests/functional/cylc-reinstall/01-file-transfer.t b/tests/functional/cylc-reinstall/01-file-transfer.t index 146c44b5735..dc79cf4c31c 100644 --- a/tests/functional/cylc-reinstall/01-file-transfer.t +++ b/tests/functional/cylc-reinstall/01-file-transfer.t @@ -21,7 +21,7 @@ if ! command -v 'tree' >'/dev/null'; then skip_all '"tree" command not available' fi -set_test_number 14 +set_test_number 9 # Test cylc install copies files to run dir successfully. TEST_NAME="${TEST_NAME_BASE}-basic" @@ -88,20 +88,4 @@ cmp_ok 'after-reinstall-run1-tree.out' '01-file-transfer-basic-tree.out' popd || exit 1 purge_rnd_workflow - -# Test cylc reinstall creates flow.cylc given suite.rc. -TEST_NAME="${TEST_NAME_BASE}-reinstall-creates-flow.cylc-given-suite.rc" -make_rnd_workflow -pushd "${RND_WORKFLOW_SOURCE}" || exit 1 -rm -f flow.cylc -touch suite.rc -run_ok "${TEST_NAME}-install" cylc install --no-run-name --flow-name="${RND_WORKFLOW_NAME}" -rm -f "${RND_WORKFLOW_RUNDIR}/flow.cylc" -exists_fail "${RND_WORKFLOW_RUNDIR}/flow.cylc" -run_ok "${TEST_NAME}-reinstall" cylc reinstall "${RND_WORKFLOW_NAME}" -exists_ok "${RND_WORKFLOW_RUNDIR}/flow.cylc" -exists_fail "${RND_WORKFLOW_SOURCE}/flow.cylc" -popd || exit 1 -purge_rnd_workflow - exit diff --git a/tests/functional/cylc-reinstall/02-failures.t b/tests/functional/cylc-reinstall/02-failures.t index e13d2486ecc..63f3523b4b4 100644 --- a/tests/functional/cylc-reinstall/02-failures.t +++ b/tests/functional/cylc-reinstall/02-failures.t @@ -65,8 +65,8 @@ run_ok "${TEST_NAME}-install" cylc install -C "${RND_WORKFLOW_SOURCE}" --flow-na touch "${RND_WORKFLOW_SOURCE}/suite.rc" run_fail "${TEST_NAME}" cylc reinstall "${RND_WORKFLOW_NAME}" cmp_ok "${TEST_NAME}.stderr" <<__ERR__ -WorkflowFilesError: Both flow.cylc and suite.rc files are present in the source \ -directory. Please remove one and try again. For more information visit: \ +WorkflowFilesError: Both flow.cylc and suite.rc files are present in ${RND_WORKFLOW_SOURCE}. \ +Please remove one and try again. For more information visit: \ https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html#backward-compatibility __ERR__ purge_rnd_workflow diff --git a/tests/functional/deprecations/03-suiterc.t b/tests/functional/deprecations/03-suiterc.t index f8248591fef..0faa15e1b93 100644 --- a/tests/functional/deprecations/03-suiterc.t +++ b/tests/functional/deprecations/03-suiterc.t @@ -18,7 +18,7 @@ # Test backwards compatibility for suite.rc files . "$(dirname "$0")/test_header" -set_test_number 5 +set_test_number 2 init_suiterc() { local TEST_NAME="$1" @@ -44,19 +44,3 @@ print(SUITERC_DEPR_MSG)') TEST_NAME="${TEST_NAME_BASE}-validate" run_ok "${TEST_NAME}" cylc validate . grep_ok "$MSG" "${TEST_NAME_BASE}-validate.stderr" - -# Test install suite.rc -# See also tests/functional/cylc-install/00-simple.t -TEST_NAME="${TEST_NAME_BASE}-install-after-validate" -run_ok "${TEST_NAME}" cylc install --flow-name="${WORKFLOW_NAME}" --no-run-name - -cd "${WORKFLOW_RUN_DIR}" || exit 1 -exists_ok "flow.cylc" - -TEST_NAME="flow.cylc-readlink" -readlink "flow.cylc" > "${TEST_NAME}.out" -cmp_ok "${TEST_NAME}.out" <<< "suite.rc" - -cd "${TEST_DIR}" || exit 1 -rm -rf "${TEST_DIR:?}/${WORKFLOW_NAME}/" -purge diff --git a/tests/functional/optional-outputs/03-c7backcompat.t b/tests/functional/optional-outputs/03-c7backcompat.t index 3da19fa7d10..83f2119694b 100644 --- a/tests/functional/optional-outputs/03-c7backcompat.t +++ b/tests/functional/optional-outputs/03-c7backcompat.t @@ -35,8 +35,7 @@ grep_ok "${ERR}" "${TEST_NAME}.stderr" # Rename config to "suite.rc" mv "${WORKFLOW_RUN_DIR}/flow.cylc" "${WORKFLOW_RUN_DIR}/suite.rc" -ln -s "${WORKFLOW_RUN_DIR}/suite.rc" "${WORKFLOW_RUN_DIR}/flow.cylc" - +-ln -s "${WORKFLOW_RUN_DIR}/suite.rc" "${WORKFLOW_RUN_DIR}/flow.cylc" # It should now validate, with a deprecation message TEST_NAME="${TEST_NAME_BASE}-validate_as_c7" run_ok "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 4849f8f24f6..78a3eea1b0d 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1805,8 +1805,8 @@ def test_check_flow_file( with pytest.raises(WorkflowFilesError) as exc: check_flow_file(tmp_path) assert str(exc.value) == ( - "Both flow.cylc and suite.rc files are present in the " - "source directory. Please remove one and try again. " + "Both flow.cylc and suite.rc files are present in " + f"{tmp_path}. Please remove one and try again. " "For more information visit: " "https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html" "#backward-compatibility" From c14db21b94d543fc20822fc8698700390a9aadf1 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Wed, 10 Nov 2021 18:19:52 +0000 Subject: [PATCH 02/11] change log --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 470cb3c7ebe..ebbf0be9e72 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -56,7 +56,7 @@ First Release Candidate for Cylc 8. ### Enhancements -[#](https://github.com/cylc/cylc-flow/pull/) - +[#4506](https://github.com/cylc/cylc-flow/pull/4506) - Cylc no longer creates a `flow.cylc` symlink to a `suite.rc` file. [#4526](https://github.com/cylc/cylc-flow/pull/4526) - Prevent runN and run\d+ From a4354b6f5b523a00df9235ec5251c230af934023 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Mon, 15 Nov 2021 10:17:06 +0000 Subject: [PATCH 03/11] shellcheck --- tests/functional/deprecations/03-suiterc.t | 1 - tests/functional/optional-outputs/03-c7backcompat.t | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/functional/deprecations/03-suiterc.t b/tests/functional/deprecations/03-suiterc.t index 0faa15e1b93..7fdfa6f6f1e 100644 --- a/tests/functional/deprecations/03-suiterc.t +++ b/tests/functional/deprecations/03-suiterc.t @@ -24,7 +24,6 @@ init_suiterc() { local TEST_NAME="$1" local FLOW_CONFIG="${2:--}" WORKFLOW_NAME="${CYLC_TEST_REG_BASE}/${TEST_SOURCE_DIR_BASE}/${TEST_NAME}" - WORKFLOW_RUN_DIR="$RUN_DIR/$WORKFLOW_NAME" mkdir -p "${TEST_DIR}/${WORKFLOW_NAME}/" cat "${FLOW_CONFIG}" >"${TEST_DIR}/${WORKFLOW_NAME}/suite.rc" cd "${TEST_DIR}/${WORKFLOW_NAME}" || exit diff --git a/tests/functional/optional-outputs/03-c7backcompat.t b/tests/functional/optional-outputs/03-c7backcompat.t index 83f2119694b..432430e09bf 100644 --- a/tests/functional/optional-outputs/03-c7backcompat.t +++ b/tests/functional/optional-outputs/03-c7backcompat.t @@ -35,7 +35,7 @@ grep_ok "${ERR}" "${TEST_NAME}.stderr" # Rename config to "suite.rc" mv "${WORKFLOW_RUN_DIR}/flow.cylc" "${WORKFLOW_RUN_DIR}/suite.rc" --ln -s "${WORKFLOW_RUN_DIR}/suite.rc" "${WORKFLOW_RUN_DIR}/flow.cylc" +ln -s "${WORKFLOW_RUN_DIR}/suite.rc" "${WORKFLOW_RUN_DIR}/flow.cylc" # It should now validate, with a deprecation message TEST_NAME="${TEST_NAME_BASE}-validate_as_c7" run_ok "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" From d638b3ecc755c24f3e6da6e08a69213e866a44a5 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Fri, 26 Nov 2021 12:02:00 +0000 Subject: [PATCH 04/11] Review feedback and add checks to suite.rc and flow.cylc file logic. --- CHANGES.md | 1 + cylc/flow/workflow_files.py | 33 ++++++++++++++++---- tests/unit/test_workflow_files.py | 50 ++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ebbf0be9e72..ce66f02b434 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -58,6 +58,7 @@ First Release Candidate for Cylc 8. [#4506](https://github.com/cylc/cylc-flow/pull/4506) - Cylc no longer creates a `flow.cylc` symlink to a `suite.rc` file. +This only affects you if you have used a prior Cylc 8 pre-release. [#4526](https://github.com/cylc/cylc-flow/pull/4526) - Prevent runN and run\d+ being allowed as installation target names. diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index cdd09d40dc7..a8fec4eccae 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -544,7 +544,7 @@ def get_contact_file(reg): get_workflow_srv_dir(reg), WorkflowFiles.Service.CONTACT) -def get_flow_file(reg: str) -> str: +def get_flow_file(reg: str) -> Path: """Return the path of a workflow's flow.cylc file. Creates a flow.cylc symlink to suite.rc if only suite.rc exists. @@ -1758,25 +1758,46 @@ def get_run_dir_info( return relink, run_num, rundir -def detect_both_flow_and_suite(path): +def detect_both_flow_and_suite(path: Path) -> None: """Detects if both suite.rc and flow.cylc are in directory. Permits flow.cylc to be a symlink. + Return true if present, raises error if flow.cylc path sent is a forbidden + symlink. Raises: WorkflowFilesError: If both flow.cylc and suite.rc are in directory """ + flow_cylc = None msg = (f"Both {WorkflowFiles.FLOW_FILE} and {WorkflowFiles.SUITE_RC} " f"files are present in {path}. Please remove one and" " try again. For more information visit: https://cylc.github.io/" "cylc-doc/latest/html/7-to-8/summary.html#backward-compatibility") if path.resolve().name == WorkflowFiles.SUITE_RC: flow_cylc = path.parent / WorkflowFiles.FLOW_FILE - if flow_cylc.is_file() and not flow_cylc.is_symlink(): - raise WorkflowFilesError(msg) elif (path / WorkflowFiles.SUITE_RC).is_file(): flow_cylc = path / WorkflowFiles.FLOW_FILE - if flow_cylc.is_file() and not flow_cylc.is_symlink(): - raise WorkflowFilesError(msg) + if flow_cylc and flow_cylc.is_file() and is_forbidden(flow_cylc): + raise WorkflowFilesError(msg) + + +def is_forbidden(flow_file: Path) -> bool: + """Returns True for a forbidden file structure scenario. + + Forbidden criteria: + A symlink elsewhere on file system but suite.rc also exists in the + directory. + flow.cylc and suite.rc in same directory but no symlink + Args: + flow_file : Absolute Path to the flow.cylc file + """ + if not flow_file.is_symlink(): + if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists(): + return True + return False + link = flow_file.resolve() + if Path(link).parent == flow_file.parent: # link points within dir + return False + return True def detect_flow_exists( diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 78a3eea1b0d..1ff96f17d0e 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -52,6 +52,7 @@ glob_in_run_dir, infer_latest_run, init_clean, + is_forbidden, is_installed, parse_cli_sym_dirs, parse_reg, @@ -1817,10 +1818,12 @@ def test_check_flow_file( def test_detect_both_flow_and_suite(tmp_path): - """Test flow.cylc and suite.rc together in dir raises error.""" + """Test flow.cylc and suite.rc (as files) together in dir raises error.""" tmp_path.joinpath(WorkflowFiles.FLOW_FILE).touch() tmp_path.joinpath(WorkflowFiles.SUITE_RC).touch() + forbidden = is_forbidden(tmp_path / WorkflowFiles.FLOW_FILE) + assert forbidden is True with pytest.raises(WorkflowFilesError) as exc: detect_both_flow_and_suite(tmp_path) assert str(exc.value) == ( @@ -1915,6 +1918,51 @@ def test_check_flow_file_symlink( assert log_msg in caplog.messages +def test_detect_both_flow_and_suite_symlinked(tmp_path): + """Test flow.cylc symlinked to suite.rc together in dir is permitted.""" + Path(tmp_path) + (tmp_path / WorkflowFiles.SUITE_RC).touch() + flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE) + flow_file.symlink_to(WorkflowFiles.SUITE_RC) + try: + detect_both_flow_and_suite(tmp_path) + except WorkflowFilesError: + pytest.fail("Unexpected WorkflowFilesError") + + +def test_flow_symlinked_elsewhere_and_suite_present(tmp_path): + """flow.cylc symlinked to suite.rc elsewhere, and suite.rc in dir raises""" + tmp_path = Path(tmp_path) + tmp_path.joinpath('some_other_dir').mkdir(exist_ok=True) + suite_file = tmp_path.joinpath('some_other_dir', WorkflowFiles.SUITE_RC) + suite_file.touch() + run_dir = tmp_path.joinpath('run_dir') + run_dir.mkdir(exist_ok=True) + flow_file = (run_dir / WorkflowFiles.FLOW_FILE) + flow_file.symlink_to(suite_file) + (run_dir / WorkflowFiles.SUITE_RC).touch() + forbidden = is_forbidden(flow_file) + assert forbidden is True + with pytest.raises(WorkflowFilesError) as exc: + detect_both_flow_and_suite(run_dir) + assert str(exc.value) == ( + "Both flow.cylc and suite.rc files are present in the " + "source directory. Please remove one and try again. " + "For more information visit: " + "https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html" + "#backward-compatibility" + ) + + +def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path): + """Test sending a non symlink path is not marked as forbidden""" + Path(tmp_path) + flow_file = (tmp_path / WorkflowFiles.FLOW_FILE) + flow_file.touch() + forbidden = is_forbidden(Path(flow_file)) + assert forbidden is False + + @pytest.mark.parametrize( 'symlink_dirs, err_msg, expected', [ From e42f548440f668d03258efbb0d1a5381c84ac2a7 Mon Sep 17 00:00:00 2001 From: Melanie Hall <37735232+datamel@users.noreply.github.com> Date: Fri, 26 Nov 2021 13:50:06 +0000 Subject: [PATCH 05/11] Update cylc/flow/workflow_files.py Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- cylc/flow/workflow_files.py | 7 ++----- tests/unit/test_workflow_files.py | 9 ++------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index a8fec4eccae..ae4e8e32e85 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -545,10 +545,7 @@ def get_contact_file(reg): def get_flow_file(reg: str) -> Path: - """Return the path of a workflow's flow.cylc file. - - Creates a flow.cylc symlink to suite.rc if only suite.rc exists. - """ + """Return the path of a workflow's flow.cylc file.""" run_dir = get_workflow_run_dir(reg) path = check_flow_file(run_dir) return path @@ -1851,7 +1848,7 @@ def check_flow_file( raise WorkflowFilesError(NO_FLOW_FILE_MSG.format(path)) -def create_workflow_srv_dir(rundir=None): +def create_workflow_srv_dir(rundir: Path) -> None: """Create workflow service directory""" workflow_srv_d = rundir.joinpath(WorkflowFiles.Service.DIRNAME) diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 1ff96f17d0e..5473bbc0fb0 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1920,19 +1920,14 @@ def test_check_flow_file_symlink( def test_detect_both_flow_and_suite_symlinked(tmp_path): """Test flow.cylc symlinked to suite.rc together in dir is permitted.""" - Path(tmp_path) (tmp_path / WorkflowFiles.SUITE_RC).touch() flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE) flow_file.symlink_to(WorkflowFiles.SUITE_RC) - try: - detect_both_flow_and_suite(tmp_path) - except WorkflowFilesError: - pytest.fail("Unexpected WorkflowFilesError") + detect_both_flow_and_suite(tmp_path) -def test_flow_symlinked_elsewhere_and_suite_present(tmp_path): +def test_flow_symlinked_elsewhere_and_suite_present(tmp_path: Path): """flow.cylc symlinked to suite.rc elsewhere, and suite.rc in dir raises""" - tmp_path = Path(tmp_path) tmp_path.joinpath('some_other_dir').mkdir(exist_ok=True) suite_file = tmp_path.joinpath('some_other_dir', WorkflowFiles.SUITE_RC) suite_file.touch() From 0beeac48e14696771c6c7407c5a15546e5d34c59 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Mon, 29 Nov 2021 11:49:49 +0000 Subject: [PATCH 06/11] Add test and tweak logic to permit symlinks elsewhere for flow.cylc file add test for deprecation message cylc install --- cylc/flow/workflow_files.py | 16 ++--- tests/functional/cylc-install/00-simple.t | 11 ++- tests/unit/test_workflow_files.py | 86 +---------------------- 3 files changed, 19 insertions(+), 94 deletions(-) diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index ae4e8e32e85..b5e92959b93 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -1686,9 +1686,7 @@ def install_workflow( f"An error occurred when copying files from {source} to {rundir}") install_log.warning(f" Warning: {stderr}") cylc_install = Path(rundir.parent, WorkflowFiles.Install.DIRNAME) - check_deprecation( - check_flow_file(rundir) - ) + check_deprecation(check_flow_file(rundir)) if no_run_name: cylc_install = Path(rundir, WorkflowFiles.Install.DIRNAME) source_link = cylc_install.joinpath(WorkflowFiles.Install.SOURCE) @@ -1792,9 +1790,13 @@ def is_forbidden(flow_file: Path) -> bool: return True return False link = flow_file.resolve() - if Path(link).parent == flow_file.parent: # link points within dir + if link.parent == flow_file.parent: + # link points within dir (permitted) return False - return True + # link points elsewhere, check that suite.rc does not also exist in dir + if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists(): + return True + return False def detect_flow_exists( @@ -1823,9 +1825,7 @@ def detect_flow_exists( return False -def check_flow_file( - path: Union[Path, str], -) -> Path: +def check_flow_file(path: Union[Path, str]) -> Path: """Checks the path for a suite.rc or flow.cylc file. Raises: diff --git a/tests/functional/cylc-install/00-simple.t b/tests/functional/cylc-install/00-simple.t index 48e171f3192..be839f48c1d 100755 --- a/tests/functional/cylc-install/00-simple.t +++ b/tests/functional/cylc-install/00-simple.t @@ -18,7 +18,7 @@ #------------------------------------------------------------------------------ # Test workflow installation . "$(dirname "$0")/test_header" -set_test_number 25 +set_test_number 26 create_test_global_config "" " [install] @@ -71,10 +71,17 @@ make_rnd_workflow rm -f "${RND_WORKFLOW_SOURCE}/flow.cylc" touch "${RND_WORKFLOW_SOURCE}/suite.rc" run_ok "${TEST_NAME}" cylc install --flow-name="${RND_WORKFLOW_NAME}" -C "${RND_WORKFLOW_SOURCE}" - contains_ok "${TEST_NAME}.stdout" <<__OUT__ INSTALLED $RND_WORKFLOW_NAME/run1 from ${RND_WORKFLOW_SOURCE} __OUT__ + +# Test deprecation message is displayed on installing a suite.rc file +MSG=$(python -c 'from cylc.flow.workflow_files import SUITERC_DEPR_MSG; +print(SUITERC_DEPR_MSG)') + +grep_ok "$MSG" "${TEST_NAME}.stderr" + + purge_rnd_workflow # ----------------------------------------------------------------------------- diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 5473bbc0fb0..14ebe659bc1 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1835,89 +1835,6 @@ def test_detect_both_flow_and_suite(tmp_path): ) -@pytest.mark.parametrize( - 'flow_file_target, suiterc_exists, err, expected_file', - [ - pytest.param( - WorkflowFiles.SUITE_RC, True, None, WorkflowFiles.FLOW_FILE, - id="flow.cylc symlinked to suite.rc" - ), - pytest.param( - WorkflowFiles.SUITE_RC, False, WorkflowFilesError, None, - id="flow.cylc symlinked to non-existent suite.rc" - ), - pytest.param( - 'other-path', True, None, WorkflowFiles.SUITE_RC, - id="flow.cylc symlinked to other file, suite.rc exists" - ), - pytest.param( - 'other-path', False, WorkflowFilesError, None, - id="flow.cylc symlinked to other file, no suite.rc" - ), - pytest.param( - None, True, None, WorkflowFiles.SUITE_RC, - id="No flow.cylc, suite.rc exists" - ), - pytest.param( - None, False, WorkflowFilesError, None, - id="No flow.cylc, no suite.rc" - ), - ] -) -@pytest.mark.parametrize( - 'symlink_suiterc_arg', - [pytest.param(False, id="symlink_suiterc=False "), - pytest.param(True, id="symlink_suiterc=True ")] -) -def test_check_flow_file_symlink( - flow_file_target: Optional[str], - suiterc_exists: bool, - err: Optional[Type[Exception]], - expected_file: Optional[str], - symlink_suiterc_arg: bool, - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """Test check_flow_file() when flow.cylc is a symlink or doesn't exist. - - Params: - flow_file_target: Relative path of the flow.cylc symlink's target, or - None if the symlink doesn't exist. - suiterc_exists: Whether there is a suite.rc file in the dir. - err: Type of exception if expected to get raised. - expected_file: Which file's path should get returned, when - symlink_suiterc_arg is FALSE (otherwise it will always be - flow.cylc, assuming no exception occurred). - symlink_suiterc_arg: Value of the symlink_suiterc arg passed to - check_flow_file(). - """ - flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE) - suiterc = tmp_path.joinpath(WorkflowFiles.SUITE_RC) - tmp_path.joinpath('other-path').touch() - if suiterc_exists: - suiterc.touch() - if flow_file_target: - flow_file.symlink_to(flow_file_target) - - caplog.set_level(logging.INFO, CYLC_LOG) - log_msg = "" - - if err: - with pytest.raises(err): - check_flow_file(tmp_path, symlink_suiterc_arg) - else: - assert expected_file is not None # otherwise test is wrong - result = check_flow_file(tmp_path, symlink_suiterc_arg) - if symlink_suiterc_arg is True: - assert flow_file.samefile(suiterc) - expected_file = WorkflowFiles.FLOW_FILE - if flow_file_target != WorkflowFiles.SUITE_RC: - log_msg = "Symlink created: flow.cylc -> suite.rc" - assert result == tmp_path.joinpath(expected_file) - - if log_msg: - assert log_msg in caplog.messages - - def test_detect_both_flow_and_suite_symlinked(tmp_path): """Test flow.cylc symlinked to suite.rc together in dir is permitted.""" (tmp_path / WorkflowFiles.SUITE_RC).touch() @@ -1935,6 +1852,8 @@ def test_flow_symlinked_elsewhere_and_suite_present(tmp_path: Path): run_dir.mkdir(exist_ok=True) flow_file = (run_dir / WorkflowFiles.FLOW_FILE) flow_file.symlink_to(suite_file) + forbidden_external = is_forbidden(flow_file) + assert forbidden_external is False (run_dir / WorkflowFiles.SUITE_RC).touch() forbidden = is_forbidden(flow_file) assert forbidden is True @@ -1951,7 +1870,6 @@ def test_flow_symlinked_elsewhere_and_suite_present(tmp_path: Path): def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path): """Test sending a non symlink path is not marked as forbidden""" - Path(tmp_path) flow_file = (tmp_path / WorkflowFiles.FLOW_FILE) flow_file.touch() forbidden = is_forbidden(Path(flow_file)) From a49283c68af4cd2f3f02f8eba24a71e53f46ba45 Mon Sep 17 00:00:00 2001 From: Melanie Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 30 Nov 2021 09:53:43 +0000 Subject: [PATCH 07/11] Update tests/unit/test_workflow_files.py Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- tests/unit/test_workflow_files.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 14ebe659bc1..8bd87fcb41f 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1859,13 +1859,10 @@ def test_flow_symlinked_elsewhere_and_suite_present(tmp_path: Path): assert forbidden is True with pytest.raises(WorkflowFilesError) as exc: detect_both_flow_and_suite(run_dir) - assert str(exc.value) == ( - "Both flow.cylc and suite.rc files are present in the " - "source directory. Please remove one and try again. " - "For more information visit: " - "https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html" - "#backward-compatibility" - ) + assert str(exc.value).startswith( + "Both flow.cylc and suite.rc files are present in the " + "source directory. Please remove one and try again." + ) def test_is_forbidden_symlink_returns_false_for_non_symlink(tmp_path): From 90f2693bb61eb1d51737d00b0edbc6f8b3e03c2f Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 30 Nov 2021 12:54:48 +0000 Subject: [PATCH 08/11] dedent test --- tests/unit/test_workflow_files.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index 14ebe659bc1..db4c8719a0f 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1826,13 +1826,12 @@ def test_detect_both_flow_and_suite(tmp_path): assert forbidden is True with pytest.raises(WorkflowFilesError) as exc: detect_both_flow_and_suite(tmp_path) - assert str(exc.value) == ( - "Both flow.cylc and suite.rc files are present in the " - "source directory. Please remove one and try again. " - "For more information visit: " - "https://cylc.github.io/cylc-doc/latest/html/7-to-8/summary.html" - "#backward-compatibility" - ) + assert str(exc.value) == ( + f"Both flow.cylc and suite.rc files are present in {tmp_path}. Please " + "remove one and try again. For more information visit: " + "https://cylc.github.io/cylc-doc/latest/html/7-to-8/" + "summary.html#backward-compatibility" + ) def test_detect_both_flow_and_suite_symlinked(tmp_path): From 97fab793f78055b58dcf36e3122e25c93e05b85f Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 30 Nov 2021 13:18:10 +0000 Subject: [PATCH 09/11] Fix test --- tests/unit/test_workflow_files.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_workflow_files.py b/tests/unit/test_workflow_files.py index f0936fe53f1..cdcc5e32fb1 100644 --- a/tests/unit/test_workflow_files.py +++ b/tests/unit/test_workflow_files.py @@ -1859,8 +1859,8 @@ def test_flow_symlinked_elsewhere_and_suite_present(tmp_path: Path): with pytest.raises(WorkflowFilesError) as exc: detect_both_flow_and_suite(run_dir) assert str(exc.value).startswith( - "Both flow.cylc and suite.rc files are present in the " - "source directory. Please remove one and try again." + "Both flow.cylc and suite.rc files are present in " + f"{run_dir}. Please remove one and try again." ) From 6e4895c225b6f60f7805012a82ff12ee07712477 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 2 Dec 2021 09:53:15 +0000 Subject: [PATCH 10/11] Update cylc/flow/workflow_files.py Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- cylc/flow/workflow_files.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index b5e92959b93..0a831a21a02 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -1790,13 +1790,10 @@ def is_forbidden(flow_file: Path) -> bool: return True return False link = flow_file.resolve() - if link.parent == flow_file.parent: - # link points within dir (permitted) - return False - # link points elsewhere, check that suite.rc does not also exist in dir - if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists(): - return True - return False + suite_rc = flow_file.parent / WorkflowFiles.SUITE_RC + if suite_rc.exists(): + return link != suite_rc + return True def detect_flow_exists( From 5484cb6aa4e01fd922ebf2aafa921f870f115fb3 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Thu, 2 Dec 2021 11:16:11 +0000 Subject: [PATCH 11/11] Change logic comparing symlinked flow.cylc file. --- cylc/flow/workflow_files.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cylc/flow/workflow_files.py b/cylc/flow/workflow_files.py index b5e92959b93..a31b91ee7b4 100644 --- a/cylc/flow/workflow_files.py +++ b/cylc/flow/workflow_files.py @@ -1790,8 +1790,8 @@ def is_forbidden(flow_file: Path) -> bool: return True return False link = flow_file.resolve() - if link.parent == flow_file.parent: - # link points within dir (permitted) + if link == flow_file.parent.resolve() / WorkflowFiles.SUITE_RC: + # link points within dir to suite.rc (permitted) return False # link points elsewhere, check that suite.rc does not also exist in dir if flow_file.parent.joinpath(WorkflowFiles.SUITE_RC).exists():