Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the depends field in the tool registry, moving it from a global tool property to a per-backend option to improve install-order scheduling and prevent misuse. The changes span registry configurations, documentation, the JSON schema, and the internal logic for parsing and promoting backend options. A critical issue was identified in src/registry.rs where the presence of bracketed options in the backend string prevents the retrieval of registry-defined defaults, and a fix was proposed to strip these options before lookup.
Greptile SummaryThis PR replaces registry-level
Confidence Score: 5/5The change is safe to merge: removing registry-level depends narrows the scope of auto-injected install dependencies to the correct layers, and test.tools is strictly scoped to mise test-tool with no effect on normal installs. All thirteen registry depends removals are either covered by backend-native dependency declarations (vfox PLUGIN.depends, core:elixir built-in Erlang handling) or intentionally deferred. The new test.tools path in build.rs reads the TOML array directly without any serialization round-trip. The subprocess test path re-reads from the registry, so test.tools is correctly applied in both parallel and in-process modes. No files require special attention; the most sensitive path (src/backend/mod.rs) is a straightforward four-line deletion with no regressions in the surrounding dependency logic. Reviews (7): Last reviewed commit: "Merge branch 'main' into feat/registry-i..." | Re-trigger Greptile |
fc2ebd9 to
0d0ca20
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
CI is complete on The registry-specific checks are green now:
The remaining red status is from unrelated e2e shards:
These are outside the registry/test.tools path touched here. The logs also show repeated Docker temp cleanup permission errors such as This comment was generated by an AI coding assistant. |
Summary
mainafter registry: remove bashly asdf fallback #9578 and registry: use github backend for rebar #9576 landed, then fast-forwarded over the latestmainlockfile maintenance merge.dependsfrom generatedRegistryTool, the registry schema, and backend dependency expansion.test.toolssupport for registry tests. This is consumed only bymise test-tool; it does not affect normal installs.Why registry
dependsis not neededRegistry
dependsmixed unrelated concerns: install ordering, runtime companions, and tools needed only for registry tests. That made shorthand registry entries apply dependencies too broadly across every backend.The remaining valid dependency paths are covered elsewhere:
PLUGIN.depends; mise already reads those through the vfox backend.depends, but rebar was the only registry entry still relying on that path, and registry: use github backend for rebar #9576 moved it togithub:erlang/rebar3. New asdf plugins are not being added to the registry.test.cmdbelong intest.tools, which is consumed only bymise test-tooland does not affect normal installs.Registry file changes (
registry/*.toml)depends = ["java"]; addedtest.tools = ["java"]because the vendored vfox metadata notes Java 11+ andsdkmanagershells out tojava.depends = ["erlang"];core:elixirhandles Erlang as a backend dependency.depends = ["java"]; its Linux registry asset is a native executable, and the Docker probe passed without adding Java.depends = ["java"]; addedtest.tools = ["java"]because the Gradle launcher requiresjavaforgradle -V.depends = ["kotlin"]; addedtest.tools = ["kotlin"]after Docker/registry CI showedkscript --versionfails without Kotlin.depends = ["java"]; its registry test remains disabled.depends = ["kubectl"];mise x kubecolor -- kubecolor --kubecolor-versionpassed in the e2e Docker image with nokubectlon PATH, so notest.toolsentry is needed.depends = ["python"]; addedtest.tools = ["python"]because the vendored vfox post-install hook creates a venv withpython3/python, and the test should not rely on system Python.depends = ["python"]; addedtest.tools = ["python"]because the aquapipx.pyzlauncher uses#!/usr/bin/env python3.depends = ["erlang"]; #9576 moved this registry entry togithub:erlang/rebar3; addedtest.tools = ["erlang"]becauserebar3 --versionneeds the Erlang runtime.depends = ["java"]; the conda backend installs OpenJDK in the package environment and the Docker probe passed without registrytest.tools.depends = ["java"]; no registry test command is present.depends = ["kubectl"];tridentctl version --clientpassed in Docker withoutkubectl.vfox notes
vfox-android-sdkis vendored incrates/vfox/embedded-plugins/; its metadata says Java 11+ is required, and the installedsdkmanagerlauncher checksJAVA_HOME/java. Its post-install hook only uses common shell utilities (mv,mkdir,rm,chmod), which are not registry tools.vfox-pipenvis vendored incrates/vfox/embedded-plugins/; its post-install hook explicitly searches forpython3/python, creates a venv, then runs pip. Utility shell usage such aspwd,mkdir, andchmodis intentionally ignored.vfox-gradleis not vendored undercrates/vfox/embedded-plugins/; the registry test still needs Java because the Gradle launcher itself invokesjava.Pipenv note
mise-plugins/vfox-pipenv/pull/1must be merged forpipenvto work properly through the vfox backend. GitHub Actions and the e2e image currently have Python available, but the registry should not rely on system Python:PLUGIN.dependsonly declares plugin execution order, so without a managed Python declaration pipenv falls back to whatever system Python is available.Validation
taplo fmt --check registry/android-sdk.toml registry/gradle.toml registry/pipenv.toml registry/pipx.toml registry/kscript.toml registry/rebar.tomlgit diff --checkghcr.io/jdx/mise:e2emise x <tool> -- <test.cmd>forandroid-sdk,google-java-format,gradle,kscript,kubecolor,pipenv,pipx,sbt, andtridentctl