bunfig: fall back to $HOME/.bunfig.toml when $XDG_CONFIG_HOME has none - #36486
bunfig: fall back to $HOME/.bunfig.toml when $XDG_CONFIG_HOME has none#36486Properrr wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesUser config resolution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bunfig/arguments.rs`:
- Around line 54-55: Add tests covering get_home_config_path and its resolver
for .bunfig.toml: verify the home-directory file is used when the XDG
configuration file is absent, and verify the XDG file takes precedence when both
exist. Include sibling configuration entry points required by the existing test
conventions and assert the resolved paths or contents observably.
In `@test/cli/install/npmrc.test.ts`:
- Line 208: Replace both Bun.$ mkdir invocations in
test/cli/install/npmrc.test.ts at lines 208 and 241 with the filesystem API's
recursive mkdir operation, preserving the existing homeDir and xdgDir directory
setup behavior at each site.
- Line 220: Update both child environment definitions in
test/cli/install/npmrc.test.ts at lines 220-220 and 254-254 to set USERPROFILE
to homeDir alongside HOME, ensuring the fallback and precedence tests use the
fixture profile on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b0bab32-48de-4ed0-bab6-a786893006b5
📒 Files selected for processing (3)
src/bunfig/arguments.rssrc/install/PackageManager.rstest/cli/install/npmrc.test.ts
|
Triage note: this is the PR we are keeping for #23128. #36289 was a robobun PR with the same What I checked, with your two source hunks applied on top of
One thing the branch needs: Two optional ideas from the closed PRs, take them or leave them:
#34987 (loading the global bunfig for runtime commands) overlaps this PR on the fallback helper but fixes a different problem and stays open; whichever lands second rebases over the other. |
`get_home_config_path` returned `$XDG_CONFIG_HOME/.bunfig.toml` whenever the variable was set, so exporting `XDG_CONFIG_HOME` (which many Linux setups do by default) silently stopped `$HOME/.bunfig.toml` from being read at all. Probe `$XDG_CONFIG_HOME/.bunfig.toml` and use it only when the file is actually there, otherwise fall back to `$HOME/.bunfig.toml`. Only one user-level file is ever loaded, as before. Rebased onto main after the same fix for the user-level `.npmrc` landed in PackageManager::init (oven-sh#36289); this commit now only carries the `.bunfig.toml` half. Fixes oven-sh#23128
… lookup Read both variables with get_not_empty() and fold the lookup back into get_home_config_path, which is its only caller now that the .npmrc side lives in PackageManager::init. Cover the four lookup cases through `bun pm cache`, next to the matching user .npmrc cases: the XDG file wins when present, XDG set without a file falls back to $HOME, XDG unset, and XDG set to an empty string.
90c6522 to
ea40a60
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@Properrr, a maintainer asked for this PR to be rebased onto main now that #36289 has landed (correction to my earlier note here: #36289 was merged, not closed, so the
Verification on the rebased branch: the two fallback cases fail on the released build and all four pass with the fix ( |
|
Thanks @Properrr. This change has been carried over to #38313, which is the same diff on an in-repo branch so it can run through CI. Your commit is kept as the first commit there, so the authorship stays with you; the second commit adds the empty-variable handling and the tests. Closing this one in favor of #38313. #23128 stays open until that lands. |
Fixes #23128 (the
.bunfig.tomlhalf; the.npmrchalf landed in #36289).Rebased onto main by @robobun at a maintainer's request: #36289 already changed the
PackageManager.rshunk this PR used to touch, so this PR now only carries the.bunfig.tomlside and uses the same rule as #36289. The original description is kept at the bottom.Problem
XDG_CONFIG_HOMEexported (GitHub Actions ubuntu runners and most Linux desktops export it),~/.bunfig.tomlis never read bybun install,bun add,bun pm ...and the other commands that load the global bunfig, whether or not$XDG_CONFIG_HOME/.bunfig.tomlexists.get_home_config_pathinsrc/bunfig/arguments.rsreturned$XDG_CONFIG_HOME/.bunfig.tomlas soon as the variable was set, without checking that the file exists.XDG_CONFIG_HOME=""had the same effect, since the empty value still counted as set.$XDG_CONFIG_HOME/.bunfig.tomlor$HOME/.bunfig.toml, and.npmrchas used that rule since install: fall back to $HOME/.npmrc when $XDG_CONFIG_HOME is set #36289.Fix
get_home_config_pathuses$XDG_CONFIG_HOME/.bunfig.tomlonly when that file exists (bun_sys::exists_z), otherwise$HOME/.bunfig.toml. Both variables are read withget_not_empty(), so an empty string counts as unset. Still at most one global file is loaded, and a bunfig that was deliberately placed underXDG_CONFIG_HOMEkeeps winning.PackageManager.rsis untouched; the user-level.npmrclookup there already works this way after install: fall back to $HOME/.npmrc when $XDG_CONFIG_HOME is set #36289.test/cli/install/npmrc.test.ts,describe("global .bunfig.toml lookup"), next to theuser .npmrc lookupcases from install: fall back to $HOME/.npmrc when $XDG_CONFIG_HOME is set #36289. Each case points the candidate files at differently named cache directories and reads back which onebun pm cacheprints (offline, no registry involved):$XDG_CONFIG_HOME/.bunfig.tomlpresent: it winsXDG_CONFIG_HOMEset, no file there:$HOME/.bunfig.tomlis used (fails on main)XDG_CONFIG_HOMEunset:$HOME/.bunfig.tomlis usedXDG_CONFIG_HOME="":$HOME/.bunfig.tomlis used (fails on main)USE_SYSTEM_BUN=1 bun test test/cli/install/npmrc.test.ts -t "bunfig.toml lookup"(bun 1.4.0): 2 pass, 2 fail, both failures printing the default cache directory instead of the one from$HOME/.bunfig.tomlbun bd test test/cli/install/npmrc.test.ts: 35 pass (whole file)bun bd test test/cli/install/minimum-release-age.test.ts -t "global bunfig": 2 pass (existing tests that setXDG_CONFIG_HOMEto a directory containing a.bunfig.toml)Background
.bunfig.tomlthat install-family commands (andbunx) load before the project's./bunfig.toml; the local file's settings override it. Runtime commands such asbun rundo not load it; that is unchanged here (bunfig: load global ~/.bunfig.toml for runtime commands; fall back past $XDG_CONFIG_HOME #34987 proposed changing it and is closed in favour of this PR).XDG_CONFIG_HOMEis the freedesktop variable for a user's config directory, usually~/.config. Bun also looks for.bunfig.tomldirectly inside it, so on a machine that merely exports the variable the lookup used to stop at a path where nothing exists.bun pm cacheprints the install cache directory, which[install] cache = "..."in any loaded bunfig overrides, which is why the tests use it to see which file was read.BUN_INSTALL_CACHE_DIRwould take precedence over bunfig and CI exports it, so the tests drop it from the environment.Original description (before the rebase)
What does this PR do?
Fixes #23128.
bun installresolved its user-level.npmrcwith:That picks one directory or the other. Because many Linux distros and shells export
XDG_CONFIG_HOMEby default, users who never opted into XDG would have$HOME/.npmrcsilently ignored — scoped registries and auth tokens dropped with no diagnostic.$HOME/.bunfig.tomlhad the identical bug inget_home_config_path.This probes
$XDG_CONFIG_HOME/<name>and only uses it when the file is actually there, otherwise falling back to$HOME/<name>.Two notes on the approach:
userconfigmodel and avoids introducing a new config layer with new precedence rules to reason about.The resolution is shared between
.npmrcand.bunfig.tomlviabun_bunfig::arguments::user_config_path, since both sites had the same bug.bun_installalready depends onbun_bunfig, so no new edges. Cost is one extrastatat startup, and only when both env vars are set.How did you verify your code works?
Two tests added to
test/cli/install/npmrc.test.ts. Both assert offline viabun pm cachereadingcache=out of the resolved.npmrc, so there's no registry or network dependency:falls back to $HOME/.npmrc when $XDG_CONFIG_HOME has noneprefers $XDG_CONFIG_HOME/.npmrc over $HOME/.npmrcUSE_SYSTEM_BUN=1can't validate this file — it fails at import onbun:internal-for-testing, which release builds don't ship. So I verified the tests are real by reverting the source change, rebuilding, and re-running:The fallback test fails without the fix; the precedence test passes without it, confirming it guards existing behavior rather than the new path. With the fix restored:
The bunfig suites are included because the shared helper also governs
.bunfig.tomlresolution.cargo clippy -p bun_bunfig -p bun_install --no-depsis clean.Not covered: I only exercised this on Linux. The code path is platform-independent, and the tests now set
USERPROFILEalongsideHOMEsinceenv_var::HOMEreadsUSERPROFILEon Windows — but I have not run them on macOS or Windows.Correction to an earlier version of this description: I originally wrote that
HOMEis unset on Windows so the(None, ...)arms preserve existing behavior there. That was wrong —env_var::HOMEis defined asposix = "HOME", windows = "USERPROFILE"(src/bun_core/env_var.rs:146), so it is populated on Windows too. The practical impact is small becauseXDG_CONFIG_HOMEis rarely set on Windows, but when it is, resolution changes there the same way it does on Linux. Thanks to @coderabbitai for the nudge that surfaced this.