Fix reinstall not detecting changed permissions by itself

changes.d/6658.fix.md

@@ -0,0 +1 @@
+Fixed `cylc reinstall` not picking up file permissions changes.

cylc/flow/install.py

@@ -213,19 +213,12 @@ def reinstall_workflow(
         dry_run=dry_run,
     )
 
-    # Add '+++' to -out-format to mark lines passed through formatter.
-    rsync_cmd.append('--out-format=+++%o %n%L+++')
-
     # Run rsync command:
     reinstall_log.info(cli_format(rsync_cmd))
     LOG.debug(cli_format(rsync_cmd))
     proc = Popen(rsync_cmd, stdout=PIPE, stderr=PIPE, text=True)  # nosec
     # * command is constructed via internal interface
-    stdout, stderr = proc.communicate()
-
-    # Strip unwanted output.
-    stdout = ('\n'.join(re.findall(r'\+\+\+(.*)\+\+\+', stdout))).strip()
-    stderr = stderr.strip()
+    stdout, stderr = (i.strip() for i in proc.communicate())
 
     if proc.returncode != 0:
         raise WorkflowFilesError(

cylc/flow/remote.py

@@ -180,9 +180,14 @@ def get_includes_to_rsync(rsync_includes=None):
 DEFAULT_RSYNC_OPTS = [
     '-a',
     '--checksum',
-    '--out-format=%o %n%L',
+    '--out-format=%i %o %n%L',  # see comment below
     '--no-t'
 ]
+# %o: the operation (send or del.)
+# %i: itemized changes (needed for rsync to report files with
+#     changed permissions)
+# %n: filename
+# %L: "-> symlink_target" if applicable
 
 DEFAULT_INCLUDES = [
     '/ana/***',  # Rose ana analysis modules

cylc/flow/scripts/reinstall.py

@@ -68,33 +68,43 @@
   "cylc install" even if present in the source directory.
 """
 
+from functools import partial
 from pathlib import Path
+import re
 import sys
-from typing import Optional, TYPE_CHECKING, List, Callable
-from functools import partial
+from typing import (
+    TYPE_CHECKING,
+    Callable,
+    List,
+    Optional,
+)
 
 from ansimarkup import parse as cparse
 
 from cylc.flow.exceptions import (
     ServiceFileError,
     WorkflowFilesError,
 )
-from cylc.flow.install import (
-    reinstall_workflow,
-)
+import cylc.flow.flags
+from cylc.flow.install import reinstall_workflow
 from cylc.flow.network.multi import call_multi
 from cylc.flow.option_parsers import (
+    ID_MULTI_ARG_DOC,
     CylcOptionParser as COP,
     OptionSettings,
-    ID_MULTI_ARG_DOC
 )
 from cylc.flow.pathutil import get_workflow_run_dir
 from cylc.flow.plugins import run_plugins_async
+from cylc.flow.terminal import (
+    DIM,
+    cli_function,
+    is_terminal,
+)
 from cylc.flow.workflow_files import (
     get_workflow_source_dir,
     load_contact_file,
 )
-from cylc.flow.terminal import cli_function, DIM, is_terminal
+
 
 if TYPE_CHECKING:
     from optparse import Values
@@ -269,9 +279,8 @@ async def reinstall(
         update anything, else returns True.
 
     """
-    # run pre_configure plugins
     if not dry_run:
-        # don't run plugins in dry-mode
+        # run pre_configure plugins
         async for _entry_point, _plugin_result in run_plugins_async(
             'cylc.pre_configure',
             srcdir=src_dir,
@@ -287,18 +296,17 @@ async def reinstall(
         dry_run=dry_run,
     )
 
-    # display changes
     if dry_run:
-        if not stdout or stdout == 'send ./':
-            # no rsync output == no changes => exit
+        # display changes
+        changes = format_reinstall_output(stdout)
+        if not changes:
             return False
 
         # display rsync output
-        write('\n'.join(format_rsync_out(stdout)), file=sys.stderr)
+        write('\n'.join(changes), file=sys.stdout)
 
-    # run post_install plugins
     if not dry_run:
-        # don't run plugins in dry-mode
+        # run post_install plugins
         async for _entry_point, _plugin_result in run_plugins_async(
             'cylc.post_install',
             srcdir=src_dir,
@@ -311,15 +319,15 @@ async def reinstall(
     return True
 
 
-def format_rsync_out(out: str) -> List[str]:
+def format_reinstall_output(out: str) -> List[str]:
     r"""Format rsync stdout for presenting to users.
 
     Note: Output formats of different rsync implementations may differ so keep
           this code simple and robust.
 
     Example:
-        >>> format_rsync_out(
-        ...     'send foo\ndel. bar\nbaz'
+        >>> format_reinstall_output(
+        ...     '>f+++++++++ send foo\n*deleting   del. bar\nbaz'
         ...     '\ncannot delete non-empty directory: opt'
         ... ) == [
         ...     cparse('<green>send foo</green>'),
@@ -331,20 +339,23 @@ def format_rsync_out(out: str) -> List[str]:
     """
     lines = []
     for line in out.splitlines():
-        if line[0:4] == 'send':
-            # file added or updated
-            lines.append(cparse(f'<green>{line}</green>'))
-        elif line[0:4] == 'del.':
-            # file deleted
-            lines.append(cparse(f'<red>{line}</red>'))
-        elif line == 'cannot delete non-empty directory: opt':
-            # These "cannot delete non-empty directory" messages can arise
-            # as a result of excluding files within sub-directories.
-            # This opt dir message is likely to occur when a rose-suit.conf
+        if not line or line.startswith('cannot delete non-empty directory:'):
+            # This can arise as a result of deleting a subdir when we are
+            # excluding files within the subdir.
+            # This is likely to occur for the opt dir when a rose-suite.conf
             # file is present.
+            # Skip this line as nothing will happen to this dir.
             continue
+        match = re.match(r'^(.{11}) (send|del\.) (.*)$', line)
+        if match:
+            summary, operation, file = match.groups()
+            color = 'green' if operation == 'send' else 'red'
+            formatted_line = f"<{color}>{operation} {file}</{color}>"
+            if cylc.flow.flags.verbosity > 0:
+                formatted_line = f"<{DIM}>{summary}</{DIM}> {formatted_line}"
+            lines.append(cparse(formatted_line))
         else:
-            # other uncategorised log line
+            # shouldn't happen; tolerate unknown rsync implementation?
             lines.append(line)
     return lines
 

tests/integration/test_reinstall.py

@@ -19,28 +19,23 @@
 import asyncio
 from contextlib import asynccontextmanager
 from pathlib import Path
+from secrets import token_hex
 from types import SimpleNamespace
-from uuid import uuid1
 
 import pytest
 
-from cylc.flow.exceptions import (
-    WorkflowFilesError,
-)
-from cylc.flow.install import (
-    reinstall_workflow,
-)
+from cylc.flow.exceptions import WorkflowFilesError
+from cylc.flow.install import reinstall_workflow
 from cylc.flow.option_parsers import Options
 from cylc.flow.scripts.reinstall import (
     get_option_parser as reinstall_gop,
     reinstall_cli,
 )
-from cylc.flow.workflow_files import (
-    WorkflowFiles,
-)
+from cylc.flow.workflow_files import WorkflowFiles
 
 from .utils.entry_points import EntryPointWrapper
 
+
 ReInstallOptions = Options(reinstall_gop())
 
 # cli opts
@@ -66,17 +61,30 @@ def non_interactive(monkeypatch):
     )
 
 
+@pytest.fixture
+def answer_prompt(monkeypatch: pytest.MonkeyPatch):
+    """Answer reinstall prompt."""
+
+    def inner(answer: str):
+        monkeypatch.setattr(
+            'cylc.flow.scripts.reinstall._input', lambda x: answer
+        )
+
+    return inner
+
+
 @pytest.fixture
 def one_src(tmp_path):
-    src_dir = tmp_path
+    src_dir = tmp_path / 'src'
+    src_dir.mkdir()
     (src_dir / 'flow.cylc').touch()
     (src_dir / 'rose-suite.conf').touch()
     return SimpleNamespace(path=src_dir)
 
 
 @pytest.fixture
 def one_run(one_src, test_dir, run_dir):
-    w_run_dir = test_dir / str(uuid1())
+    w_run_dir = test_dir / token_hex(4)
     w_run_dir.mkdir()
     (w_run_dir / 'flow.cylc').touch()
     (w_run_dir / 'rose-suite.conf').touch()
@@ -147,7 +155,7 @@ async def test_interactive(
     capsys,
     capcall,
     interactive,
-    monkeypatch
+    answer_prompt
 ):
     """It should perform a dry-run and prompt in interactive mode."""
     # capture reinstall calls
@@ -158,23 +166,18 @@ async def test_interactive(
     # give it something to reinstall
     (one_src.path / 'a').touch()
 
-    # reinstall answering "no" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'n'
+    answer_prompt('n')
+    assert (
+        await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id)
+        is False
     )
-    assert await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id) is False
 
     # only one rsync call should have been made (i.e. the --dry-run)
     assert [call[1].get('dry_run') for call in reinstall_calls] == [True]
     assert 'Reinstall canceled, no changes made.' in capsys.readouterr().out
     reinstall_calls.clear()
 
-    # reinstall answering "yes" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'y'
-    )
+    answer_prompt('y')
     assert await reinstall_cli(opts=ReInstallOptions(), workflow_id=one_run.id)
 
     # two rsync calls should have been made (i.e. the --dry-run and the real)
@@ -233,7 +236,9 @@ async def test_rsync_stuff(one_src, one_run, capsys, non_interactive):
     assert not (one_run.path / 'c').exists()
 
 
-async def test_rose_warning(one_src, one_run, capsys, interactive, monkeypatch):
+async def test_rose_warning(
+    one_src, one_run, capsys, interactive, answer_prompt
+):
     """It should warn that Rose installed files will be deleted.
 
     See https://github.com/cylc/cylc-rose/issues/149
@@ -243,11 +248,7 @@ async def test_rose_warning(one_src, one_run, capsys, interactive, monkeypatch):
         'Files created by Rose file installation will show as deleted'
     )
 
-    # reinstall answering "no" to any prompt
-    monkeypatch.setattr(
-        'cylc.flow.scripts.reinstall._input',
-        lambda x: 'n'
-    )
+    answer_prompt('n')
     (one_src.path / 'a').touch()  # give it something to install
 
     # reinstall (with rose-suite.conf file)
@@ -302,6 +303,35 @@ async def test_rsync_fail(one_src, one_run, mock_glbl_cfg, non_interactive):
     assert 'An error occurred reinstalling' in str(exc_ctx.value)
 
 
+async def test_permissions_change(
+    one_src,
+    one_run,
+    interactive,
+    answer_prompt,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture,
+):
+    """It detects permissions changes."""
+    # Add script file:
+    script_path: Path = one_src.path / 'myscript'
+    script_path.touch()
+    await reinstall_cli(
+        opts=ReInstallOptions(skip_interactive=True), workflow_id=one_run.id
+    )
+    assert (one_run.path / 'myscript').is_file()
+    capsys.readouterr()  # clears capsys
+
+    # Change permissions (e.g. user forgot to make it executable before)
+    script_path.chmod(0o777)
+    # Answer "no" to reinstall prompt (we just want to test dry run)
+    answer_prompt('n')
+    await reinstall_cli(
+        opts=ReInstallOptions(), workflow_id=one_run.id
+    )
+    out, _ = capsys.readouterr()
+    assert "send myscript" in out
+
+
 @pytest.fixture
 def my_install_plugin(monkeypatch):
     """This configures a single post_install plugin.