Conversation
Install hegel-core into $XDG_CACHE_HOME/hegel/versions/<version>/venv instead of the project-local .hegel/venv directory. Falls back to ~/Library/Caches/hegel on macOS and ~/.cache/hegel on Linux. Adds mkdir-based cross-process file locking so parallel test processes don't race on installation. Server logs remain in project-local .hegel/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…elease The eprintln calls were writing to stderr instead of the install log. Remove them rather than redirecting, since the log file already captures uv output. Also change release_file_lock to expect instead of silently discarding errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DRMacIver
changed the title
The test_missing_uv_error_message test was removing .hegel/ to clear the cached install, but hegel-core now lives in the XDG cache directory. Fix by pointing XDG_CACHE_HOME at an empty temp dir so no cached install is found. Also fix a formatting diff caught by CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Liam-DeVoe
left a comment
Liam-DeVoe
left a comment
There was a problem hiding this comment.
I'm broadly okay with this. The installation logic is now complicated enough that I'd like it to live in a src/installation.rs file. This will help with using it as a reference for other hegel libraries as well.
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::sync::Mutex as StdMutex; | ||
| use tempfile::TempDir; | ||
|
|
||
| // Environment variable tests must run serially since env vars are process-global. | ||
| // Use into_ok() to recover from poison (the should_panic test poisons the mutex). | ||
| static ENV_LOCK: StdMutex<()> = StdMutex::new(()); | ||
|
|
||
| fn lock_env() -> std::sync::MutexGuard<'static, ()> { | ||
| ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()) |
There was a problem hiding this comment.
these tests should live in a tests file to keep the actual code file clean
| /// | ||
| /// Resolution order: | ||
| /// 1. `$XDG_CACHE_HOME/hegel` if `XDG_CACHE_HOME` is set | ||
| /// 2. `~/Library/Caches/hegel` on macOS | ||
| /// 3. `~/.cache/hegel` on other platforms | ||
| fn hegel_cache_dir() -> PathBuf { |
There was a problem hiding this comment.
I'd actually prefer ~/.cache/hegel even on macos. Yes, this is strictly speaking not the macos idiomatic location for caches. But it eliminates a code path for us, and plenty of tools use ~/.cache on macos, among them pip, uv, and gh.
I would say ~/.cache is actually the more standard location for coding ecosystem tool caches, while ~/Library/Caches is standard for application-level caches.
There was a problem hiding this comment.
Sure, seems reasonable.
| let home = std::env::var("HOME").expect( | ||
| "Could not determine home directory: HOME is not set. \ | ||
| Set XDG_CACHE_HOME or HEGEL_SERVER_COMMAND to work around this.", | ||
| ); |
There was a problem hiding this comment.
use https://doc.rust-lang.org/std/env/fn.home_dir.html instead, which gets us windows support
| // Acquire cross-process file lock. | ||
| let lock_dir = version_dir.join(".install-lock"); | ||
| acquire_file_lock(&lock_dir)?; | ||
| let result = do_install(&venv_dir, &version_file, &hegel_bin, &install_log); |
There was a problem hiding this comment.
.install-lock is actually a directory here, making this a lockdir, not a lockfile. I'd prefer to use a file here
There was a problem hiding this comment.
I think Claude had some argument as to why a lockdir was better (atomic cross platform or something?) but I don't remember what it was. I'll investigate and see whether there's an actually compelling reason.
2 participants
Install hegel-core into $XDG_CACHE_HOME/hegel/versions//venv (or a suitable fallback for XDG_CACHE_HOME if not set) instead of the project-local .hegel/venv
Fixes #108 (more or less. Technically one can still trigger the bad behaviour if you work really hard, but it requires you to be doing something really screwy)