feat: XDG directory layout via VpDirs - #2346
Conversation
✅ Deploy Preview for viteplus-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
b8069a5 to
2f8be03
Compare
forehalo
changed the title
08b7589 to
4e2abdf
Compare
forehalo
changed the title
|
@liangmiQwQ Your refactor needs to wait until this merge is completed first, as the changes are expected to be quite significant. |
78a23b6 to
a433ed4
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
402f6db to
6b26e69
Compare
Capture the resolution design, grandfathering of existing ~/.vite-plus installs, installer alignment, and follow-ups for VP_HOME cleanup and migrate-on-upgrade.
Replace get_vp_home / the monolithic home module with VpDirs: category roots (bin, data, cache, config, state) come from an ordered resolution chain in dirs/resolution.rs (legacy Exist-gated grandfathering, VP_* and XDG Set overrides, then platform defaults), and first-level data subdirs (current, js_runtime, package_manager, packages, bins) are pure joins. Files and deeper trees stay with their features. Wire EnvConfig so test_guard/for_test_with_home isolate install roots without process-env leaks. Groundwork for voidzero-dev#827.
Switch global CLI, shims, js_runtime, package_manager, and related helpers off the old home path helpers onto VpDirs. Inject VP_BIN_DIR/VP_DATA_DIR/ VP_CACHE_DIR into JS children under the split layout; teach hooks and org-tarball to honor those roots. implode, env setup/doctor, and snapshot fixtures understand split vs legacy layouts without binary self-location (pin VP_HOME / isolated roots explicitly in tests).
Fresh installs land versions under the data dir and shims under the bin dir (XDG/platform defaults via VpDirs). install.sh, install.ps1, vp-setup, Dockerfile, trampoline, and install-global-cli follow the same resolution chain as the CLI; an existing ~/.vite-plus or explicit VP_HOME/--install-dir keeps the legacy monolithic root. Document the installer env surface (VP_HOME deprecated) and cover split plus legacy upgrade/implode paths in standalone install CI.
6b26e69 to
ca9dcca
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca9dcca689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let split = base.join("data").join("current").join("bin").join("vp.exe"); | ||
| split.is_file().then_some((split, false)) |
There was a problem hiding this comment.
When VP_DATA_DIR or VP_BIN_DIR places the split data and bin directories under different parents, the Windows trampoline only searches for vp.exe at the fixed sibling path <parent-of-bin>\data\current\bin\vp.exe. The installer supports these overrides independently, so every generated vp.exe, node.exe, and other trampoline exits before spawning the real CLI for such an installation, even if the overrides remain set. The trampoline needs to locate the configured data directory rather than assuming the default sibling layout.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| if ($env:VP_DATA_DIR) { | ||
| $InstallDir = $env:VP_DATA_DIR |
There was a problem hiding this comment.
When a Windows user sets a relative VP_DATA_DIR, this installer accepts it and installs the payload relative to its working directory, while VpDirs deliberately treats relative VP_*_DIR values as unset. Consequently the installed CLI resolves its data from the platform default instead of the location populated by the installer; VP_BIN_DIR has the same mismatch below. Validate these overrides as rooted paths and fall back to the defaults when they are relative.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| // (legacy layout) or `<base>\data\current\bin\vp.exe` (split layout). | ||
| #[cfg(windows)] | ||
| const VP_USE_CMD_CONTENT: &str = "@echo off\r\nset VP_ENV_USE_EVAL_ENABLE=1\r\nset VP_HOME=%~dp0..\r\nfor /f \"delims=\" %%i in ('%~dp0..\\current\\bin\\vp.exe env use %*') do %%i\r\nset VP_ENV_USE_EVAL_ENABLE=\r\n"; | ||
| const VP_USE_CMD_CONTENT: &str = "@echo off\r\nset VP_ENV_USE_EVAL_ENABLE=1\r\nset \"VP_EXE=%~dp0..\\current\\bin\\vp.exe\"\r\nif not exist \"%VP_EXE%\" set \"VP_EXE=%~dp0..\\data\\current\\bin\\vp.exe\"\r\nfor /f \"delims=\" %%i in ('%VP_EXE% env use %*') do %%i\r\nset VP_ENV_USE_EVAL_ENABLE=\r\n"; |
There was a problem hiding this comment.
For a Windows split install using independent VP_BIN_DIR and VP_DATA_DIR values, vp-use.cmd searches only the legacy sibling and default <base>\data layouts, so the documented vp-use 20 flow invokes a nonexistent executable even though the main installation is otherwise valid. Render the resolved VpDirs::current_dir() location into this wrapper, or pass enough configuration for it to find the custom data root.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| elif [ -n "\${HOME-}" ]; then | ||
| __vp_bin="$HOME/.local/bin" |
There was a problem hiding this comment.
On a default Windows split installation, generated Git hooks reach this branch with neither VP_BIN_DIR nor VP_HOME set and append $HOME/.local/bin, but Windows installs the shims under %LOCALAPPDATA%\vite-plus\bin. Hooks launched by an IDE or other process that has not inherited the installer-updated User PATH therefore cannot find vp, managed Node, or other Vite+ shims. The fallback needs to use the Windows platform bin location rather than the Unix split default.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| if let Some(home) = EnvConfig::get().user_home { | ||
| return Some(home); |
There was a problem hiding this comment.
On Windows when HOME is set to a different directory from USERPROFILE, this returns the HOME value cached by EnvConfig, so the resolution chain never checks %USERPROFILE%\.vite-plus. In particular, vp-setup.exe can miss an existing legacy installation and create a second split installation even though the PowerShell installer correctly grandfathers %USERPROFILE%\.vite-plus. Select the platform-specific home variable on Windows, or use BaseDirs, before applying the legacy existence check.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
| let mut failed = false; | ||
| for dir in &self.dirs { | ||
| if !dir.as_path().exists() { | ||
| continue; | ||
| } | ||
| if remove_vite_plus_dir(dir).is_err() { |
There was a problem hiding this comment.
When a split-layout user points VP_DATA_DIR or VP_CACHE_DIR at a shared or critical directory—for example VP_DATA_DIR=$HOME or VP_CACHE_DIR=/tmp—vp implode --yes places that override directly in self.dirs and recursively deletes the entire directory. Unlike the shared bin cleanup, there is no ownership or safe-path check before remove_dir_all, so a valid absolute override can cause extensive unrelated data loss. Reject unsafe roots/home directories or remove only Vite+-owned contents for overridden category paths.
Useful? React with 👍 / 👎.
| dir.as_path().exists() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| macro_rules! resolutions { |
There was a problem hiding this comment.
If $HOME/.vite-plus or ./.vite-plus exists as a regular file or dangling-install artifact rather than a directory, this existence gate still selects the legacy layout. All resolved paths then become children of that file, so configuration, runtime downloads, and shim setup fail, while both installers correctly use directory-only checks and would have selected the split layout. Use is_dir() for legacy-root gates so the CLI and installers resolve the same layout.
Useful? React with 👍 / 👎.
|
IMO, I would be a little bit curious about the benefit for this move. The current For example, I help maintain the Vite+'s global package installations in several, I'm concerned that using I'm not against this direction, I just want to understand the motivation and the problem it solves. If it does help Vite+ improve, then now is really a good time to implement it (before RC). |
| | ------------------------------------------------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | **`VP_HOME`** (deprecated) | Set | When set, pins the **legacy monolithic mapping** under that root for all categories. | | ||
| | **`~/.vite-plus`** | Exist | When that directory exists, use the legacy mapping under it. | | ||
| | **`./.vite-plus`** | Exist | When present in the process cwd, same legacy mapping (project-local / tests). | |
There was a problem hiding this comment.
This compatibility is limited to the voidzero-dev/vite-plus repo, right? Normal projects shouldn't be compatible with this logic.
3 participants
Close #827
Summary
Centralize on-disk paths in
vp_shared::VpDirsand default fresh installs to the split XDG/platform layout. Existing default installs under~/.vite-plusremain on the legacy monolithic root for now (grandfathered via existence-gated resolution). Automatic layout migration is out of scope here and tracked as follow-up work.Resolution (high level)
Ordered sources (per category): deprecated
VP_HOME(custom full-root pin) → existing~/.vite-plus/./.vite-plus(exist-gated legacy mapping) →VP_BIN_DIR/VP_DATA_DIR/VP_CACHE_DIR→XDG_*→ platform defaults.~/.vite-plus)~/.local/bin<root>/bin~/.local/share/vite-plus<root>~/.cache/vite-plus<root>/cache~/.config/vite-plus<root>~/.local/state/vite-plus<root>Windows split uses
%LOCALAPPDATA%\vite-plus\{bin,data,cache,state}and%APPDATA%\vite-plusfor config. RelativeVP_*/XDG_*values are treated as unset.Commits (review order)
docs(rfc): document split directory layout via VpDirsrfcs/directory-layout.mddescribing the resolution chain, grandfathering, installer alignment, and follow-ups (VP_HOMEcleanup, migrate-on-upgrade).env-command.md) are left unchanged.feat(shared): introduce VpDirs with strategy-gated path resolutionget_vp_home/home.rswithVpDirs+dirs/resolution.rsstrategy chain.EnvConfig/ env vars so tests isolate layout viatest_guard/for_test_with_homewithout process-env leaks.AGENTS.mdpointer to the central path API.refactor: migrate call sites from get_vp_home to VpDirsjs_runtime, package manager, implode, env setup/doctor, and related helpers ontoVpDirs.VP_BIN_DIR/VP_DATA_DIR/VP_CACHE_DIRinto JS children when unset; hooks / org-tarball honor those roots.VpDirs).~/.vite-plusbeforebootstrap-cliso existing snapshot/e2e paths keep finding the global binary until layout cleanup lands.feat(install): default installers to the split XDG layoutinstall.sh/install.ps1/vp-setup/ trampoline / Dockerfile /install-global-clishare the same resolution chain as the CLI.~/.vite-plusor explicitVP_HOME/--install-dir→ legacy monolithic root.VP_HOMEdeprecated) and standalone-install CI coverage for split + legacy upgrade/implode.User impact
~/.vite-plusVP_HOME~/.local/binneeds to be on PATHFollow-up
VP_HOMEusage in the repo (especially PTY snapshot tests); preferVP_*_DIR/ XDGvp upgrade, migrate default~/.vite-pluscontents into splitVP_*_DIR/XDG_*/ platform dirs and remove the legacy rootTest plan
cargo check/ unit tests fordirsresolution and layout helpersVP_HOME/ disable shared seed where fakes are used)install.sh/install.ps1)install-global-clifor_test_with_homebin mappingtest-standalone-install(released + local split/legacy-upgrade jobs)~/.vite-plusstill works without moving