Add support for menuinst.toml by lrandersson · Pull Request #477 · conda/menuinst

lrandersson · 2026-05-11T16:56:29Z

Summary

Adds the possibility to override DISTRIBUTION_NAME and improves tracking of installed shortcuts.

Changes

Add MENUINST_DISTRIBUTION_NAME env var support with resolution order:
1. MENUINST_DISTRIBUTION_NAME env var (highest priority)
2. $BASE_PREFIX/Menu/menuinst.toml value
3. base_prefix.name (fallback)
Add menuinst.toml for persistent storage of distribution_name and shortcut tracking
Add read_menuinst_toml() in utils.py and write_menuinst_toml() in api.py
Add record_shortcuts() / remove_shortcut_records() called from install() / remove()
Add tomli (py<3.11) and tomli-w dependencies

Planned changes once on main

constructor: Set MENUINST_DISTRIBUTION_NAME=%APPNAME% in run_installation.bat for MSI installers
Integration tests - Add tests that call install()/remove() with real menu JSON to verify end-to-end TOML recording
Use recorded paths for removal - Update remove() to use the recorded shortcut paths from menuinst.toml whenever possible instead of recomputing from menu JSON metadata

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
Add / update outdated documentation?

lrandersson · 2026-05-12T19:39:57Z

+        # 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)


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.

marcoesters

This is nice - I am a little worried about the special-casing of DISTRIBUTION_NAME and made an alternative suggestion. I haven't thought through all the ramifications of that suggestion yet though.

marcoesters · 2026-05-13T16:02:34Z

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


I think I understand why you're doing this ("public" API vs "private" utility function), but it looks awkward to have the read and write functions in different modules. I'm not sure if we should add the read function to the API just because of that though.

Yeah the intent was that the utils is read-only but the other ones actually cause changes. I originally had them all n api.py but decided last minute to move it. I'm fine with either. Proposals:

Move both to utils.py (keep read/write together)

Move both to api.py (where mutations happen)

Keep as-is and document the rationale

I don't have too strong of an opinion either way, but 2. for the "main" TOML functions (top-level read and write) sounds the most sensible to me.

marcoesters · 2026-05-13T16:05:03Z

+    # 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 env_name := os.environ.get("MENUINST_DISTRIBUTION_NAME"):


Let's call it distribution_name to be consistent.

I'm wondering though if it makes sense to record all MENUINST_* environment variables here.

I'm just generally wondering how we would distinguish between those "global" parameters that should persist vs. parameters that should be recalculated during install-time. Maybe we need a separate command for that? menuinst --set-global-placeholder or something similar?

I think the generalization is interesting but comes with additional complexity, which I would propose we postpone until we have concrete use cases. Refactoring this wouldn't be difficult but I'm afraid we run into fixing something where we don't have any real use case (if that makes sense).
Also renamed the variable: d5324fb

I was thinking about #334 in particular. Or the environment placeholder in the Anaconda Prompt menus or the package version placeholders for spyder. Right now, this needs to be implemented with a sed-like workaround for Windows.

Setting variables like this via menuinst and then using placeholder syntax would be really convenient and remove all special casing. Installers would then just call _conda.exe menuinst --set-(global|local)-placeholder CUSTOM_PLACEHOLDER=something (or similar).

marcoesters · 2026-05-13T16:23:29Z

            "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"),


Since we already have MENUINST_BASE_PREFIX and the like, I wonder if we can generalize overwriting those placeholders instead of special-casing DISTRIBUTION_NAME (even though BASE_PREFIX and PREFIX are special cases).

Yeah my answer to this is similar to above. I think the idea is good but something I think we can do better once we have concrete use cases. I would also argue that MENUINST_BASE_PREFIX and MENUINST_PREFIX are runtime path overrides and are slightly different from DISTRIBUTION_NAME in this scenario.

marcoesters · 2026-05-13T16:30:38Z

+    # 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)


I think this illustrates the brittleness of the special casing a little bit. If we come up with other variables that must only be written into the base prefix, we have to keep making sure that they are in the correct place. Using the menuinst --set-global-placeholder would make make this more stable.

Yeah I think that's definitely a valid concern. I think in its current state with only one variable, it's doable, but if we add more I think definitely we can iterate on the design to something more robust. I've been trying to keep it simple and avoid over-engineering.

marcoesters · 2026-05-13T16:36:04Z

+    import tomli as tomllib
+
 logger = getLogger(__name__)
+MENUINST_TOML_SCHEMA_VERSION = 1


I suspect we will need to be more fine-grained than that?

You mean something like 1.0.0? Absolutely, I just made it simple (1) right now to illustrate the purpose.

I would use SchemaVer like we do for the menuinst schema: https://docs.snowplow.io/docs/api-reference/iglu/common-architecture/schemaver/

marcoesters · 2026-05-13T16:40:45Z

+    except Exception as e:
+        logger.warning("Failed to read menuinst.toml from %s: %s", toml_path, e)
+        return {}


I think we should actually error out here. The only way I can reasonably imagine that the file is invalid is if someone manipulated the file manually. In that case, returning {} may result in unexpected behavior.

See d5324fb

marcoesters · 2026-05-13T16:42:52Z

+    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.


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.

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.

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.

marcoesters · 2026-05-13T16:44:40Z

+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 is persisted to menuinst.toml."""


Suggested change

"""Test that MENUINST_DISTRIBUTION_NAME env var is persisted to menuinst.toml."""

"""Test that MENUINST_DISTRIBUTION_NAME env var persists in menuinst.toml."""

lrandersson · 2026-05-14T18:13:59Z

@marcoesters for the second round of review, see my responses below (trying to avoid spamming with email notifications):

Use SchemaVer format: df00f53
Move functions such that read/write are now in the same module (both in utils.py): df00f53
Regarding the custom placeholders: Add support for menuinst.toml #477 (comment)
I see the value and agree that it would be good to add, but to reduce scope creep of this PR which is intended primarily to resolve the MSI installer issue I propose that I create a separate issue as a follow-up to this one, since as far as I understand there are questions that we need to address that I don't think is ideal to resolve via review comments:

CLI interface design - --set-global-placeholder KEY=value vs --set-local-placeholder KEY=value
Storage format - How to distinguish global vs local placeholders in menuinst.toml? As an example: Currently menuinst.toml has distribution_name as a top-level key. For arbitrary placeholders, we need to decide:
- Maybe a flat structure? FOO = "value" at root level (risk: collision with future schema keys)
- or nested under a section? [placeholders] section with FOO = "value"
Resolution ordering - When resolving {{ FOO }} in a menu.json, what takes precedence?
- Environment variable MENUINST_FOO?
- Value in current prefix's menuinst.toml?
- Value in base_prefix's menuinst.toml?
Scope semantics - global = base_prefix only vs local = per-environment?
Let me know what you think, just to recall, I think the idea is great but I just think it should be done as part of a separate PR.

lrandersson self-assigned this May 11, 2026

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 11, 2026

conda-bot added this to 🔎 Review May 11, 2026

github-project-automation Bot moved this to 🆕 New in 🔎 Review May 11, 2026

lrandersson marked this pull request as ready for review May 11, 2026 17:44

lrandersson requested a review from a team as a code owner May 11, 2026 17:44

lrandersson marked this pull request as draft May 12, 2026 17:19

lrandersson marked this pull request as ready for review May 12, 2026 19:28

lrandersson added 7 commits May 12, 2026 15:28

work in progress

0ad397a

Add tests

cb1360d

work in progress

48eb399

Add todo

04e17e3

Ensure .toml file is written even if no shortcuts

606c8bc

Fix bug with source

a2fbeab

Add activate: False

a291190

lrandersson force-pushed the dev-ra-829 branch from 64d332c to a291190 Compare May 12, 2026 19:28

lrandersson commented May 12, 2026

View reviewed changes

lrandersson mentioned this pull request May 13, 2026

MSI: Add changes for new menuinst fix conda/constructor#1234

Draft

3 tasks

marcoesters reviewed May 13, 2026

View reviewed changes

marcoesters mentioned this pull request May 13, 2026

Expand the ability to specify parameters by is_base or not #334

Draft

3 tasks

Review fixes

d5324fb

lrandersson requested a review from marcoesters May 13, 2026 17:45

lrandersson added 2 commits May 13, 2026 13:59

Update test to account for platform differences

7aeb8e6

Add SchemaVer, move function

df00f53

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

	"""Test that MENUINST_DISTRIBUTION_NAME env var is persisted to menuinst.toml."""
	"""Test that MENUINST_DISTRIBUTION_NAME env var persists in menuinst.toml."""

Conversation

lrandersson commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Planned changes once on main

Checklist - did you ...

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoesters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lrandersson commented May 11, 2026 •

edited

Loading