Fix reinstall not detecting changed permissions by itself

Author: MetRonnie

cylc/flow/install.py

@@ -213,19 +213,16 @@ 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+++')
+    rsync_cmd.append('--out-format=%i %o %n%L')
+    # %i: itemized changes - needed for rsync to report files with
+    # changed permissions
 
     # 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

@@ -183,6 +183,9 @@ def get_includes_to_rsync(rsync_includes=None):
     '--out-format=%o %n%L',
     '--no-t'
 ]
+# %o: the operation (send or del.)
+# %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.stderr)
 
-    # 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

@@ -68,7 +68,8 @@ def non_interactive(monkeypatch):
 
 @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)

tests/unit/scripts/test_reinstall.py

@@ -0,0 +1,56 @@
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from textwrap import dedent
+
+from ansimarkup import parse as cparse
+import pytest
+
+from cylc.flow.scripts.reinstall import format_reinstall_output
+from cylc.flow.terminal import DIM
+
+
+@pytest.mark.parametrize('verbose', [True, False])
+def test_format_reinstall_output(verbose, monkeypatch: pytest.MonkeyPatch):
+    """It should:
+    - colorize the output
+    - remove the itemized changes summary if not in verbose mode
+    - remove the "cannot delete non-empty directory" message
+    """
+    output = dedent("""
+        *deleting   del. Cloud.jpg
+        >f+++++++++ send cloud.jpg
+        .f...p..... send foo
+        >fcsTp..... send bar
+        cannot delete non-empty directory: scarf
+        >f+++++++++ send meow.txt
+        cL+++++++++ send garage -> foo
+    """).strip()
+    expected = [
+        f"<{DIM}>*deleting  </{DIM}> <red>del. Cloud.jpg</red>",
+        f"<{DIM}>>f+++++++++</{DIM}> <green>send cloud.jpg</green>",
+        f"<{DIM}>.f...p.....</{DIM}> <green>send foo</green>",
+        f"<{DIM}>>fcsTp.....</{DIM}> <green>send bar</green>",
+        f"<{DIM}>>f+++++++++</{DIM}> <green>send meow.txt</green>",
+        f"<{DIM}>cL+++++++++</{DIM}> <green>send garage -> foo</green>",
+    ]
+    if verbose:
+        monkeypatch.setattr('cylc.flow.flags.verbosity', 1)
+    else:
+        # itemized changes summary should not be in output
+        shift = len(f'<{DIM}></{DIM}> ') + 11
+        expected = [i[shift:] for i in expected]
+    assert format_reinstall_output(output) == [cparse(i) for i in expected]