Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Remove the suite.rc flow.cylc symlink #4506

Merged
merged 13 commits into from
Dec 3, 2021
Merged

Remove the suite.rc flow.cylc symlink #4506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9f401fe
Remove the suite.rc flow.cylc symlink
datamel Nov 9, 2021
c14db21
change log
datamel Nov 10, 2021
a4354b6
shellcheck
datamel Nov 15, 2021
d638b3e
Review feedback and add checks to suite.rc and flow.cylc file logic.
datamel Nov 26, 2021
e42f548
Update cylc/flow/workflow_files.py
datamel Nov 26, 2021
0beeac4
Add test and tweak logic to permit symlinks elsewhere for flow.cylc file
datamel Nov 29, 2021
a49283c
Update tests/unit/test_workflow_files.py
datamel Nov 30, 2021
90f2693
dedent test
datamel Nov 30, 2021
8d55298
Merge branch 'remove-suite-flow-symlink' of github.com:datamel/cylc-f…
datamel Nov 30, 2021
97fab79
Fix test
datamel Nov 30, 2021
6e4895c
Update cylc/flow/workflow_files.py
wxtim Dec 2, 2021
5484cb6
Change logic comparing symlinked flow.cylc file.
datamel Dec 2, 2021
46d7737
logic fix
datamel Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ First Release Candidate for Cylc 8.

### Enhancements

[#4506](https://github.com/cylc/cylc-flow/pull/4506) -
Cylc no longer creates a `flow.cylc` symlink to a `suite.rc` file.
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
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.

Expand Down
112 changes: 54 additions & 58 deletions cylc/flow/workflow_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@

if TYPE_CHECKING:
from optparse import Values
from logging import Logger


class KeyType(Enum):
Expand Down Expand Up @@ -545,14 +544,11 @@ def get_contact_file(reg):
get_workflow_srv_dir(reg), WorkflowFiles.Service.CONTACT)


def get_flow_file(reg: str) -> str:
"""Return the path of a workflow's flow.cylc file.

Creates a flow.cylc symlink to suite.rc if only suite.rc exists.
"""
def get_flow_file(reg: str) -> Path:
"""Return the path of a workflow's flow.cylc file."""
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
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved


def get_workflow_source_dir(
Expand Down Expand Up @@ -678,7 +674,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)
Expand Down Expand Up @@ -1297,7 +1293,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(
Expand Down Expand Up @@ -1587,7 +1583,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)
Expand Down Expand Up @@ -1690,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, symlink_suiterc=True, logger=install_log)
)
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)
Expand Down Expand Up @@ -1759,23 +1753,50 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

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

Is this desirable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliver-sanders, I believe you said we should not stop users from manually making a symlink.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we check that the symlink points to the right suite.rc, and raise an error if not?

Copy link
Contributor Author

@datamel datamel Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If there is a flow.cylc there it will use that, regardless of if there is a suite.rc file there. Do we want to forbid users from symlinking other places? I can't think of a reason to forbid, I am probably missing something! I think so long as it is a valid workflow (which will be picked up with other checks) then do we really care how they are sourcing the workflow, it is a bit messy but that is the user's choice?

Copy link
Member

@MetRonnie MetRonnie Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It sounds reasonable to me that users could have a flow.cylc that is a symlink to either its neighbour suite.rc or somewhere else. But if they have a flow.cylc that symlinks to somewhere else and a suite.rc in there, that sounds to me like it should fail in the same way as a normal flow.cylc file and a suite.rc in there.

TLDR:

ok_but_a_bit_dodgy
`-- flow.cylc -> ~/something.cylc

ok
|-- flow.cylc -> suite.rc  # or flow.cylc -> ~/cylc-run/ok/suite.rc?
`-- suite.rc

bad
|-- flow.cylc -> ~/something.cylc
`-- suite.rc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolving this as there is a use case (at the MO) for users to source workflow from outside the run directory.

Copy link
Member

@MetRonnie MetRonnie Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry Mel, not sure if you saw my latest comment above #4506 (comment). I think it is similar to what Oliver said (except he said we don't have to worry about the middle case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't see that, thanks @MetRonnie, mustn't have refreshed the page before posting.

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} "
"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:
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 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():
return True
return False


def detect_flow_exists(
Expand Down Expand Up @@ -1804,55 +1825,30 @@ def detect_flow_exists(
return False


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.
def check_flow_file(path: Union[Path, str]) -> Path:
"""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: Path) -> None:
"""Create workflow service directory"""

workflow_srv_d = rundir.joinpath(WorkflowFiles.Service.DIRNAME)
Expand Down Expand Up @@ -1882,7 +1878,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]]:
Expand Down Expand Up @@ -1964,7 +1960,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
Expand Down
24 changes: 8 additions & 16 deletions tests/functional/cylc-install/00-simple.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#------------------------------------------------------------------------------
# Test workflow installation
. "$(dirname "$0")/test_header"
set_test_number 29
set_test_number 26

create_test_global_config "" "
[install]
Expand Down Expand Up @@ -66,29 +66,21 @@ 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"
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__
MetRonnie marked this conversation as resolved.
Show resolved Hide resolved
# 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

# 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

Expand Down
4 changes: 2 additions & 2 deletions tests/functional/cylc-install/02-failures.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__

Expand Down
24 changes: 1 addition & 23 deletions tests/functional/cylc-reinstall/00-simple.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
18 changes: 1 addition & 17 deletions tests/functional/cylc-reinstall/01-file-transfer.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
4 changes: 2 additions & 2 deletions tests/functional/cylc-reinstall/02-failures.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 1 addition & 18 deletions tests/functional/deprecations/03-suiterc.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,12 @@
# 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"
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
Expand All @@ -44,19 +43,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
Loading