From f76806655fca666649b270fa529ea834aae52691 Mon Sep 17 00:00:00 2001 From: Benjamin Lee Date: Fri, 21 Jun 2024 17:08:10 -0700 Subject: [PATCH] xtask-specific binaries in GRAPEVINE_XTASK_PATH Some possible alternatives to this: - Keep putting them in PATH. - Make xtask a nix derivation. We would lose out on incremental compilation this way, and would end up recompiling xtask from scratch whenever something in the main package changed. - Have xtask call `nix build --inputs-from $toplevel nixpkgs#go` and such. Slow and tedious. --- nix/shell.nix | 12 ++++--- xtask/src/complement.rs | 11 +++--- xtask/src/complement/test2json.rs | 2 -- xtask/src/main.rs | 56 +++++++++++++++++++++++++++---- 4 files changed, 63 insertions(+), 18 deletions(-) diff --git a/nix/shell.nix b/nix/shell.nix index 01d6f430..5d5c1afd 100644 --- a/nix/shell.nix +++ b/nix/shell.nix @@ -6,6 +6,7 @@ , go , inputs , jq +, lib , lychee , markdownlint-cli , mdbook @@ -19,6 +20,13 @@ mkShell { # sources, and it can read this environment variable to do so. The # `rust-src` component is required in order for this to work. RUST_SRC_PATH = "${toolchain}/lib/rustlib/src/rust/library"; + + # See the doc comment on `use_xtask_path` in `xtask/src/main.rs`. + GRAPEVINE_XTASK_PATH = lib.makeBinPath [ + # Keep sorted + complement + go + ]; }; # Development tools @@ -36,10 +44,6 @@ mkShell { markdownlint-cli mdbook toolchain - - # TODO: don't pollute the devshell with these - go - complement ] ++ default.nativeBuildInputs diff --git a/xtask/src/complement.rs b/xtask/src/complement.rs index 8837cde1..f93cbc19 100644 --- a/xtask/src/complement.rs +++ b/xtask/src/complement.rs @@ -58,9 +58,8 @@ pub(crate) struct Args { } #[allow(clippy::needless_pass_by_value)] -pub(crate) fn main(args: Args) -> Result<()> { - let sh = Shell::new().unwrap(); - let toplevel = get_toplevel_path(&sh) +pub(crate) fn main(args: Args, sh: &Shell) -> Result<()> { + let toplevel = get_toplevel_path(sh) .wrap_err("failed to determine repository root directory")?; let baseline_path = args .baseline @@ -74,12 +73,12 @@ pub(crate) fn main(args: Args) -> Result<()> { create_out_dir(&args.out).wrap_err_with(|| { format!("error initializing output directory {:?}", args.out) })?; - let docker_image = load_docker_image(&sh, &toplevel).wrap_err( + let docker_image = load_docker_image(sh, &toplevel).wrap_err( "failed to build and load complement-grapevine docker image", )?; - let test_count = count_complement_tests(&sh, &docker_image) + let test_count = count_complement_tests(sh, &docker_image) .wrap_err("failed to determine total complement test count")?; - let results = run_complement(&sh, &args.out, &docker_image, test_count) + let results = run_complement(sh, &args.out, &docker_image, test_count) .wrap_err("failed to run complement tests")?; let summary_path = args.out.join("summary.tsv"); compare_summary(&baseline, &results, &baseline_path, &summary_path)?; diff --git a/xtask/src/complement/test2json.rs b/xtask/src/complement/test2json.rs index 59253b14..217a2a4f 100644 --- a/xtask/src/complement/test2json.rs +++ b/xtask/src/complement/test2json.rs @@ -45,8 +45,6 @@ pub(crate) fn run_complement( test_count: u64, ) -> Result { // TODO: handle SIG{INT,TERM} - // TODO: XTASK_PATH variable, so that we don't need to pollute devshell with - // go let cmd = cmd!(sh, "go tool test2json complement.test -test.v=test2json") .env("COMPLEMENT_BASE_IMAGE", docker_image) .env("COMPLEMENT_SPAWN_HS_TIMEOUT", "5") diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 70fa34c2..e3f267d5 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -1,8 +1,10 @@ mod complement; -use std::process::ExitCode; +use std::{env, ffi::OsString, process::ExitCode}; use clap::{Parser, Subcommand}; +use miette::{miette, IntoDiagnostic, Result, WrapErr}; +use xshell::Shell; #[derive(Parser)] struct Args { @@ -16,11 +18,7 @@ enum Command { } fn main() -> ExitCode { - let args = Args::parse(); - let result = match args.command { - Command::Complement(args) => complement::main(args), - }; - let Err(e) = result else { + let Err(e) = try_main() else { return ExitCode::SUCCESS; }; // Include a leading newline because sometimes an error will occur in @@ -28,3 +26,49 @@ fn main() -> ExitCode { eprintln!("\n{e:?}"); ExitCode::FAILURE } + +fn try_main() -> Result<()> { + let args = Args::parse(); + let sh = new_shell()?; + match args.command { + Command::Complement(args) => complement::main(args, &sh), + } +} + +fn new_shell() -> Result { + let path = get_shell_path()?; + let sh = Shell::new() + .into_diagnostic() + .wrap_err("failed to initialize internal xshell::Shell wrapper")?; + sh.set_var("PATH", path); + Ok(sh) +} + +/// Returns the value to set the `PATH` environment variable to in +/// [`xshell::Shell`] instances. +/// +/// This function appends the paths from the `GRAPEVINE_XTASK_PATH` environment +/// variable to the existing value of `PATH` set in the xtask process. +/// +/// Executable dependencies that are only called by commands in xtask should be +/// added to `GRAPEVINE_XTASK_PATH` instead of `PATH` in the devshell, to avoid +/// polluting the devshell path with extra entries. +fn get_shell_path() -> Result { + let xtask_path = env::var_os("GRAPEVINE_XTASK_PATH").ok_or(miette!( + help = "This tool must be run from inside the Grapevine devshell. \ + Make sure you didn't interrupt direnv or something similar.", + "GRAPEVINE_XTASK_PATH environment variable is unset" + ))?; + if let Some(path) = env::var_os("PATH") { + let old_paths = env::split_paths(&path); + let xtask_paths = env::split_paths(&xtask_path); + env::join_paths(old_paths.chain(xtask_paths)) + .into_diagnostic() + .wrap_err( + "error constructing new PATH value to include the paths from \ + GRAPEVINE_XTASK_PATH", + ) + } else { + Ok(xtask_path) + } +}