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

Source symlink fix #4338

Merged
merged 4 commits into from
Sep 19, 2021
Merged

Source symlink fix #4338

merged 4 commits into from
Sep 19, 2021

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Aug 2, 2021

These changes close #4322

  • 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).
  • Applied any dependency changes to both setup.py and
    conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@datamel datamel self-assigned this Aug 2, 2021
@datamel datamel added the bug Something is wrong :( label Aug 2, 2021
@datamel datamel added this to the cylc-8.0b3 milestone Aug 2, 2021
@datamel datamel added the small label Aug 2, 2021
Comment on lines 1356 to 1527
raise WorkflowFilesError(
'Workflow run directory contains a broken source symlink: '
f'{source_link}\n'
f'Restore the original source or modify this'
' symlink, then try cylc install again.'
)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like the fact that the symlink is broken shouldn't be the main point, it's that Workflow source dir is not accessible (from the other error message). In fact, I think the other error message can be reused for this?

It makes me wonder if we should have a cli option for cylc install to "re-source" the workflow from the given directory (which would be a follow-up issue).

@MetRonnie
Copy link
Member

Just ran into this bug again, would be good to get it resolved soon

@oliver-sanders oliver-sanders requested review from wxtim and removed request for oliver-sanders September 8, 2021 12:20
@wxtim wxtim requested a review from MetRonnie September 9, 2021 14:48
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks good, tested as working 👍

CHANGES.md Outdated
Comment on lines 171 to 173
[#4319](https://github.com/cylc/cylc-flow/pull/4319) -
Update cylc reinstall to skip cylc dirs work and share #4319

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[#4319](https://github.com/cylc/cylc-flow/pull/4319) -
Update cylc reinstall to skip cylc dirs work and share #4319

Already exists at lines 96-7 above, which I think is the right place for it.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Happy with it to merge.

However, going back to the example I commented earlier (sorry!)

  • Create ~/cylc-src/test/flow.cylc
  • $ cylc install test
  • Delete the ~/cylc-src/test/ dir
  • Create ~/Documents/test/flow.cylc (or wherever, doesn't have to be in Documents).
  • $ cylc install -C ~/Documents/test

The warning message

WorkflowFilesError: Workflow source dir is not accessible: ~/cylc-src/test".
Restore the source or modify the "~/cylc-run/test/_cylc-install/source"
symlink to continue.

might not be so helpful in this case, and I wonder if the Source directory not consistent between runs message should take precedence instead.

Up to you if you want to address this

@hjoliver
Copy link
Member

I'm gonna merge this - thanks @datamel 👍 and post a "small" issue to review the wording of installation-related error messages.

@hjoliver hjoliver merged commit 102cc83 into cylc:master Sep 19, 2021
@datamel datamel deleted the source-sym-fix branch September 1, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FileExistsError during second cylc install with --directory option
3 participants