diff --git a/docs/WEB_INTERFACE_GUIDE.md b/docs/WEB_INTERFACE_GUIDE.md index 10ef082b..f2e077b6 100644 --- a/docs/WEB_INTERFACE_GUIDE.md +++ b/docs/WEB_INTERFACE_GUIDE.md @@ -102,7 +102,9 @@ Configure basic system settings: check are shown under the toggle, and anything other than success raises a banner on **Overview**. - *Checks first:* the code update is skipped, with the reason shown, if - tracked files were edited locally, the checkout has local commits, a + tracked files were edited locally (permission-only changes and edits under + `plugins/` or `plugin-repos/` don't count; the pull carries those across + and puts them back), the checkout has local commits, a rebase/merge is in progress, the branch has no upstream, less than 300 MB is free, or the newest version already failed once. A failed fetch is retried the next day. diff --git a/scripts/utils/auto_update_verify.py b/scripts/utils/auto_update_verify.py index 048721a2..787933fd 100644 --- a/scripts/utils/auto_update_verify.py +++ b/scripts/utils/auto_update_verify.py @@ -191,8 +191,11 @@ class Verifier: if not old: return False, 'the commit to roll back to is unknown' requirements = self.changed_requirements(old, new) if new else list(REQUIREMENT_FILES) - # Safe to --hard: the updater refuses to run with local edits to - # tracked files, so the only thing this discards is the update. + # --hard: the updater refuses to run with local edits to tracked core + # files (web_interface/auto_update.local_changes), so outside the + # plugin folders the only thing this discards is the update. Edits + # under plugins/ and plugin-repos/, which that check leaves to the + # pull's --autostash, are reset along with it. result = self._run(['git', 'reset', '--hard', old], timeout=120) if result.returncode != 0: return False, (f'"git reset --hard {old}" failed: ' diff --git a/test/test_auto_update.py b/test/test_auto_update.py index 8acb1e8b..46a02230 100644 --- a/test/test_auto_update.py +++ b/test/test_auto_update.py @@ -72,7 +72,7 @@ class Repo: def real_pull(device, **extra): - def core_update(): + def core_update(**kwargs): before = git(device, 'rev-parse', 'HEAD') r = subprocess.run(['git', 'pull', '-q', '--rebase', '--autostash'], cwd=str(device), capture_output=True, text=True) @@ -339,6 +339,140 @@ class TestPreflightRefuses: assert repo.head() == old and h.sudo == [] +# -- what counts as a local change --------------------------------------------- + +def _device_with_plugin_and_script(tmp_path): + """A clone carrying a bundled plugin and an executable script, as LEDMatrix does.""" + repo = Repo(tmp_path) + for rel, text in (('plugin-repos/web-ui-info/manager.py', 'x = 1\n'), + ('web_interface/static/v3/js/plugins/store.js', 'var a;\n'), + ('scripts/run.sh', 'echo hi\n')): + (repo.seed / rel).parent.mkdir(parents=True, exist_ok=True) + (repo.seed / rel).write_text(text) + git(repo.seed, 'add', '.') + git(repo.seed, 'update-index', '--chmod=+x', 'scripts/run.sh') + git(repo.seed, 'commit', '-qm', 'layout') + git(repo.seed, 'push', '-q', 'origin', 'main') + git(repo.device, 'pull', '-q') + return repo + + +def _chmod_like_the_installer(device): + """first_time_install.sh sets tracked 100755 files to 644: a mode-only change. + + Windows has no exec bit, so there core.fileMode=true alone shows it.""" + import os + git(device, 'config', 'core.fileMode', 'true') + os.chmod(device / 'scripts' / 'run.sh', 0o644) + assert 'mode change 100755 => 100644 scripts/run.sh' in git(device, 'diff', '--summary') + + +@needs_git +class TestLocalChangesAreCountedOnce: + """The preflight promises the pull will not stash. That holds only if both + count local changes the same way (auto_update.local_changes).""" + + def test_mode_changes_and_plugin_folders_do_not_count(self, tmp_path): + repo = _device_with_plugin_and_script(tmp_path) + _chmod_like_the_installer(repo.device) + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + assert au.local_changes(repo.device) == [] + + def test_a_core_folder_named_plugins_still_counts(self, tmp_path): + """The old filter matched 'plugins/' anywhere in the line.""" + repo = _device_with_plugin_and_script(tmp_path) + (repo.device / 'web_interface/static/v3/js/plugins/store.js').write_text('var mine;\n') + assert au.local_changes(repo.device) == ['web_interface/static/v3/js/plugins/store.js'] + + def test_a_staged_rename_is_reported_by_its_new_name(self, tmp_path): + repo = Repo(tmp_path) + git(repo.device, 'mv', 'app.py', 'main.py') + assert au.local_changes(repo.device) == ['main.py'] + + def test_the_preflight_refuses_a_core_edit_under_a_plugins_folder(self, tmp_path): + repo = _device_with_plugin_and_script(tmp_path) + old = repo.head() + repo.publish() + (repo.device / 'web_interface/static/v3/js/plugins/store.js').write_text('var mine;\n') + result = Harness(tmp_path, repo).updater.run() + assert result['core_outcome'] == 'blocked' + assert 'store.js' in result['core_message'] + assert repo.head() == old + + @pytest.fixture + def core_update(self, monkeypatch): + """The real perform_core_update, pointed at a test clone.""" + from web_interface.blueprints import api_v3 as pkg + from web_interface.blueprints.api_v3 import system + + def point_at(device): + monkeypatch.setattr(system, 'PROJECT_ROOT', device) + monkeypatch.setattr(pkg.api_v3, 'plugin_store_manager', None, raising=False) + monkeypatch.setattr(system, '_pip_install_requirements', + lambda *a, **k: pytest.fail('no requirements changed')) + return system.perform_core_update + return point_at + + @pytest.mark.parametrize('change', [ + 'bundled_plugin_edit', + pytest.param('installer_chmod', marks=pytest.mark.skipif( + sys.platform == 'win32', + reason='no exec bit: the reset inside --autostash cannot clear a mode change, ' + 'so git will not reapply it (on Linux it reapplies cleanly)')), + ]) + def test_an_update_that_passed_the_preflight_leaves_no_stash(self, tmp_path, core_update, change): + """Found by audit: a bundled-plugin edit or the installer's chmods + passed the preflight, then perform_core_update stashed them for good. + Now --autostash carries them across the pull and puts them back.""" + repo = _device_with_plugin_and_script(tmp_path) + new = repo.publish() + if change == 'installer_chmod': + _chmod_like_the_installer(repo.device) + else: + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + h = Harness(tmp_path, repo, core_update=core_update(repo.device)) + assert h.updater.preflight({})[0] == 'ready' + + result = h.updater.run() + assert result['core_outcome'] == 'verifying', result['core_message'] + assert repo.head() == new + assert git(repo.device, 'stash', 'list') == '' + if change == 'installer_chmod': + assert 'mode change 100755 => 100644 scripts/run.sh' in git(repo.device, 'diff', '--summary') + else: + assert 'store update' in (repo.device / 'plugin-repos/web-ui-info/manager.py').read_text() + + def test_edits_made_after_the_preflight_are_refused_not_stashed(self, tmp_path, core_update): + repo = Repo(tmp_path) + old = repo.head() + repo.publish() + perform = core_update(repo.device) + + def edit_then_update(**kwargs): + (repo.device / 'app.py').write_text('mine\n') + return perform(**kwargs) + result = Harness(tmp_path, repo, core_update=edit_then_update).updater.run() + assert result['core_outcome'] == 'blocked' + assert 'app.py' in result['core_message'] and 'will not stash' in result['core_message'] + assert repo.head() == old + assert git(repo.device, 'stash', 'list') == '' + assert (repo.device / 'app.py').read_text() == 'mine\n' + + def test_update_code_still_stashes_core_edits_but_not_plugin_folders(self, tmp_path, core_update): + repo = _device_with_plugin_and_script(tmp_path) + new = repo.publish() + (repo.device / 'scripts/run.sh').write_text('echo mine\n') + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + payload = core_update(repo.device)() + assert payload['status'] == 'success', payload['message'] + assert 'stashed' in payload['message'] + assert repo.head() == new + assert 'LEDMatrix auto-stash before update' in git(repo.device, 'stash', 'list') + stashed = git(repo.device, 'stash', 'show', '--name-only', 'stash@{0}') + assert stashed.split() == ['scripts/run.sh'] + assert 'store update' in (repo.device / 'plugin-repos/web-ui-info/manager.py').read_text() + + # -- the update and its hand-off to the health check --------------------------- @needs_git @@ -395,7 +529,7 @@ class TestUpdateIsVerified: old = repo.head() repo.publish() h = Harness(tmp_path, repo, - core_update=lambda: {'status': 'error', 'message': 'Update failed: network'}) + core_update=lambda **kw: {'status': 'error', 'message': 'Update failed: network'}) result = h.updater.run() assert result['core_outcome'] == 'error' assert repo.head() == old @@ -407,7 +541,7 @@ class TestUpdateIsVerified: repo.publish() pull = real_pull(repo.device) - def half_failed(): + def half_failed(**kwargs): pull() return {'status': 'error', 'message': 'Update failed: interrupted'} result = Harness(tmp_path, repo, core_update=half_failed).updater.run() diff --git a/web_interface/auto_update.py b/web_interface/auto_update.py index 129f8d15..610391a1 100644 --- a/web_interface/auto_update.py +++ b/web_interface/auto_update.py @@ -263,6 +263,56 @@ def start_setup_if_needed(was_enabled, config): return 'Could not restart the display to finish automatic update setup; restart it from the Overview tab.' +#: Top-level folders of separately installed plugins. Edits there are not the +#: core's local changes: the Plugin Store updates plugins in place, including +#: the bundled ones committed under plugin-repos/, and the pull's --autostash +#: carries those edits across and reapplies them. +SEPARATE_INSTALL_DIRS = ('plugins', 'plugin-repos') + + +def local_changes(project_dir, run=None, timeout=30): + """Tracked files edited in the checkout, as both code updates count them. + + The one definition shared by the automatic update's preflight and + ``perform_core_update`` (Update Code), so the preflight's promise that + nothing will be stashed holds for the pull that follows it. + + * Mode-only changes do not count (``core.fileMode=false``): installers + chmod tracked scripts, and --autostash carries modes across the pull. + * Paths under ``SEPARATE_INSTALL_DIRS`` do not count. The match is on + the leading folder, not a substring, so + ``web_interface/static/v3/js/plugins/x.js`` is still a core edit. + + Returns the changed paths, or None when git could not tell. Raises what + ``run`` raises (e.g. ``subprocess.TimeoutExpired``). + """ + run = run or subprocess.run + result = run(['git', '-c', 'core.fileMode=false', 'status', '--porcelain', + '--untracked-files=no', '-z'], + cwd=str(project_dir), capture_output=True, text=True, timeout=timeout) + if result.returncode != 0: + return None + changed = [] + entries = iter((result.stdout or '').split('\0')) + for entry in entries: + if len(entry) < 4: + continue + paths = [entry[3:]] + if entry[0] in 'RC' or entry[1] in 'RC': + paths.append(next(entries, '')) # -z puts a rename's source next + if any(p and p.split('/', 1)[0] not in SEPARATE_INSTALL_DIRS for p in paths): + changed.append(paths[0]) + return changed + + +def describe_local_changes(changed): + """Why an automatic update refused to touch a checkout with local edits.""" + example = f' (for example {changed[0]})' if changed else '' + return (f'{len(changed or ()) or "Some"} tracked file(s) in the LEDMatrix folder were edited ' + f'locally{example}. Automatic updates will not stash your changes; ' + 'commit or revert them, or update manually with Update Code.') + + def _load_verifier(path): spec = importlib.util.spec_from_file_location('ledmatrix_auto_update_verifier', str(path)) module = importlib.util.module_from_spec(spec) @@ -429,17 +479,11 @@ class AutoUpdater: return 'blocked', ('The current branch has no upstream to update from (or HEAD is detached). ' 'Use Update Code once, or Tools -> Switch branch.'), {} - # Mode-only changes are ignored: older installers chmod tracked - # scripts, and --autostash in the pull carries those across. Plugin - # folders are separate installs, as in perform_core_update. - status = self._git('-c', 'core.fileMode=false', 'status', '--porcelain', '--untracked-files=no') - changed = [line[3:] for line in status.stdout.splitlines() - if line.strip() and 'plugins/' not in line and 'plugin-repos/' not in line] - if status.returncode != 0 or changed: - example = f' (for example {changed[0]})' if changed else '' - return 'blocked', (f'{len(changed) or "Some"} tracked file(s) in the LEDMatrix folder were edited ' - f'locally{example}. Automatic updates will not stash your changes; ' - 'commit or revert them, or update manually with Update Code.'), {} + # The same predicate perform_core_update refuses on when called from + # here, so what passes this check is never stashed by the pull. + changed = local_changes(self.project_root, run=self.run_command) + if changed is None or changed: + return 'blocked', describe_local_changes(changed), {} free = self.disk_free(str(self.project_root)) if free < MIN_FREE_BYTES: @@ -484,11 +528,15 @@ class AutoUpdater: return {'outcome': 'error', 'message': f'Could not prepare the update health check: {e}.'} try: - core = self.core_update() + # Refuse rather than stash: edits made since the preflight are the + # user's, and nothing would ever restore a stash taken here. + core = self.core_update(stash_local_changes=False) except Exception as e: logger.exception("perform_core_update raised") core = {'status': 'error', 'message': f'Update failed: {e}'} new_head = self._git('rev-parse', 'HEAD').stdout.strip() + if core.get('local_changes') is not None and new_head == old_head: + return {'outcome': 'blocked', 'message': describe_local_changes(core['local_changes'])} pending = { 'status': 'pending', 'old_head': old_head, diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index 0ba98a1c..1e82b7fd 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/web_interface/blueprints/api_v3/system.py @@ -235,13 +235,17 @@ _core_update_lock = threading.Lock() CORE_REQUIREMENT_FILES = ('requirements.txt', 'web_interface/requirements.txt') -def perform_core_update(): +def perform_core_update(stash_local_changes=True): """Pull the latest LEDMatrix code and sync its dependencies. Shared by the Overview "Update Code" button and the weekly automatic updater (web_interface/auto_update.py), so both take exactly the same path. Returns the JSON-able payload the button has always received: ``status``, ``message`` and ``restart_required``. + + Update Code stashes local edits before pulling. The automatic updater + passes ``stash_local_changes=False``: then local edits make this return + an error carrying ``local_changes`` (the edited paths) without pulling. """ # The button and the scheduler can fire together; two pulls racing # over one checkout (and one stash) is how local changes get lost. @@ -249,12 +253,12 @@ def perform_core_update(): return {'status': 'error', 'restart_required': False, 'message': 'An update is already in progress; try again shortly.'} try: - return _perform_core_update_locked() + return _perform_core_update_locked(stash_local_changes) finally: _core_update_lock.release() -def _perform_core_update_locked(): +def _perform_core_update_locked(stash_local_changes=True): project_dir = str(PROJECT_ROOT) # Decide how to pull BEFORE stashing. If this checkout cannot be @@ -265,37 +269,35 @@ def _perform_core_update_locked(): logger.warning("git pull not attempted: %s", pull_error) return {'status': 'error', 'message': pull_error, 'restart_required': False} - # Check if there are local changes that need to be stashed - # Exclude plugins directory - plugins are separate repos and shouldn't be stashed with base project - # Use --untracked-files=no to skip untracked files check (much faster with symlinked plugins) + # Local changes, counted exactly as the automatic update's preflight + # counts them (auto_update.local_changes): mode-only changes and the + # plugin folders don't count, and the pull's --autostash carries those + # across and reapplies them. + from web_interface import auto_update try: - status_result = subprocess.run( - ['git', 'status', '--porcelain', '--untracked-files=no'], - capture_output=True, - text=True, - timeout=30, - cwd=project_dir - ) - # Filter out any changes in plugins directory - plugins are separate repositories - # Git status format: XY filename (where X is status of index, Y is status of work tree) - status_lines = [line for line in status_result.stdout.strip().split('\n') - if line.strip() and 'plugins/' not in line] - has_changes = bool('\n'.join(status_lines).strip()) - except subprocess.TimeoutExpired: - # If status check times out, assume there might be changes and proceed - # This is safer than failing the update - has_changes = True - status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})() + changed = auto_update.local_changes(project_dir) + except (subprocess.SubprocessError, OSError) as status_err: + logger.warning("git status failed before pull: %s", status_err) + changed = None + # When git cannot say, assume there are changes rather than pull over them. + has_changes = changed is None or bool(changed) + + if has_changes and not stash_local_changes: + # The automatic updater: it promised not to stash, and nothing would + # ever restore a stash taken on its behalf. + return {'status': 'error', 'restart_required': False, 'dependency_failures': [], + 'local_changes': list(changed or []), + 'message': auto_update.describe_local_changes(changed)} stash_info = "" - # Stash local changes if they exist (excluding plugins) - # Plugins are separate repositories and shouldn't be stashed with base project updates + # Stash local changes if they exist. The plugin folders are left out: + # plugins are separate installs, and --autostash carries their edits. if has_changes: try: - # Use pathspec to exclude plugins directory from stash stash_result = subprocess.run( - ['git', 'stash', 'push', '-m', 'LEDMatrix auto-stash before update', '--', ':!plugins'], + ['git', 'stash', 'push', '-m', 'LEDMatrix auto-stash before update', '--', + *(f':!{folder}' for folder in auto_update.SEPARATE_INSTALL_DIRS)], capture_output=True, text=True, timeout=30,