Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 74 additions & 2 deletions menuinst/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
DEFAULT_PREFIX,
_UserOrSystem,
elevate_as_needed,
read_menuinst_toml,
user_is_admin,
write_menuinst_toml,
)

log = getLogger(__name__)
Expand All @@ -38,6 +40,55 @@ def _maybe_try_user(base_prefix: str, target_prefix: str) -> bool:
return Path(base_prefix, ".nonadmin").is_file()


def record_shortcuts(
prefix: Path,
base_prefix: Path,
source: str,
paths: list[os.PathLike],
distribution_name: str | None = None,
) -> None:
"""Record created shortcuts to menuinst.toml."""
if not paths:
return

data = read_menuinst_toml(prefix)

# Write distribution_name only to base prefix, and only if not already set
if prefix.samefile(base_prefix) and distribution_name:
data.setdefault("distribution_name", distribution_name)
Comment thread
marcoesters marked this conversation as resolved.

# Append shortcuts
shortcuts = data.setdefault("shortcuts", [])
for path in paths:
shortcuts.append({"source": source, "path": str(path)})

write_menuinst_toml(prefix, data)


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.
Comment on lines +71 to +73
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I may be missing some of the complexity here, but I think this is well in scope of this PR. All the pieces are already there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added this TODO to help the reviewers and keep it simple. Merging this wouldn't break any existing behavior. I'm happy to do it in this PR but perhaps even opening a separate PR targeting this branch would be more feasible, let me know what you prefer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A separate PR targeting this branch would work for me. As of this PR, all we do with those recorded paths is remove them from the record on uninstallation. So, the records currently don't find much use.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@marcoesters please see this separate PR lrandersson#1 targeting this functionality.

"""
data = read_menuinst_toml(prefix)
if not data:
return

shortcuts = data.get("shortcuts", [])
if not shortcuts:
return

# Filter out entries matching this source
filtered = [s for s in shortcuts if s.get("source") != source]
if len(filtered) == len(shortcuts):
return # Nothing was removed

data["shortcuts"] = filtered
write_menuinst_toml(prefix, data)


def _load(
metadata_or_path: os.PathLike | dict,
target_prefix: str | None = None,
Expand Down Expand Up @@ -77,6 +128,19 @@ def install(
for menu_item in menu_items:
paths += menu_item.create()

# Record shortcuts to menuinst.toml
if isinstance(metadata_or_path, (str, Path)):
source = Path(metadata_or_path).name
else:
source = f"{menu.name}.json"
record_shortcuts(
Path(target_prefix),
Path(base_prefix),
source,
paths,
distribution_name=menu.placeholders.get("DISTRIBUTION_NAME"),
)

return paths


Expand Down Expand Up @@ -108,6 +172,13 @@ def remove(
paths += menu_item.remove()
paths += menu.remove()

# Remove shortcut records from menuinst.toml
if isinstance(metadata_or_path, (str, Path)):
source = Path(metadata_or_path).name
else:
source = f"{menu.name}.json"
remove_shortcut_records(Path(target_prefix), source)

return paths


Expand Down Expand Up @@ -198,7 +269,8 @@ def _install_adapter(path: str, remove: bool = False, prefix: str = DEFAULT_PREF
kwargs.setdefault("base_prefix", kwargs.pop("root_prefix", DEFAULT_BASE_PREFIX))
if kwargs["base_prefix"] is None:
kwargs["base_prefix"] = DEFAULT_BASE_PREFIX
# Pass path so install/remove records the actual filename in menuinst.toml
if remove:
_api_remove(metadata, target_prefix=prefix, **kwargs)
_api_remove(json_path, target_prefix=prefix, **kwargs)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change was done to properly register the file name the shortcuts originate from in the toml file. There are other ways to achieve the same behavior but this approach required the least intrusive code changes.

else:
install(metadata, target_prefix=prefix, **kwargs)
install(json_path, target_prefix=prefix, **kwargs)
13 changes: 13 additions & 0 deletions menuinst/cli/cli.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from __future__ import annotations

import argparse
import os
import re
import sys
from pathlib import Path

from ..api import _install_adapter
from ..utils import read_menuinst_toml, write_menuinst_toml

_MENU_RE = re.compile(r"(?:[-\._]menu)?\.json$", re.IGNORECASE)

Expand Down Expand Up @@ -70,6 +72,17 @@ def install(
if root_prefix:
root_prefix = str(Path(root_prefix).expanduser().resolve())

# Persist distribution_name from env var before processing shortcuts.
# The env var allows installers to set distribution_name dynamically at
# install time. We must persist it here because the env var is transient
# and may not be set when packages with shortcuts are installed later.
if distribution_name := os.environ.get("MENUINST_DISTRIBUTION_NAME"):
base = Path(root_prefix) if root_prefix else prefix
data = read_menuinst_toml(base)
if "distribution_name" not in data:
data["distribution_name"] = distribution_name
write_menuinst_toml(base, data)

for json_path in (prefix / "Menu").glob("*.json"):
if (
packages
Expand Down
19 changes: 18 additions & 1 deletion menuinst/platforms/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
data_path,
deep_update,
logged_run,
read_menuinst_toml,
slugify,
)

Expand Down Expand Up @@ -64,6 +65,22 @@ def render(self, value: Any, slug: bool = False, extra: dict | None = None) -> A
value = slugify(value)
return value

def _get_distribution_name(self) -> str:
"""
Get distribution name with resolution order:
1. MENUINST_DISTRIBUTION_NAME env var
2. $BASE_PREFIX/Menu/menuinst.toml
3. base_prefix.name (fallback)
"""
if name := os.environ.get("MENUINST_DISTRIBUTION_NAME"):
return name

data = read_menuinst_toml(self.base_prefix)
if name := data.get("distribution_name"):
return name

return self.base_prefix.name

@property
def placeholders(self) -> dict[str, str]:
"""
Expand All @@ -74,7 +91,7 @@ def placeholders(self) -> dict[str, str]:
"""
return {
"BASE_PREFIX": str(self.base_prefix),
"DISTRIBUTION_NAME": self.base_prefix.name,
"DISTRIBUTION_NAME": self._get_distribution_name(),
"BASE_PYTHON": str(self.base_prefix / "bin" / "python"),
Comment thread
marcoesters marked this conversation as resolved.
"PREFIX": str(self.prefix),
"ENV_NAME": self.env_name,
Expand Down
65 changes: 65 additions & 0 deletions menuinst/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,26 @@
from typing import Any, Callable, Iterable, Literal, Mapping, Optional, Sequence, Union
from unicodedata import normalize

try:
import tomllib
except ImportError:
import tomli as tomllib

import tomli_w

logger = getLogger(__name__)
MENUINST_TOML_SCHEMA_VERSION = "1-0-0"
_TargetOrBase = Union[Literal["target"], Literal["base"]]


def parse_schemaver(version: str) -> tuple[int, int, int]:
"""Parse SchemaVer string 'MODEL-REVISION-ADDITION' into tuple of ints."""
parts = version.split("-")
if len(parts) != 3:
raise ValueError(f"Invalid SchemaVer format: {version}")
return int(parts[0]), int(parts[1]), int(parts[2])


_UserOrSystem = Union[Literal["user"], Literal["system"]]


Expand Down Expand Up @@ -70,6 +88,53 @@ def _default_prefix(which: _TargetOrBase = "target") -> str:
DEFAULT_BASE_PREFIX = _default_prefix("base")


def read_menuinst_toml(prefix: Path) -> dict:
"""Read menuinst.toml from prefix, returning empty dict if missing.

Raises an exception if the file exists but is invalid.
"""
toml_path = prefix / "Menu" / "menuinst.toml"
if not toml_path.exists():
return {}
try:
with open(toml_path, "rb") as f:
data = tomllib.load(f)
except Exception as exc:
raise ValueError(f"Failed to read {toml_path}") from exc
version = data.get("schema_version", "1-0-0")
try:
file_ver = parse_schemaver(version)
current_ver = parse_schemaver(MENUINST_TOML_SCHEMA_VERSION)
except ValueError:
logger.warning(
"menuinst.toml at %s has invalid schema_version %r",
toml_path,
version,
)
return data
if file_ver > current_ver:
logger.warning(
"menuinst.toml at %s has schema_version %s, "
"but this menuinst only understands version %s",
toml_path,
version,
MENUINST_TOML_SCHEMA_VERSION,
)
return data


def write_menuinst_toml(prefix: Path, data: dict) -> None:
"""Write menuinst.toml atomically."""
data.setdefault("schema_version", MENUINST_TOML_SCHEMA_VERSION)
menu_dir = prefix / "Menu"
menu_dir.mkdir(parents=True, exist_ok=True)
toml_path = menu_dir / "menuinst.toml"
tmp_path = menu_dir / "menuinst.toml.tmp"
with open(tmp_path, "wb") as f:
tomli_w.dump(data, f)
tmp_path.replace(toml_path)


def slugify(text: str):
# Adapted from from django.utils.text.slugify
# Copyright (c) Django Software Foundation and individual contributors.
Expand Down
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ requires-python = ">=3.10"
dynamic = [
"version"
]
dependencies = [
"tomli; python_version < '3.11'",
"tomli-w",
]

[project.scripts]
menuinst = "menuinst.cli:main"
Expand Down
2 changes: 2 additions & 0 deletions recipe/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ requirements:
- setuptools_scm >=6.2
run:
- python
- tomli # [py<311]
- tomli-w

test:
requires:
Expand Down
35 changes: 31 additions & 4 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
from __future__ import annotations

import json
from typing import TYPE_CHECKING
from pathlib import Path

import pytest
from conftest import DATA

from menuinst.api import install, remove
from menuinst.cli import main as menuinst_main

if TYPE_CHECKING:
from pathlib import Path


def _setup_menu_directory(prefix: Path) -> dict[str, set[Path]]:
packages = {
Expand Down Expand Up @@ -114,3 +111,33 @@ def test_cli_errors(argv: list[str]) -> None:
with pytest.raises(SystemExit) as exc:
menuinst_main(argv)
assert exc.value.code == 2


@pytest.mark.parametrize(
"env_var_set,expect_toml",
[
pytest.param(True, True, id="env_var_set_creates_toml"),
pytest.param(False, False, id="no_env_var_no_toml"),
],
)
def test_cli_env_var_persistence(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch, env_var_set: bool, expect_toml: bool
) -> None:
"""Test that MENUINST_DISTRIBUTION_NAME env var persists in menuinst.toml."""
(tmp_path / ".nonadmin").touch()
(tmp_path / "Menu").mkdir()

if env_var_set:
monkeypatch.setenv("MENUINST_DISTRIBUTION_NAME", "TestApp")
else:
monkeypatch.delenv("MENUINST_DISTRIBUTION_NAME", raising=False)

menuinst_main(["--install", "--prefix", str(tmp_path), "--root-prefix", str(tmp_path)])

toml_path = tmp_path / "Menu" / "menuinst.toml"
if expect_toml:
assert toml_path.exists(), "menuinst.toml should be created when env var is set"
content = toml_path.read_text()
assert 'distribution_name = "TestApp"' in content
else:
assert not toml_path.exists(), "menuinst.toml should not be created without env var"
Loading
Loading