Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

cylc clean: fix bug where --rm share would not clean share/cycle symlink first #6364

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Sep 10, 2024

Fixes a bug possibly (?) seen before by some users at the Met Office, but not tracked by an issue.

If share/cycle is a symlink directory, it must also be cleaned first when only specifying --rm share.

Reproduceable example

# global.cylc
[install]
    [[symlink dirs]]
        [[[localhost]]]
            share = $HOME/sym-other
            share/cycle = $HOME/sym-cycle
$ cylc play <workflow>
$ cylc stop <workflow>
$ cylc clean <workflow> --rm share
# Restart:
$ cylc play <workflow>
WorkflowFilesError: Symlink dir target already exists: (~/cylc-run/<workflow>/share/cycle ->) ~/sym-cycle/cylc-run/<workflow>/share/cycle
Tip: in future, use 'cylc clean' instead of manually deleting workflow run dirs.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • Tests are included
  • Changelog entry included if this is a change that can affect users
  • No docs needed
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the 8.3.4 milestone Sep 10, 2024
@MetRonnie MetRonnie self-assigned this Sep 10, 2024
@oliver-sanders
Copy link
Member

Fixes a bug possibly (?) seen before by some users at the Met Office, but not tracked by an issue.

If it only applies to the --rm option, then I don't think this has been reported. There was an unconfirmed report of cylc clean (no opts) leaving files behind, however, the evidence was destroyed so it couldn't be verified.

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 11, 2024

$ cylc play
WorkflowFilesError: Symlink dir target already exists: (~/cylc-run//share/cycle ->) ~/sym-cycle/cylc-run//share/cycle
Tip: in future, use 'cylc clean' instead of manually deleting workflow run dirs.

I actually don't get this traceback, however, I can confirm the share/cycle directory was left behind before but isn't after.

for symlink_dir in abs_symlink_dirs:
# Note: must clean e.g. share/cycle/ before share/ if the former
# is a symlink even if only the latter was specified.
if any(is_relative_to(symlink_dir, path) for path in matches):
Copy link
Member Author

Choose a reason for hiding this comment

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

@oliver-sanders RE: the traceback you were getting, can you try this in your testing?

Suggested change
if any(is_relative_to(symlink_dir, path) for path in matches):
if (
any(is_relative_to(symlink_dir, path) for path in matches)
and symlink_dir.is_symlink()
):

Copy link
Member

Choose a reason for hiding this comment

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

Seems to do the trick!

@oliver-sanders oliver-sanders modified the milestones: 8.3.4, 8.3.5 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants