From 0bfeb1ebbe235314ef1ad6a9d4f5af21344791bc Mon Sep 17 00:00:00 2001 From: Myles Anderson <135627999+myles332@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:28:22 -0700 Subject: [PATCH] Find the LaTeX toolchain on the user's PATH, and recommend Tectonic (#227) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Give shell_env the one PATH search every lookup uses `find_on_path` sat in `harness::detect` as `pub(super)`, so anything outside the harness registry that needed a shell-PATH lookup wrote the loop again — `find_opencode` and `resolves_on_path` both had a copy, the former commented "Mirrors `find_on_path`". Move it to `shell_env`, which already owns `search_path()` and whose module doc is about which PATH orx searches, and point all four call sites at it. The shared lookup now also drops *relative* PATH entries, not just empty ones. A relative entry names no fixed directory: it resolved against orx's own cwd, which for a Finder-launched bundle is never where a user's tool lives, and the child that later ran it would resolve the same name somewhere else again. `other_orx_on_path` in install_cli.rs keeps its own loop — it needs every candidate and canonicalizes them, so it is a different search. Co-Authored-By: Claude Opus 5 * Find the LaTeX toolchain on the user's PATH, not launchd's A `.tex` file opened in the macOS app reported "No LaTeX toolchain found on PATH" on a machine with tectonic installed. The bundle is started by launchd with `PATH=/usr/bin:/bin:/usr/sbin:/sbin`, where no TeX lives, and `latex.rs` was the one lookup still resolving against the process environment — the harnesses had gone through `shell_env`'s probed shell PATH for exactly this reason. Verified against a bundle launched with launchd's environment: `/api/latex/engine` returns `tectonic` where the installed build returns null. Finding the tool is not enough on its own: latexmk finds the engine, biber and bibtex on PATH itself, so the child is handed the same PATH we found it on. The tool is spawned by its absolute path but symlinks are left unresolved — TeX picks its format file from the name it was invoked as, so canonicalizing `pdflatex` to `pdftex` would silently run plain TeX. `on_path` no longer spawns ` --version` to decide. It costs a probe that a present-but-unrunnable binary would have caught, and buys back five subprocesses on every `.tex` tab open. Co-Authored-By: Claude Opus 5 * Point a user with no LaTeX at Tectonic rather than MacTeX The no-engine hint led with MacTeX and offered `brew install --cask mactex` as the command to copy — a multi-gigabyte install for someone who just wants to see their paper. Tectonic is one self-contained binary that fetches each document's packages, so it is the install a user can actually finish, and it is what the copyable command now installs. The distribution stays named second, because Tectonic runs only XeTeX. The hint says so: it is the last screen that can, since the hint disappears the moment an engine exists and the amber substitution note only fires for a document that names its engine explicitly. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- src/invocation.rs | 7 +--- src/local/harness/claude.rs | 4 +- src/local/harness/codex.rs | 5 ++- src/local/harness/detect.rs | 10 ----- src/local/latex.rs | 83 +++++++++++++++++++++++++++---------- src/local/opencode.rs | 11 +---- src/local/shell_env.rs | 46 +++++++++++++++++++- 7 files changed, 114 insertions(+), 52 deletions(-) diff --git a/src/invocation.rs b/src/invocation.rs index 83398ff1..47b06142 100644 --- a/src/invocation.rs +++ b/src/invocation.rs @@ -43,12 +43,7 @@ pub fn no_run_command(project_id: &str) -> String { } fn resolves_on_path(bin: &str) -> bool { - let Some(paths) = crate::local::shell_env::search_path() else { - return false; - }; - std::env::split_paths(&paths) - .filter(|dir| !dir.as_os_str().is_empty()) - .any(|dir| dir.join(bin).is_file()) + crate::local::shell_env::find_on_path(bin).is_some() } fn quote_for_shell(path: &str) -> String { diff --git a/src/local/harness/claude.rs b/src/local/harness/claude.rs index 8a5db340..0d54b19a 100644 --- a/src/local/harness/claude.rs +++ b/src/local/harness/claude.rs @@ -30,8 +30,7 @@ use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use super::detect::{ - bin_version, find_on_path, nonempty_str, parse_version, read_json, HarnessAuthState, - HarnessInfo, ModelInfo, + bin_version, nonempty_str, parse_version, read_json, HarnessAuthState, HarnessInfo, ModelInfo, }; use super::options::{ HarnessOptions, OptionChoice, PermissionMode, PlanActivation, REASONING_DEFAULT_ID, @@ -46,6 +45,7 @@ use crate::local::chat::{ }; use crate::local::claude::{SpawnConfig, SpawnSpec, TurnEvent}; use crate::local::opencode::ensure_playbook; +use crate::local::shell_env::find_on_path; /// FALLBACK model list, used only when the `list_models` control request fails /// (a CLI too old to answer it, or a spawn/timeout failure). The primary source diff --git a/src/local/harness/codex.rs b/src/local/harness/codex.rs index b94f8da2..9077b697 100644 --- a/src/local/harness/codex.rs +++ b/src/local/harness/codex.rs @@ -38,8 +38,8 @@ use tokio::io::{AsyncBufReadExt, BufReader}; use tokio::process::Command; use super::detect::{ - bin_version, find_on_path, jwt_payload, nonempty_str, parse_version, read_json, - resolve_symlinks, title_case, HarnessInfo, ModelInfo, + bin_version, jwt_payload, nonempty_str, parse_version, read_json, resolve_symlinks, title_case, + HarnessInfo, ModelInfo, }; use super::options::{ resolve_reasoning, HarnessOptions, OptionChoice, PermissionMode, PlanActivation, @@ -57,6 +57,7 @@ use crate::local::chat::{ }; use crate::local::codex::{CodexClient, JsonRpcError, ServerReqKind, TurnEvent}; use crate::local::opencode::ensure_playbook; +use crate::local::shell_env::find_on_path; use crate::store::{Store, StoredChatMessage}; // FALLBACK model table, used only when the app-server catalog is unreachable diff --git a/src/local/harness/detect.rs b/src/local/harness/detect.rs index 0b76e117..5ac8f73f 100644 --- a/src/local/harness/detect.rs +++ b/src/local/harness/detect.rs @@ -166,16 +166,6 @@ impl HarnessInfo { } } -pub(super) fn find_on_path(bin: &str) -> Option { - let paths = crate::local::shell_env::search_path()?; - std::env::split_paths(&paths) - // An empty component (`PATH=":/usr/bin"`) means cwd — never a place to - // pick up a binary we are about to execute. - .filter(|dir| !dir.as_os_str().is_empty()) - .map(|dir| dir.join(bin)) - .find(|c| c.is_file()) -} - /// Dereference symlinks to the real installed binary. Installers commonly drop /// a lone symlink into `~/.local/bin`, but some CLIs locate sibling helper /// executables relative to the path they were *invoked as*, without resolving diff --git a/src/local/latex.rs b/src/local/latex.rs index 50ef3d9d..501d23a8 100644 --- a/src/local/latex.rs +++ b/src/local/latex.rs @@ -19,6 +19,7 @@ use std::sync::mpsc; use std::time::{Duration, Instant}; use crate::error::{anyhow, Result}; +use crate::local::shell_env::{find_on_path, search_path}; /// The TeX engine a document is written for. Overleaf's default is pdfLaTeX and /// so is ours; a document says otherwise with a `% !TeX program` line. @@ -133,15 +134,27 @@ pub struct Compilation { pub log: String, } -/// True when a binary answers `--version` on this machine's PATH. +/// True when a file of that name sits on the shell's PATH — not the process's, +/// which in app mode is launchd's and has no TeX on it. That it runs is not +/// checked; `find_engine` probes on every `.tex` tab and spawning five engines +/// to ask their versions cost more than the case it caught. fn on_path(binary: &str) -> bool { - Command::new(binary) - .arg("--version") - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .is_ok_and(|status| status.success()) + find_on_path(binary).is_some() +} + +/// A TeX tool, with the shell's PATH in its environment: latexmk finds the +/// engine, biber and bibtex on PATH, so resolving latexmk alone is not enough. +fn tex_command(binary: &str) -> Command { + // biber is never probed — it is chosen from what a pass wrote — so a bare + // name here is what makes a machine without it fail as ENOENT at spawn. + let mut command = match find_on_path(binary) { + Some(path) => Command::new(path), + None => Command::new(binary), + }; + if let Some(paths) = search_path() { + command.env("PATH", paths); + } + command } /// Pick how to compile a document written for `program`. Split from the PATH @@ -191,8 +204,9 @@ pub fn find_engine() -> Option<&'static str> { .find(|binary| on_path(binary)) } -/// Guidance for the no-engine case. A full distribution comes first because it -/// is what matches Overleaf — every engine, biber, and the whole of CTAN. +/// Guidance for the no-engine case. Tectonic comes first because it is the one +/// a user can finish, and its XeTeX-only trade is named here because this is +/// the last screen that can say so — the hint is gone once an engine exists. pub fn install_hint() -> String { let distribution = if cfg!(target_os = "macos") { "MacTeX" @@ -200,9 +214,10 @@ pub fn install_hint() -> String { "TeX Live" }; format!( - "No LaTeX toolchain found on PATH. Install {distribution} for the same \ - engines and packages Overleaf runs, or Tectonic for a smaller \ - XeTeX-only setup that needs no distribution." + "No LaTeX toolchain found on PATH. Install Tectonic — one self-contained \ + binary that fetches each document's packages, though it runs only XeTeX. \ + For every engine and package, install {distribution} instead, which is \ + what Overleaf runs." ) } @@ -210,7 +225,7 @@ pub fn install_hint() -> String { /// known to work — a wrong command pasted into a terminal is worse than none. pub fn install_command() -> Option<&'static str> { if cfg!(target_os = "macos") { - Some("brew install --cask mactex") + Some("brew install tectonic") } else { None } @@ -298,7 +313,7 @@ fn run_bibliography( // cwd is the aux dir (bibtex refuses to write outside it), so `.` no longer // means the paper's directory — both search paths have to say so. let search = format!("{}:", source_dir.to_string_lossy()); - let mut command = Command::new(tool); + let mut command = tex_command(tool); command .arg(stem) .current_dir(outdir) @@ -353,7 +368,7 @@ struct Run { } fn run_pass(binary: &str, dir: &Path, args: &[String]) -> Result { - let mut command = Command::new(binary); + let mut command = tex_command(binary); command.args(args).current_dir(dir); run_command(command, binary) } @@ -838,20 +853,46 @@ mod tests { } #[test] - fn the_no_engine_guidance_points_at_a_full_distribution_first() { + fn the_no_engine_guidance_points_at_tectonic_first() { let hint = install_hint(); + let distribution = if cfg!(target_os = "macos") { + "MacTeX" + } else { + "TeX Live" + }; + let tectonic = hint.find("Tectonic").expect("the recommendation is named"); + let fallback = hint.find(distribution).expect("the fallback is named"); assert!( - hint.contains("Overleaf"), - "parity is the reason to install it" + tectonic < fallback, + "the install a user can finish comes first" ); - // The prose must not duplicate the copyable command. + // The caveat has to sit in Tectonic's own clause: recommending it + // without one sends a user to an engine that silently retypesets their + // pdfLaTeX document, and this hint is the only place that says so. + let xetex = hint.find("XeTeX").expect("the limitation is named"); + assert!(tectonic < xetex && xetex < fallback); + // The prose must not duplicate the copyable command, but the command + // must install what the prose leads with. assert!(!hint.contains("brew install")); #[cfg(target_os = "macos")] - assert_eq!(install_command(), Some("brew install --cask mactex")); + assert_eq!(install_command(), Some("brew install tectonic")); #[cfg(not(target_os = "macos"))] assert_eq!(install_command(), None); } + #[test] + fn a_tex_tool_is_handed_the_path_its_own_children_need() { + // `sh` stands in for a TeX tool; all that matters is that it is on PATH. + let command = tex_command("sh"); + let (_, value) = command + .get_envs() + .find(|(key, _)| *key == "PATH") + .expect("the child is given an explicit PATH"); + assert_eq!(value, search_path().as_deref()); + // And the tool itself is resolved, not left to the child's own lookup. + assert!(Path::new(command.get_program()).is_absolute()); + } + #[test] fn tectonic_does_not_treat_recoverable_errors_as_fatal() { // microtype under XeTeX errors with "switching it off" and carries on diff --git a/src/local/opencode.rs b/src/local/opencode.rs index ae7a0165..39a7ca94 100644 --- a/src/local/opencode.rs +++ b/src/local/opencode.rs @@ -31,15 +31,8 @@ const HEALTH_TIMEOUT: Duration = Duration::from_secs(30); /// `opencode` on PATH, else the installer's default drop location. pub fn find_opencode() -> Result { - if let Some(paths) = crate::local::shell_env::search_path() { - // An empty component (`PATH=":/usr/bin"`) means cwd — never a place to - // pick up a binary we are about to execute. Mirrors `find_on_path`. - for dir in std::env::split_paths(&paths).filter(|dir| !dir.as_os_str().is_empty()) { - let candidate = dir.join("opencode"); - if candidate.is_file() { - return Ok(candidate); - } - } + if let Some(found) = crate::local::shell_env::find_on_path("opencode") { + return Ok(found); } if let Some(home) = dirs::home_dir() { let fallback = home.join(".opencode").join("bin").join("opencode"); diff --git a/src/local/shell_env.rs b/src/local/shell_env.rs index f6330f17..681abebb 100644 --- a/src/local/shell_env.rs +++ b/src/local/shell_env.rs @@ -17,7 +17,8 @@ //! `publish-branch` worker — still inherit the process environment. use std::collections::HashMap; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; +use std::path::PathBuf; use std::sync::OnceLock; /// Deliberately short. These are the variables whose divergence makes the app @@ -35,11 +36,29 @@ pub fn var(key: &str) -> Option { .or_else(|| std::env::var_os(key)) } -/// The PATH to search for harness binaries and hand to harness children. +/// The PATH to search for the binaries orx spawns, and to hand its children. pub fn search_path() -> Option { var("PATH") } +/// Where `binary` lives, or None when this machine has no such tool. The path +/// is returned as it sits on PATH; a caller that needs the real binary behind a +/// symlink composes with `resolve_symlinks`. +pub fn find_on_path(binary: &str) -> Option { + search_in(&search_path()?, binary) +} + +/// Split from the PATH lookup so the search is testable without a probe. +fn search_in(paths: &OsStr, binary: &str) -> Option { + // A relative entry (`""`, meaning cwd, or `bin`) names no fixed directory — + // it resolves against whichever cwd is current, so it is never a place to + // pick up a binary. + std::env::split_paths(paths) + .filter(|dir| dir.is_absolute()) + .map(|dir| dir.join(binary)) + .find(|candidate| candidate.is_file()) +} + /// Hand the imported variables to a child process. Every `orx` child re-resolves /// its directories from its own environment and only app mode ever probes, so /// without this a supervisor spawned by the app would write to the default @@ -103,6 +122,29 @@ mod tests { format!("nvm loaded\n{M}{payload}{M}") } + #[test] + fn the_search_skips_relative_entries_and_takes_the_first_absolute_hit() { + let root = std::env::temp_dir().join(format!("orx-path-search-{}", std::process::id())); + let (early, late) = (root.join("early"), root.join("late")); + std::fs::create_dir_all(&early).expect("early"); + std::fs::create_dir_all(&late).expect("late"); + std::fs::write(early.join("tool"), "").expect("early tool"); + std::fs::write(late.join("tool"), "").expect("late tool"); + + let paths = + std::env::join_paths([PathBuf::new(), PathBuf::from("bin"), early.clone(), late]) + .expect("join"); + assert_eq!(search_in(&paths, "tool"), Some(early.join("tool"))); + assert_eq!(search_in(&paths, "absent"), None); + + // A relative entry is rejected even when it does resolve: cargo runs + // tests from the package root, so `src/main.rs` is a real hit here. + let relative = std::env::join_paths([PathBuf::from("src")]).expect("join"); + assert_eq!(search_in(&relative, "main.rs"), None); + + std::fs::remove_dir_all(&root).ok(); + } + #[test] fn reads_every_imported_variable() { let vars = parse_probe(