diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 00b4f4f9..8a818f0e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -1,14 +1,14 @@ name: Tests +# TODO: REVERT - temporarily allow PR tests on fork branches on: push: branches: - main + - dev-ra-829-2 paths-ignore: - "docs/**" pull_request: - branches: - - main paths-ignore: - "docs/**" diff --git a/menuinst/api.py b/menuinst/api.py index 5481a58f..3696d75c 100644 --- a/menuinst/api.py +++ b/menuinst/api.py @@ -4,6 +4,7 @@ import json import os +import shutil import sys import warnings from logging import getLogger @@ -17,6 +18,7 @@ _UserOrSystem, elevate_as_needed, read_menuinst_toml, + unlink, user_is_admin, write_menuinst_toml, ) @@ -66,12 +68,7 @@ def record_shortcuts( def remove_shortcut_records(prefix: Path, source: str) -> None: - """Remove shortcut entries matching source from menuinst.toml. - - TODO: Use the recorded paths as the source of truth for shortcut removal, - instead of recomputing paths from menu JSON metadata. This would handle - cases where shortcuts were moved or the menu JSON changed. - """ + """Remove shortcut entries matching source from menuinst.toml.""" data = read_menuinst_toml(prefix) if not data: return @@ -89,6 +86,36 @@ def remove_shortcut_records(prefix: Path, source: str) -> None: write_menuinst_toml(prefix, data) +def get_recorded_paths(prefix: Path, source: str) -> list[Path]: + """Get recorded shortcut paths for a source from menuinst.toml.""" + data = read_menuinst_toml(prefix) + shortcuts = data.get("shortcuts", []) + return [Path(s["path"]) for s in shortcuts if s.get("source") == source and "path" in s] + + +def delete_paths(paths: list[Path]) -> list[Path]: + """Delete files or directories at given paths, warning if not found. + + Handles both files (.lnk, .desktop) and directories (.app bundles on macOS). + """ + deleted = [] + for path in paths: + if path.is_file(): + log.debug("Removing %s", path) + unlink(path) + deleted.append(path) + elif path.is_dir(): + log.debug("Removing %s", path) + shutil.rmtree(path, ignore_errors=True) + deleted.append(path) + else: + log.warning( + "Shortcut not found at expected location: %s - may have been moved or deleted", + path, + ) + return deleted + + def _load( metadata_or_path: os.PathLike | dict, target_prefix: str | None = None, @@ -125,10 +152,15 @@ def install( paths = [] paths += menu.create() + item_paths = [] for menu_item in menu_items: - paths += menu_item.create() + created = menu_item.create() + paths += created + item_paths += created # Record shortcuts to menuinst.toml + # Only record MenuItem paths, not Menu paths (.directory files, etc.) + # The Menu's remove() method handles its own cleanup logic if isinstance(metadata_or_path, (str, Path)): source = Path(metadata_or_path).name else: @@ -137,7 +169,7 @@ def install( Path(target_prefix), Path(base_prefix), source, - paths, + item_paths, distribution_name=menu.placeholders.get("DISTRIBUTION_NAME"), ) @@ -154,29 +186,46 @@ def remove( ) -> list[os.PathLike]: target_prefix = target_prefix or DEFAULT_PREFIX base_prefix = base_prefix or DEFAULT_BASE_PREFIX + + # When running as admin with .nonadmin marker, force user mode + # This ensures Menu and MenuItem objects use correct paths + if _maybe_try_user(target_prefix, base_prefix): + _mode = "user" + menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, _mode) menu_items = [item for item in menu_items if item.enabled_for_platform()] if not menu_items: warnings.warn(f"Metadata for {menu.name} is not enabled for {sys.platform}") return [] - paths = [] - for menu_item in menu_items: - paths += menu_item.remove() - paths += menu.remove() - - if not paths and _maybe_try_user(target_prefix, base_prefix): - menu, menu_items = _load(metadata_or_path, target_prefix, base_prefix, "user") - menu_items = [item for item in menu_items if item.enabled_for_platform()] - for menu_item in menu_items: - paths += menu_item.remove() - paths += menu.remove() - - # Remove shortcut records from menuinst.toml + # Determine source name for TOML lookup if isinstance(metadata_or_path, (str, Path)): source = Path(metadata_or_path).name else: source = f"{menu.name}.json" + + # 1. Delete MenuItem shortcut files (.lnk, .desktop, .app) + recorded_paths = get_recorded_paths(Path(target_prefix), source) + if recorded_paths: + paths = delete_paths(recorded_paths) + else: + # Fallback for shortcuts created before menuinst.toml tracking existed. + # This ensures backwards compatibility when upgrading menuinst after + # packages with shortcuts were already installed. + paths = [] + for menu_item in menu_items: + paths.extend(menu_item.delete_paths()) + + # 2. Side-effect cleanup (registry, LaunchServices, MIME types) + for menu_item in menu_items: + menu_item.cleanup_side_effects() + + # 3. Menu directory cleanup (.directory files, etc.) + # The Menu handles its own removal logic (e.g., only removes .directory + # if no other shortcuts from the same menu exist) + paths.extend(menu.remove()) + + # 4. Update TOML records remove_shortcut_records(Path(target_prefix), source) return paths diff --git a/menuinst/platforms/linux.py b/menuinst/platforms/linux.py index 995b897f..9685c071 100644 --- a/menuinst/platforms/linux.py +++ b/menuinst/platforms/linux.py @@ -206,16 +206,24 @@ def create(self) -> Iterable[os.PathLike]: self._update_desktop_database() return self._paths() - def remove(self) -> Iterable[os.PathLike]: - paths = (path for path in self._paths() if Path(path).is_file()) + def cleanup_side_effects(self) -> None: + """Unregister MIME types and update desktop database.""" self._maybe_register_mime_types(register=False) - if paths: - for path in paths: - log.debug("Removing %s", path) - unlink(path) - self._update_desktop_database() + self._update_desktop_database() + + def delete_paths(self) -> tuple[Path, ...]: + """Delete .desktop file and related MIME XML files.""" + paths = tuple(path for path in self._paths() if Path(path).is_file()) + for path in paths: + log.debug("Removing %s", path) + unlink(path) return paths + def remove(self) -> tuple[Path, ...]: + """Remove .desktop file and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _update_desktop_database(self): exe = shutil.which("update-desktop-database") if exe: diff --git a/menuinst/platforms/osx.py b/menuinst/platforms/osx.py index dc85651f..6279de81 100644 --- a/menuinst/platforms/osx.py +++ b/menuinst/platforms/osx.py @@ -96,14 +96,23 @@ def create(self) -> tuple[Path]: self._sign_with_entitlements() return (self.location,) - def remove(self) -> tuple[Path]: - log.debug("Removing %s", self.location) + def cleanup_side_effects(self) -> None: + """Unregister from LaunchServices.""" self._maybe_register_with_launchservices(register=False) + + def delete_paths(self) -> tuple[Path, ...]: + """Delete .app bundle.""" + log.debug("Removing %s", self.location) if self.location.exists(): shutil.rmtree(self.location, ignore_errors=True) return (self.location,) return tuple() + def remove(self) -> tuple[Path, ...]: + """Remove .app bundle and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _create_application_tree(self) -> tuple[Path]: paths = [ self.location / "Contents" / "Resources", diff --git a/menuinst/platforms/win.py b/menuinst/platforms/win.py index 9cb80bbd..847ccdb4 100644 --- a/menuinst/platforms/win.py +++ b/menuinst/platforms/win.py @@ -208,7 +208,8 @@ def create(self) -> tuple[Path, ...]: return paths - def remove(self) -> tuple[Path, ...]: + def cleanup_side_effects(self) -> None: + """Clean up registry entries, terminal profiles, etc.""" changed_extensions = self._unregister_file_extensions() changed_protocols = self._unregister_url_protocols() if changed_extensions or changed_protocols: @@ -217,13 +218,19 @@ def remove(self) -> tuple[Path, ...]: for location in self.menu.terminal_profile_locations: self._add_remove_windows_terminal_profile(location, remove=True) + def delete_paths(self) -> tuple[Path, ...]: + """Delete shortcut files.""" paths = tuple(path for path in self._paths() if Path(path).is_file()) for path in paths: log.debug("Removing %s", path) unlink(path) - return paths + def remove(self) -> tuple[Path, ...]: + """Remove shortcut files and clean up side effects.""" + self.cleanup_side_effects() + return self.delete_paths() + def _paths(self) -> tuple[Path, ...]: paths = [self.location] extra_dirs = [] diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 1f786e83..b6733313 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -1,12 +1,18 @@ """Tests for distribution_name resolution and menuinst.toml tracking.""" import json +import logging +from pathlib import Path import pytest from menuinst.api import ( _install_adapter, + delete_paths, + get_recorded_paths, + install, record_shortcuts, + remove, remove_shortcut_records, write_menuinst_toml, ) @@ -219,3 +225,173 @@ def test_toml_writes_schemaver_format(self, tmp_path): assert data["schema_version"] == MENUINST_TOML_SCHEMA_VERSION # Verify it's a valid SchemaVer string parse_schemaver(data["schema_version"]) + + +class TestGetRecordedPaths: + """Tests for get_recorded_paths().""" + + def test_returns_paths_for_source(self, tmp_path): + """Test that paths matching the given source are returned.""" + write_menuinst_toml( + tmp_path, + { + "shortcuts": [ + {"source": "foo.json", "path": "/path/to/foo.lnk"}, + {"source": "foo.json", "path": "/path/to/bar.lnk"}, + {"source": "baz.json", "path": "/path/to/baz.lnk"}, + ], + }, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [Path("/path/to/foo.lnk"), Path("/path/to/bar.lnk")] + + def test_returns_empty_list_when_no_matches(self, tmp_path): + """Test that empty list is returned when no shortcuts match source.""" + write_menuinst_toml( + tmp_path, + {"shortcuts": [{"source": "other.json", "path": "/path/to/other.lnk"}]}, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [] + + def test_returns_empty_list_when_no_toml(self, tmp_path): + """Test that empty list is returned when menuinst.toml doesn't exist.""" + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [] + + def test_skips_entries_missing_path_key(self, tmp_path): + """Test that shortcuts missing the 'path' key are skipped.""" + write_menuinst_toml( + tmp_path, + { + "shortcuts": [ + {"source": "foo.json", "path": "/valid/path.lnk"}, + {"source": "foo.json"}, # Missing path key + ], + }, + ) + paths = get_recorded_paths(tmp_path, "foo.json") + assert paths == [Path("/valid/path.lnk")] + + +class TestDeletePaths: + """Tests for delete_paths().""" + + def test_deletes_existing_files(self, tmp_path): + """Test that existing files are deleted and their paths returned.""" + file1 = tmp_path / "foo.lnk" + file2 = tmp_path / "bar.lnk" + file1.touch() + file2.touch() + + deleted = delete_paths([file1, file2]) + + assert not file1.exists() + assert not file2.exists() + assert len(deleted) == 2 + + def test_deletes_directories(self, tmp_path): + """Test that directories (e.g., .app bundles) are deleted using rmtree.""" + app_dir = tmp_path / "MyApp.app" + app_dir.mkdir() + (app_dir / "Contents").mkdir() + (app_dir / "Contents" / "Info.plist").touch() + + deleted = delete_paths([app_dir]) + + assert not app_dir.exists() + assert len(deleted) == 1 + + def test_warns_on_missing_path(self, tmp_path, caplog): + """Test that a warning is logged when path doesn't exist.""" + missing = tmp_path / "nonexistent.lnk" + + with caplog.at_level(logging.WARNING): + deleted = delete_paths([missing]) + + assert len(deleted) == 0 + assert "Shortcut not found at expected location" in caplog.text + assert str(missing) in caplog.text + + +class TestRemoveUsesTomlPaths: + """Tests for TOML-based shortcut removal.""" + + def test_remove_uses_recorded_paths(self, tmp_path): + """Test that remove() deletes files at recorded TOML paths, not computed paths.""" + (tmp_path / ".nonadmin").touch() + menu_dir = tmp_path / "Menu" + menu_dir.mkdir() + + recorded_path = tmp_path / "recorded_location" / "MyShortcut.lnk" + recorded_path.parent.mkdir(parents=True) + recorded_path.touch() + + write_menuinst_toml( + tmp_path, + {"shortcuts": [{"source": "test.json", "path": str(recorded_path)}]}, + ) + + json_file = menu_dir / "test.json" + json_file.write_text( + json.dumps( + { + "$schema": "https://json-schema.org/draft-07/schema", + "menu_name": "Test Menu", + "menu_items": [ + { + "name": "MyShortcut", + "command": ["echo", "test"], + "activate": False, + "platforms": {"linux": {}, "win": {}, "osx": {}}, + } + ], + } + ) + ) + + remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + + assert not recorded_path.exists() + + def test_remove_falls_back_when_no_toml_entries(self, tmp_path, monkeypatch): + """Test that remove() falls back to computed paths when no TOML entries exist.""" + monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False) + (tmp_path / ".nonadmin").touch() + menu_dir = tmp_path / "Menu" + menu_dir.mkdir() + + json_file = menu_dir / "test.json" + json_file.write_text( + json.dumps( + { + "$schema": "https://json-schema.org/draft-07/schema", + "menu_name": "Test Menu", + "menu_items": [ + { + "name": "MyShortcut", + "command": ["echo", "test"], + "activate": False, + "platforms": {"linux": {}, "win": {}, "osx": {}}, + } + ], + } + ) + ) + + # Install shortcuts + paths = install(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + shortcut_paths = [ + p for p in paths if Path(p).suffix in (".lnk", ".desktop") or Path(p).is_dir() + ] + assert shortcut_paths, "Expected at least one shortcut to be created" + + # Clear TOML to simulate legacy install (pre-TOML tracking) + write_menuinst_toml(tmp_path, {}) + + # Remove should use fallback computed paths + remove(str(json_file), target_prefix=str(tmp_path), base_prefix=str(tmp_path)) + + # Verify shortcuts were removed + for p in shortcut_paths: + assert not Path(p).exists(), f"Shortcut should have been removed: {p}"