From a649e1d0b64c43ba6713d0b6fa88648bcb834f8a Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 13 Feb 2024 04:02:36 -0600 Subject: [PATCH 01/33] Add ExpressionWalker and refactor Variables --- src/expression.rs | 4 ++ src/expression_walker.rs | 83 ++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 14 ++++--- src/variables.rs | 68 ++++---------------------------- 4 files changed, 102 insertions(+), 67 deletions(-) create mode 100644 src/expression_walker.rs diff --git a/src/expression.rs b/src/expression.rs index 58f1441e51..cfd787c9b2 100644 --- a/src/expression.rs +++ b/src/expression.rs @@ -45,6 +45,10 @@ impl<'src> Expression<'src> { pub(crate) fn variables<'expression>(&'expression self) -> Variables<'expression, 'src> { Variables::new(self) } + + pub(crate) fn walk<'expression>(&'expression self) -> ExpressionWalker<'expression, 'src> { + ExpressionWalker::new(self) + } } impl<'src> Display for Expression<'src> { diff --git a/src/expression_walker.rs b/src/expression_walker.rs new file mode 100644 index 0000000000..72751095e7 --- /dev/null +++ b/src/expression_walker.rs @@ -0,0 +1,83 @@ +use super::*; + +pub(crate) struct ExpressionWalker<'expression, 'src> { + stack: Vec<&'expression Expression<'src>>, +} + +impl<'expression, 'src> ExpressionWalker<'expression, 'src> { + pub(crate) fn new(root: &'expression Expression<'src>) -> ExpressionWalker<'expression, 'src> { + ExpressionWalker { stack: vec![root] } + } +} + +impl<'expression, 'src> Iterator for ExpressionWalker<'expression, 'src> { + type Item = &'expression Expression<'src>; + + fn next(&mut self) -> Option { + let top = self.stack.pop()?; + + match top { + Expression::StringLiteral { .. } + | Expression::Variable { .. } + | Expression::Backtick { .. } => {} + Expression::Call { thunk } => match thunk { + Thunk::Nullary { .. } => {} + Thunk::Unary { arg, .. } => self.stack.push(arg), + Thunk::UnaryOpt { + args: (a, opt_b), .. + } => { + self.stack.push(a); + if let Some(b) = opt_b.as_ref() { + self.stack.push(b); + } + } + Thunk::Binary { args, .. } => { + for arg in args.iter().rev() { + self.stack.push(arg); + } + } + Thunk::BinaryPlus { + args: ([a, b], rest), + .. + } => { + let first: &[&Expression] = &[a, b]; + for arg in first.iter().copied().chain(rest).rev() { + self.stack.push(arg); + } + } + Thunk::Ternary { args, .. } => { + for arg in args.iter().rev() { + self.stack.push(arg); + } + } + }, + Expression::Conditional { + lhs, + rhs, + then, + otherwise, + .. + } => { + self.stack.push(otherwise); + self.stack.push(then); + self.stack.push(rhs); + self.stack.push(lhs); + } + Expression::Concatenation { lhs, rhs } => { + self.stack.push(rhs); + self.stack.push(lhs); + } + Expression::Join { lhs, rhs } => { + self.stack.push(rhs); + if let Some(lhs) = lhs { + self.stack.push(lhs); + } + } + Expression::Group { contents } => { + self.stack.push(contents); + } + } + + Some(top) + } +} diff --git a/src/lib.rs b/src/lib.rs index 445e819019..80f33a59f3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -22,12 +22,13 @@ pub(crate) use { conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, - fragment::Fragment, function::Function, function_context::FunctionContext, - interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, - justfile::Justfile, keyed::Keyed, keyword::Keyword, lexer::Lexer, line::Line, list::List, - load_dotenv::load_dotenv, loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, - output::output, output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, - parser::Parser, platform::Platform, platform_interface::PlatformInterface, position::Position, + expression_walker::ExpressionWalker, fragment::Fragment, function::Function, + function_context::FunctionContext, interrupt_guard::InterruptGuard, + interrupt_handler::InterruptHandler, item::Item, justfile::Justfile, keyed::Keyed, + keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, + loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, output::output, + output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, + platform::Platform, platform_interface::PlatformInterface, position::Position, positional::Positional, ran::Ran, range_ext::RangeExt, recipe::Recipe, recipe_context::RecipeContext, recipe_resolver::RecipeResolver, scope::Scope, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, @@ -135,6 +136,7 @@ mod enclosure; mod error; mod evaluator; mod expression; +mod expression_walker; mod fragment; mod function; mod function_context; diff --git a/src/variables.rs b/src/variables.rs index 8a17254a23..c9456c75dc 100644 --- a/src/variables.rs +++ b/src/variables.rs @@ -1,12 +1,14 @@ use super::*; pub(crate) struct Variables<'expression, 'src> { - stack: Vec<&'expression Expression<'src>>, + walker: ExpressionWalker<'expression, 'src>, } impl<'expression, 'src> Variables<'expression, 'src> { pub(crate) fn new(root: &'expression Expression<'src>) -> Variables<'expression, 'src> { - Variables { stack: vec![root] } + Variables { + walker: root.walk(), + } } } @@ -15,65 +17,9 @@ impl<'expression, 'src> Iterator for Variables<'expression, 'src> { fn next(&mut self) -> Option> { loop { - match self.stack.pop()? { - Expression::StringLiteral { .. } | Expression::Backtick { .. } => {} - Expression::Call { thunk } => match thunk { - Thunk::Nullary { .. } => {} - Thunk::Unary { arg, .. } => self.stack.push(arg), - Thunk::UnaryOpt { - args: (a, opt_b), .. - } => { - self.stack.push(a); - if let Some(b) = opt_b.as_ref() { - self.stack.push(b); - } - } - Thunk::Binary { args, .. } => { - for arg in args.iter().rev() { - self.stack.push(arg); - } - } - Thunk::BinaryPlus { - args: ([a, b], rest), - .. - } => { - let first: &[&Expression] = &[a, b]; - for arg in first.iter().copied().chain(rest).rev() { - self.stack.push(arg); - } - } - Thunk::Ternary { args, .. } => { - for arg in args.iter().rev() { - self.stack.push(arg); - } - } - }, - Expression::Conditional { - lhs, - rhs, - then, - otherwise, - .. - } => { - self.stack.push(otherwise); - self.stack.push(then); - self.stack.push(rhs); - self.stack.push(lhs); - } - Expression::Variable { name, .. } => return Some(name.token), - Expression::Concatenation { lhs, rhs } => { - self.stack.push(rhs); - self.stack.push(lhs); - } - Expression::Join { lhs, rhs } => { - self.stack.push(rhs); - if let Some(lhs) = lhs { - self.stack.push(lhs); - } - } - Expression::Group { contents } => { - self.stack.push(contents); - } + match self.walker.next()? { + Expression::Variable { name } => return Some(name.token), + _ => continue, } } } From 47976a75f240e090b1c16fd2cc107004c2e805b4 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 15 Feb 2024 01:24:00 -0600 Subject: [PATCH 02/33] Cache recipe by evaluated body, initial --- src/attribute.rs | 1 + src/cache.rs | 13 +++++ src/error.rs | 20 +++++++ src/keyword.rs | 1 + src/lib.rs | 6 +- src/node.rs | 5 +- src/parser.rs | 1 + src/recipe.rs | 143 +++++++++++++++++++++++++++++++++++++++++++++-- src/setting.rs | 6 +- src/settings.rs | 4 ++ tests/json.rs | 30 ++++++++-- 11 files changed, 216 insertions(+), 14 deletions(-) create mode 100644 src/cache.rs diff --git a/src/attribute.rs b/src/attribute.rs index 8516ce9403..0eb4c65d35 100644 --- a/src/attribute.rs +++ b/src/attribute.rs @@ -5,6 +5,7 @@ use super::*; #[serde(rename_all = "kebab-case")] pub(crate) enum Attribute<'src> { Confirm(Option>), + Cached, Linux, Macos, NoCd, diff --git a/src/cache.rs b/src/cache.rs new file mode 100644 index 0000000000..e368229020 --- /dev/null +++ b/src/cache.rs @@ -0,0 +1,13 @@ +use std::collections::HashMap; + +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct JustfileCache { + pub(crate) recipe_caches: HashMap, +} + +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct RecipeCache { + pub(crate) hash: String, +} diff --git a/src/error.rs b/src/error.rs index c71cbb92d7..7d3f05f613 100644 --- a/src/error.rs +++ b/src/error.rs @@ -17,6 +17,13 @@ pub(crate) enum Error<'src> { token: Token<'src>, output_error: OutputError, }, + CacheFileRead { + cache_filename: Option, + }, + CacheFileWrite { + cache_filename: PathBuf, + io_error: io::Error, + }, ChooserInvoke { shell_binary: String, shell_arguments: String, @@ -96,6 +103,10 @@ pub(crate) enum Error<'src> { io_error: io::Error, }, Homedir, + ImpureCachedRecipe { + recipe: &'src str, + impure_contained: String, + }, InitExists { justfile: PathBuf, }, @@ -267,6 +278,12 @@ impl<'src> ColorDisplay for Error<'src> { }?, OutputError::Utf8(utf8_error) => write!(f, "Backtick succeeded but stdout was not utf8: {utf8_error}")?, } + CacheFileRead {cache_filename} => match cache_filename { + Some(cache_filename) => + write!(f, "Failed to read cache file: {}", cache_filename.display())?, + None => write!(f, "Failed to get default cache file")?, + } + CacheFileWrite{cache_filename, io_error} => write!(f, "Failed to write cache file ({}): {io_error}", cache_filename.display())?, ChooserInvoke { shell_binary, shell_arguments, chooser, io_error} => { let chooser = chooser.to_string_lossy(); write!(f, "Chooser `{shell_binary} {shell_arguments} {chooser}` invocation failed: {io_error}")?; @@ -355,6 +372,9 @@ impl<'src> ColorDisplay for Error<'src> { Homedir => { write!(f, "Failed to get homedir")?; } + ImpureCachedRecipe { recipe, impure_contained } => { + write!(f, "Cached recipe `{recipe}` contains {impure_contained}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...")?; + } InitExists { justfile } => { write!(f, "Justfile `{}` already exists", justfile.display())?; } diff --git a/src/keyword.rs b/src/keyword.rs index 830207e55b..8c148ea893 100644 --- a/src/keyword.rs +++ b/src/keyword.rs @@ -5,6 +5,7 @@ use super::*; pub(crate) enum Keyword { Alias, AllowDuplicateRecipes, + CacheFilename, DotenvFilename, DotenvLoad, DotenvPath, diff --git a/src/lib.rs b/src/lib.rs index 80f33a59f3..5559bff3dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,8 +17,9 @@ pub(crate) use { crate::{ alias::Alias, analyzer::Analyzer, assignment::Assignment, assignment_resolver::AssignmentResolver, ast::Ast, attribute::Attribute, binding::Binding, - color::Color, color_display::ColorDisplay, command_ext::CommandExt, compilation::Compilation, - compile_error::CompileError, compile_error_kind::CompileErrorKind, compiler::Compiler, + cache::JustfileCache, cache::RecipeCache, color::Color, color_display::ColorDisplay, + command_ext::CommandExt, compilation::Compilation, compile_error::CompileError, + compile_error_kind::CompileErrorKind, compiler::Compiler, conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, @@ -117,6 +118,7 @@ mod assignment_resolver; mod ast; mod attribute; mod binding; +mod cache; mod color; mod color_display; mod command_ext; diff --git a/src/node.rs b/src/node.rs index 1d98b99173..05ba3841e6 100644 --- a/src/node.rs +++ b/src/node.rs @@ -281,7 +281,10 @@ impl<'src> Node<'src> for Set<'src> { set.push_mut(Tree::string(&argument.cooked)); } } - Setting::DotenvFilename(value) | Setting::DotenvPath(value) | Setting::Tempdir(value) => { + Setting::CacheFilename(value) + | Setting::DotenvFilename(value) + | Setting::DotenvPath(value) + | Setting::Tempdir(value) => { set.push_mut(Tree::string(value)); } } diff --git a/src/parser.rs b/src/parser.rs index ba006ac5bd..2b90f7e9f5 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -864,6 +864,7 @@ impl<'run, 'src> Parser<'run, 'src> { self.expect(ColonEquals)?; let set_value = match keyword { + Keyword::CacheFilename => Some(Setting::CacheFilename(self.parse_string_literal()?.cooked)), Keyword::DotenvFilename => Some(Setting::DotenvFilename(self.parse_string_literal()?.cooked)), Keyword::DotenvPath => Some(Setting::DotenvPath(self.parse_string_literal()?.cooked)), Keyword::Shell => Some(Setting::Shell(self.parse_shell()?)), diff --git a/src/recipe.rs b/src/recipe.rs index 3ef2843f0b..b08618f071 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -1,6 +1,9 @@ use { super::*, - std::process::{ExitStatus, Stdio}, + std::{ + os::unix::ffi::OsStrExt, + process::{ExitStatus, Stdio}, + }, }; /// Return a `Error::Signal` if the process was terminated by a signal, @@ -143,6 +146,110 @@ impl<'src, D> Recipe<'src, D> { self.attributes.contains(&Attribute::NoQuiet) } + fn cached(&self) -> bool { + self.attributes.contains(&Attribute::Cached) + } + + fn get_updated_cache_if_outdated<'run>( + &self, + context: &RecipeContext<'src, 'run>, + mut evaluator: Evaluator<'src, 'run>, + ) -> RunResult<'src, Option<(PathBuf, JustfileCache)>> { + let find_backticks = self + .body + .iter() + .flat_map(|line| line.fragments.iter()) + .filter_map(|fragment| match fragment { + Fragment::Interpolation { expression } => Some(expression), + _ => None, + }) + .flat_map(|expression| expression.walk()) + .filter_map(|sub_expression| match sub_expression { + Expression::Backtick { contents, .. } => Some(contents), + _ => None, + }); + + if find_backticks.into_iter().next().is_some() { + return Err(Error::ImpureCachedRecipe { + recipe: self.name(), + impure_contained: "a backtick expression".to_string(), + }); + } + + let cache_filename = match &context.settings.cache_filename { + Some(cache_filename) => context.search.working_directory.join(cache_filename), + None => { + let project_dir = &context.search.working_directory; + let project_name = project_dir + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("UNKNOWN_PROJECT"); + let path_hash = blake3::hash(project_dir.as_os_str().as_bytes()).to_hex(); + + dirs::cache_dir() + .ok_or(Error::CacheFileRead { + cache_filename: None, + })? + .join("justfiles") + .join(format!("{project_name}-{path_hash}.json")) + } + }; + + let mut cache: JustfileCache = fs::read_to_string(&cache_filename) + .or(Err(())) + .and_then(|cache| serde_json::from_str(&cache).or(Err(()))) + .or_else(|_| { + Err(Error::CacheFileRead { + cache_filename: Some(cache_filename.clone()), + }) + })?; + + match cache.recipe_caches.get(self.name()) { + None => Ok(None), + Some(previous_run) => { + // TODO: Prevent double work by evaluating twice + let mut recipe_hash = blake3::Hasher::new(); + for line in &self.body { + recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); + } + let recipe_hash = recipe_hash.finalize().to_hex(); + + if previous_run.hash == recipe_hash.as_str() { + Ok(None) + } else { + cache.recipe_caches.insert( + self.name().to_string(), + RecipeCache { + hash: recipe_hash.to_string(), + }, + ); + Ok(Some((cache_filename, cache))) + } + } + } + } + + fn save_latest_cache( + &self, + cache_filename: PathBuf, + cache: JustfileCache, + ) -> RunResult<'src, ()> { + fs::write( + &cache_filename, + serde_json::to_string(&cache).or_else(|_| { + Err(Error::Internal { + message: format!("Failed to serialize cache: {cache:?}"), + }) + })?, + ) + .or_else(|io_error| { + Err(Error::CacheFileWrite { + cache_filename, + io_error, + }) + }) + } + pub(crate) fn run<'run>( &self, context: &RecipeContext<'src, 'run>, @@ -153,6 +260,27 @@ impl<'src, D> Recipe<'src, D> { ) -> RunResult<'src, ()> { let config = &context.config; + let updated_cache = if !self.cached() { + None + } else { + let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); + match self.get_updated_cache_if_outdated(context, evaluator)? { + None => { + if config.verbosity.loquacious() { + let color = config.color.stderr().banner(); + eprintln!( + "{}===> Recipe `{}` matches latest cache. Skipping...{}", + color.prefix(), + self.name, + color.suffix() + ); + } + return Ok(()); + } + Some(cache_info) => Some(cache_info), + } + }; + if config.verbosity.loquacious() { let color = config.color.stderr().banner(); eprintln!( @@ -163,13 +291,18 @@ impl<'src, D> Recipe<'src, D> { ); } - let evaluator = - Evaluator::recipe_evaluator(context.config, dotenv, &scope, context.settings, search); + let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); if self.shebang { - self.run_shebang(context, dotenv, &scope, positional, config, evaluator) + self.run_shebang(context, dotenv, &scope, positional, config, evaluator)?; + } else { + self.run_linewise(context, dotenv, &scope, positional, config, evaluator)?; + } + + if let Some((cache_filename, cache)) = updated_cache { + self.save_latest_cache(cache_filename, cache) } else { - self.run_linewise(context, dotenv, &scope, positional, config, evaluator) + Ok(()) } } diff --git a/src/setting.rs b/src/setting.rs index 0256ff94c9..6d0c0f05b9 100644 --- a/src/setting.rs +++ b/src/setting.rs @@ -3,6 +3,7 @@ use super::*; #[derive(Debug, Clone)] pub(crate) enum Setting<'src> { AllowDuplicateRecipes(bool), + CacheFilename(String), DotenvFilename(String), DotenvLoad(bool), DotenvPath(String), @@ -29,7 +30,10 @@ impl<'src> Display for Setting<'src> { | Setting::Quiet(value) | Setting::WindowsPowerShell(value) => write!(f, "{value}"), Setting::Shell(shell) | Setting::WindowsShell(shell) => write!(f, "{shell}"), - Setting::DotenvFilename(value) | Setting::DotenvPath(value) | Setting::Tempdir(value) => { + Setting::CacheFilename(value) + | Setting::DotenvFilename(value) + | Setting::DotenvPath(value) + | Setting::Tempdir(value) => { write!(f, "{value:?}") } } diff --git a/src/settings.rs b/src/settings.rs index f72cb5465e..73e9728109 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -8,6 +8,7 @@ pub(crate) const WINDOWS_POWERSHELL_ARGS: &[&str] = &["-NoLogo", "-Command"]; #[derive(Debug, PartialEq, Serialize, Default)] pub(crate) struct Settings<'src> { pub(crate) allow_duplicate_recipes: bool, + pub(crate) cache_filename: Option, pub(crate) dotenv_filename: Option, pub(crate) dotenv_load: Option, pub(crate) dotenv_path: Option, @@ -31,6 +32,9 @@ impl<'src> Settings<'src> { Setting::AllowDuplicateRecipes(allow_duplicate_recipes) => { settings.allow_duplicate_recipes = allow_duplicate_recipes; } + Setting::CacheFilename(cache_filename) => { + settings.cache_filename = Some(cache_filename); + } Setting::DotenvFilename(filename) => { settings.dotenv_filename = Some(filename); } diff --git a/tests/json.rs b/tests/json.rs index 4c71b430ff..d417a2ceba 100644 --- a/tests/json.rs +++ b/tests/json.rs @@ -44,6 +44,8 @@ fn alias() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -80,6 +82,7 @@ fn assignment() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -131,6 +134,7 @@ fn body() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -194,6 +198,7 @@ fn dependencies() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -294,6 +299,7 @@ fn dependency_argument() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -357,6 +363,7 @@ fn duplicate_recipes() { }, "settings": { "allow_duplicate_recipes": true, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -401,6 +408,7 @@ fn doc_comment() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -431,6 +439,7 @@ fn empty_justfile() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -582,6 +591,7 @@ fn parameters() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -666,6 +676,7 @@ fn priors() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -710,6 +721,7 @@ fn private() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -754,6 +766,7 @@ fn quiet() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -775,10 +788,11 @@ fn quiet() { #[test] fn settings() { case( - " + r#" set dotenv-load - set dotenv-filename := \"filename\" - set dotenv-path := \"path\" + set cache-filename := "cache-filename" + set dotenv-filename := "dotenv-filename" + set dotenv-path := "path" set export set fallback set positional-arguments @@ -787,7 +801,7 @@ fn settings() { set shell := ['a', 'b', 'c'] foo: #!bar - ", + "#, json!({ "aliases": {}, "assignments": {}, @@ -810,7 +824,8 @@ fn settings() { }, "settings": { "allow_duplicate_recipes": false, - "dotenv_filename": "filename", + "cache_filename": "cache-filename", + "dotenv_filename": "dotenv-filename", "dotenv_load": true, "dotenv_path": "path", "export": true, @@ -860,6 +875,7 @@ fn shebang() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -904,6 +920,7 @@ fn simple() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -951,6 +968,7 @@ fn attribute() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -1011,6 +1029,7 @@ fn module() { }, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -1030,6 +1049,7 @@ fn module() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, + "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, From 7d24508753b3bab4fec653d170b079629effced5 Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 00:23:09 -0600 Subject: [PATCH 03/33] Tests --- src/cache.rs | 8 +++ src/error.rs | 8 +-- src/recipe.rs | 106 +++++++++++++++++------------- tests/cached_recipes.rs | 140 ++++++++++++++++++++++++++++++++++++++++ tests/lib.rs | 1 + 5 files changed, 215 insertions(+), 48 deletions(-) create mode 100644 tests/cached_recipes.rs diff --git a/src/cache.rs b/src/cache.rs index e368229020..1e3af1e873 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -7,6 +7,14 @@ pub(crate) struct JustfileCache { pub(crate) recipe_caches: HashMap, } +impl JustfileCache { + pub(crate) fn new() -> Self { + Self { + recipe_caches: HashMap::new(), + } + } +} + #[derive(Debug, Serialize, Deserialize)] pub(crate) struct RecipeCache { pub(crate) hash: String, diff --git a/src/error.rs b/src/error.rs index 7d3f05f613..6d5270e394 100644 --- a/src/error.rs +++ b/src/error.rs @@ -103,9 +103,9 @@ pub(crate) enum Error<'src> { io_error: io::Error, }, Homedir, - ImpureCachedRecipe { + InvalidCachedRecipe { recipe: &'src str, - impure_contained: String, + describe_invalid: String, }, InitExists { justfile: PathBuf, @@ -372,8 +372,8 @@ impl<'src> ColorDisplay for Error<'src> { Homedir => { write!(f, "Failed to get homedir")?; } - ImpureCachedRecipe { recipe, impure_contained } => { - write!(f, "Cached recipe `{recipe}` contains {impure_contained}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...")?; + InvalidCachedRecipe { recipe, describe_invalid } => { + write!(f, "Cached recipe `{recipe}` contains {describe_invalid}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...")?; } InitExists { justfile } => { write!(f, "Justfile `{}` already exists", justfile.display())?; diff --git a/src/recipe.rs b/src/recipe.rs index b08618f071..c7372394b4 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -165,14 +165,21 @@ impl<'src, D> Recipe<'src, D> { }) .flat_map(|expression| expression.walk()) .filter_map(|sub_expression| match sub_expression { - Expression::Backtick { contents, .. } => Some(contents), + Expression::Backtick { .. } => Some("a backtick expression".to_string()), + Expression::Call { thunk } => match thunk { + Thunk::Nullary { name, .. } => { + let name = name.lexeme(); + (name == "uuid" || name == "just_pid").then(|| format!("a call to `{name}`")) + } + _ => None, + }, _ => None, }); - if find_backticks.into_iter().next().is_some() { - return Err(Error::ImpureCachedRecipe { + if let Some(describe_invalid) = find_backticks.into_iter().next() { + return Err(Error::InvalidCachedRecipe { recipe: self.name(), - impure_contained: "a backtick expression".to_string(), + describe_invalid, }); } @@ -195,38 +202,38 @@ impl<'src, D> Recipe<'src, D> { } }; - let mut cache: JustfileCache = fs::read_to_string(&cache_filename) - .or(Err(())) - .and_then(|cache| serde_json::from_str(&cache).or(Err(()))) - .or_else(|_| { - Err(Error::CacheFileRead { - cache_filename: Some(cache_filename.clone()), - }) - })?; + let mut cache = if !cache_filename.exists() { + JustfileCache::new() + } else { + fs::read_to_string(&cache_filename) + .or(Err(())) + .and_then(|cache| serde_json::from_str(&cache).or(Err(()))) + .or_else(|_| { + Err(Error::CacheFileRead { + cache_filename: Some(cache_filename.clone()), + }) + })? + }; - match cache.recipe_caches.get(self.name()) { - None => Ok(None), - Some(previous_run) => { - // TODO: Prevent double work by evaluating twice - let mut recipe_hash = blake3::Hasher::new(); - for line in &self.body { - recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); - } - let recipe_hash = recipe_hash.finalize().to_hex(); + // TODO: Prevent double work/evaluating twice + let mut recipe_hash = blake3::Hasher::new(); + for line in &self.body { + recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); + } + let recipe_hash = recipe_hash.finalize().to_hex(); - if previous_run.hash == recipe_hash.as_str() { - Ok(None) - } else { - cache.recipe_caches.insert( - self.name().to_string(), - RecipeCache { - hash: recipe_hash.to_string(), - }, - ); - Ok(Some((cache_filename, cache))) - } + if let Some(previous_run) = cache.recipe_caches.get(self.name()) { + if previous_run.hash == recipe_hash.as_str() { + return Ok(None); } } + cache.recipe_caches.insert( + self.name().to_string(), + RecipeCache { + hash: recipe_hash.to_string(), + }, + ); + Ok(Some((cache_filename, cache))) } fn save_latest_cache( @@ -234,20 +241,31 @@ impl<'src, D> Recipe<'src, D> { cache_filename: PathBuf, cache: JustfileCache, ) -> RunResult<'src, ()> { - fs::write( - &cache_filename, - serde_json::to_string(&cache).or_else(|_| { - Err(Error::Internal { - message: format!("Failed to serialize cache: {cache:?}"), + let cache = serde_json::to_string(&cache).or_else(|_| { + Err(Error::Internal { + message: format!("Failed to serialize cache: {cache:?}"), + }) + })?; + + cache_filename + .parent() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::Unsupported, + format!( + "Cannot create parent directory of {}", + cache_filename.display() + ), + ) + }) + .and_then(|parent| fs::create_dir_all(parent)) + .and_then(|_| fs::write(&cache_filename, cache)) + .or_else(|io_error| { + Err(Error::CacheFileWrite { + cache_filename, + io_error, }) - })?, - ) - .or_else(|io_error| { - Err(Error::CacheFileWrite { - cache_filename, - io_error, }) - }) } pub(crate) fn run<'run>( diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs new file mode 100644 index 0000000000..b2cf9b0a48 --- /dev/null +++ b/tests/cached_recipes.rs @@ -0,0 +1,140 @@ +use super::*; + +fn build_test(justfile: &str, parent: &TempDir) -> Test { + Test::with_tempdir(TempDir::new_in(&parent).expect("to create tempdir")).justfile(justfile) +} + +#[test] +fn cached_recipes_are_actually_cached() { + let justfile = r#" + set cache-filename := "../local_cache.json" + [cached] + echo: + @echo cached + "#; + let parent = tempdir(); + + build_test(justfile, &parent).stdout("cached\n").run(); + build_test(justfile, &parent).stdout("").run(); +} + +#[test] +fn uncached_recipes_are_not_uncached() { + let justfile = r#" + set cache-filename := "../local_cache.json" + @echo: + echo uncached + "#; + let parent = tempdir(); + + build_test(justfile, &parent).stdout("uncached\n").run(); + build_test(justfile, &parent).stdout("uncached\n").run(); +} + +#[test] +fn cached_recipes_are_independent() { + let justfile = r#" + set cache-filename := "../local_cache.json" + + [cached] + echo1: + @echo cached1 + + [cached] + echo2: + @echo cached2 + "#; + let parent = tempdir(); + + build_test(justfile, &parent) + .arg("echo1") + .stdout("cached1\n") + .run(); + build_test(justfile, &parent) + .arg("echo2") + .stdout("cached2\n") + .run(); + build_test(justfile, &parent).arg("echo1").stdout("").run(); + build_test(justfile, &parent).arg("echo2").stdout("").run(); +} + +#[test] +fn arguments_and_variables_are_part_of_cache_hash() { + let justfile = r#" + set cache-filename := "../local_cache.json" + my-var := "1" + [cached] + echo ARG: + @echo {{ARG}}{{my-var}} + "#; + let parent = tempdir(); + + build_test(justfile, &parent) + .args(["echo", "a"]) + .stdout("a1\n") + .run(); + build_test(justfile, &parent) + .args(["echo", "a"]) + .stdout("") + .run(); + build_test(justfile, &parent) + .args(["echo", "b"]) + .stdout("b1\n") + .run(); + build_test(justfile, &parent) + .args(["echo", "b"]) + .stdout("") + .run(); + build_test(justfile, &parent) + .args(["my-var=2", "echo", "b"]) + .stdout("b2\n") + .run(); + build_test(justfile, &parent) + .args(["my-var=2", "echo", "b"]) + .stdout("") + .run(); +} + +#[test] +fn invalid_recipe_errors() { + let commands = [ + ("@echo {{`echo deja vu`}}", "a backtick expression"), + ("@echo uuid4: {{uuid()}}", "a call to `uuid`"), + ("@echo process_id: {{just_pid()}}", "a call to `just_pid`"), + ]; + + for (command, invalid) in commands { + Test::new() + .justfile(format!( + r#" + set cache-filename := "local_cache.json" + [cached] + invalid: + {command} + "# + )) + .stderr(format!("error: Cached recipe `invalid` contains {invalid}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...\n")) + .status(EXIT_FAILURE) + .run(); + } +} + +#[test] +fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() { + assert!(false); +} + +#[test] +fn cached_deps_cannot_depend_on_preceding_uncached_ones() { + assert!(false); +} + +#[test] +fn delete_cache_works() { + assert!(false); +} + +#[test] +fn default_cache_location_works() { + assert!(false); +} diff --git a/tests/lib.rs b/tests/lib.rs index a9f3c3448f..8ce133cb24 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -37,6 +37,7 @@ mod assert_stdout; mod assert_success; mod attributes; mod byte_order_mark; +mod cached_recipes; mod changelog; mod choose; mod command; From 626dd08e40b0717c440af13e3aa9c46b97f37a61 Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 01:33:46 -0600 Subject: [PATCH 04/33] Include working directory/project dir in serialized cache --- src/cache.rs | 6 +++++- src/recipe.rs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 1e3af1e873..f909a77862 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -1,15 +1,19 @@ use std::collections::HashMap; +use std::path::PathBuf; use serde::{Deserialize, Serialize}; #[derive(Debug, Serialize, Deserialize)] pub(crate) struct JustfileCache { + /// Only serialized for user debugging + pub(crate) working_directory: PathBuf, pub(crate) recipe_caches: HashMap, } impl JustfileCache { - pub(crate) fn new() -> Self { + pub(crate) fn new(working_directory: PathBuf) -> Self { Self { + working_directory, recipe_caches: HashMap::new(), } } diff --git a/src/recipe.rs b/src/recipe.rs index c7372394b4..bd5407c7b2 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -203,7 +203,7 @@ impl<'src, D> Recipe<'src, D> { }; let mut cache = if !cache_filename.exists() { - JustfileCache::new() + JustfileCache::new(context.search.working_directory.clone()) } else { fs::read_to_string(&cache_filename) .or(Err(())) From ed9f0aa637bdd94f27793735a23c30b768059c1d Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 03:46:11 -0600 Subject: [PATCH 05/33] Rename RecipeCache.{hash => body_hash} --- src/cache.rs | 2 +- src/recipe.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index f909a77862..3c75930cad 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -21,5 +21,5 @@ impl JustfileCache { #[derive(Debug, Serialize, Deserialize)] pub(crate) struct RecipeCache { - pub(crate) hash: String, + pub(crate) body_hash: String, } diff --git a/src/recipe.rs b/src/recipe.rs index bd5407c7b2..ad62426887 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -223,14 +223,14 @@ impl<'src, D> Recipe<'src, D> { let recipe_hash = recipe_hash.finalize().to_hex(); if let Some(previous_run) = cache.recipe_caches.get(self.name()) { - if previous_run.hash == recipe_hash.as_str() { + if previous_run.body_hash == recipe_hash.as_str() { return Ok(None); } } cache.recipe_caches.insert( self.name().to_string(), RecipeCache { - hash: recipe_hash.to_string(), + body_hash: recipe_hash.to_string(), }, ); Ok(Some((cache_filename, cache))) From a1f740c4cecb7fa5f286722a254302eeeab26ec8 Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 03:47:59 -0600 Subject: [PATCH 06/33] Shorten cache filename --- src/recipe.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/recipe.rs b/src/recipe.rs index ad62426887..440835fa0c 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -191,7 +191,7 @@ impl<'src, D> Recipe<'src, D> { .file_name() .and_then(|name| name.to_str()) .unwrap_or("UNKNOWN_PROJECT"); - let path_hash = blake3::hash(project_dir.as_os_str().as_bytes()).to_hex(); + let path_hash = &blake3::hash(project_dir.as_os_str().as_bytes()).to_hex()[..16]; dirs::cache_dir() .ok_or(Error::CacheFileRead { From d885519cad1598685b594eb6d0d2312e7d25d8f9 Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 16:39:06 -0600 Subject: [PATCH 07/33] Rename JustfileCache.{recipe_caches => recipes} --- src/cache.rs | 4 ++-- src/recipe.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 3c75930cad..70b4f0fd8d 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -7,14 +7,14 @@ use serde::{Deserialize, Serialize}; pub(crate) struct JustfileCache { /// Only serialized for user debugging pub(crate) working_directory: PathBuf, - pub(crate) recipe_caches: HashMap, + pub(crate) recipes: HashMap, } impl JustfileCache { pub(crate) fn new(working_directory: PathBuf) -> Self { Self { working_directory, - recipe_caches: HashMap::new(), + recipes: HashMap::new(), } } } diff --git a/src/recipe.rs b/src/recipe.rs index 440835fa0c..5fe391236d 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -222,12 +222,12 @@ impl<'src, D> Recipe<'src, D> { } let recipe_hash = recipe_hash.finalize().to_hex(); - if let Some(previous_run) = cache.recipe_caches.get(self.name()) { + if let Some(previous_run) = cache.recipes.get(self.name()) { if previous_run.body_hash == recipe_hash.as_str() { return Ok(None); } } - cache.recipe_caches.insert( + cache.recipes.insert( self.name().to_string(), RecipeCache { body_hash: recipe_hash.to_string(), From 5a959d1db2b42cee3b5d71597c8fd8fc25bad668 Mon Sep 17 00:00:00 2001 From: Noah May Date: Fri, 16 Feb 2024 17:25:08 -0600 Subject: [PATCH 08/33] Use working directory AND justfile path for cache filename --- src/cache.rs | 8 ++++++-- src/recipe.rs | 7 +++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 70b4f0fd8d..7974720614 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -3,17 +3,21 @@ use std::path::PathBuf; use serde::{Deserialize, Serialize}; +use super::*; + #[derive(Debug, Serialize, Deserialize)] pub(crate) struct JustfileCache { /// Only serialized for user debugging + pub(crate) justfile_path: PathBuf, pub(crate) working_directory: PathBuf, pub(crate) recipes: HashMap, } impl JustfileCache { - pub(crate) fn new(working_directory: PathBuf) -> Self { + pub(crate) fn new(search: &Search) -> Self { Self { - working_directory, + justfile_path: search.justfile.clone(), + working_directory: search.working_directory.clone(), recipes: HashMap::new(), } } diff --git a/src/recipe.rs b/src/recipe.rs index 5fe391236d..b2e7074bc4 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -191,7 +191,10 @@ impl<'src, D> Recipe<'src, D> { .file_name() .and_then(|name| name.to_str()) .unwrap_or("UNKNOWN_PROJECT"); - let path_hash = &blake3::hash(project_dir.as_os_str().as_bytes()).to_hex()[..16]; + let mut path_hash = blake3::Hasher::new(); + path_hash.update(project_dir.as_os_str().as_bytes()); + path_hash.update(context.search.justfile.as_os_str().as_bytes()); + let path_hash = &path_hash.finalize().to_hex()[..16]; dirs::cache_dir() .ok_or(Error::CacheFileRead { @@ -203,7 +206,7 @@ impl<'src, D> Recipe<'src, D> { }; let mut cache = if !cache_filename.exists() { - JustfileCache::new(context.search.working_directory.clone()) + JustfileCache::new(context.search) } else { fs::read_to_string(&cache_filename) .or(Err(())) From d82e9e10a7d15f04b4c89fd4af37c8adef288bc4 Mon Sep 17 00:00:00 2001 From: Noah May Date: Wed, 6 Mar 2024 21:32:54 -0600 Subject: [PATCH 09/33] Remove InvalidCachedRecipe error --- src/error.rs | 7 ------- src/recipe.rs | 28 ---------------------------- 2 files changed, 35 deletions(-) diff --git a/src/error.rs b/src/error.rs index 6d5270e394..c7e951925d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -103,10 +103,6 @@ pub(crate) enum Error<'src> { io_error: io::Error, }, Homedir, - InvalidCachedRecipe { - recipe: &'src str, - describe_invalid: String, - }, InitExists { justfile: PathBuf, }, @@ -372,9 +368,6 @@ impl<'src> ColorDisplay for Error<'src> { Homedir => { write!(f, "Failed to get homedir")?; } - InvalidCachedRecipe { recipe, describe_invalid } => { - write!(f, "Cached recipe `{recipe}` contains {describe_invalid}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...")?; - } InitExists { justfile } => { write!(f, "Justfile `{}` already exists", justfile.display())?; } diff --git a/src/recipe.rs b/src/recipe.rs index b2e7074bc4..7214c0a505 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -155,34 +155,6 @@ impl<'src, D> Recipe<'src, D> { context: &RecipeContext<'src, 'run>, mut evaluator: Evaluator<'src, 'run>, ) -> RunResult<'src, Option<(PathBuf, JustfileCache)>> { - let find_backticks = self - .body - .iter() - .flat_map(|line| line.fragments.iter()) - .filter_map(|fragment| match fragment { - Fragment::Interpolation { expression } => Some(expression), - _ => None, - }) - .flat_map(|expression| expression.walk()) - .filter_map(|sub_expression| match sub_expression { - Expression::Backtick { .. } => Some("a backtick expression".to_string()), - Expression::Call { thunk } => match thunk { - Thunk::Nullary { name, .. } => { - let name = name.lexeme(); - (name == "uuid" || name == "just_pid").then(|| format!("a call to `{name}`")) - } - _ => None, - }, - _ => None, - }); - - if let Some(describe_invalid) = find_backticks.into_iter().next() { - return Err(Error::InvalidCachedRecipe { - recipe: self.name(), - describe_invalid, - }); - } - let cache_filename = match &context.settings.cache_filename { Some(cache_filename) => context.search.working_directory.join(cache_filename), None => { From c18a0cc2616cc0fadd16260d9d5a589a94c253bd Mon Sep 17 00:00:00 2001 From: Noah May Date: Wed, 6 Mar 2024 22:39:17 -0600 Subject: [PATCH 10/33] Print 'skipping cached recipe' message by default --- src/recipe.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index 7214c0a505..78d212724d 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -259,10 +259,17 @@ impl<'src, D> Recipe<'src, D> { let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); match self.get_updated_cache_if_outdated(context, evaluator)? { None => { - if config.verbosity.loquacious() { - let color = config.color.stderr().banner(); + if config.dry_run + || config.verbosity.loquacious() + || !((context.settings.quiet && !self.no_quiet()) || config.verbosity.quiet()) + { + let color = if config.highlight { + config.color.command(config.command_color) + } else { + config.color + }; eprintln!( - "{}===> Recipe `{}` matches latest cache. Skipping...{}", + "{}===> Hash of recipe body of `{}` matches last run. Skipping...{}", color.prefix(), self.name, color.suffix() From 6307eb0fc11e3ea70a243b5a07a50e977e13214e Mon Sep 17 00:00:00 2001 From: Noah May Date: Wed, 6 Mar 2024 22:49:44 -0600 Subject: [PATCH 11/33] Remove `CacheFilename` for now --- src/keyword.rs | 1 - src/node.rs | 5 +---- src/parser.rs | 1 - src/recipe.rs | 37 +++++++++++++++++-------------------- src/setting.rs | 6 +----- src/settings.rs | 4 ---- 6 files changed, 19 insertions(+), 35 deletions(-) diff --git a/src/keyword.rs b/src/keyword.rs index 8c148ea893..830207e55b 100644 --- a/src/keyword.rs +++ b/src/keyword.rs @@ -5,7 +5,6 @@ use super::*; pub(crate) enum Keyword { Alias, AllowDuplicateRecipes, - CacheFilename, DotenvFilename, DotenvLoad, DotenvPath, diff --git a/src/node.rs b/src/node.rs index 05ba3841e6..1d98b99173 100644 --- a/src/node.rs +++ b/src/node.rs @@ -281,10 +281,7 @@ impl<'src> Node<'src> for Set<'src> { set.push_mut(Tree::string(&argument.cooked)); } } - Setting::CacheFilename(value) - | Setting::DotenvFilename(value) - | Setting::DotenvPath(value) - | Setting::Tempdir(value) => { + Setting::DotenvFilename(value) | Setting::DotenvPath(value) | Setting::Tempdir(value) => { set.push_mut(Tree::string(value)); } } diff --git a/src/parser.rs b/src/parser.rs index 2b90f7e9f5..ba006ac5bd 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -864,7 +864,6 @@ impl<'run, 'src> Parser<'run, 'src> { self.expect(ColonEquals)?; let set_value = match keyword { - Keyword::CacheFilename => Some(Setting::CacheFilename(self.parse_string_literal()?.cooked)), Keyword::DotenvFilename => Some(Setting::DotenvFilename(self.parse_string_literal()?.cooked)), Keyword::DotenvPath => Some(Setting::DotenvPath(self.parse_string_literal()?.cooked)), Keyword::Shell => Some(Setting::Shell(self.parse_shell()?)), diff --git a/src/recipe.rs b/src/recipe.rs index 78d212724d..8db1f97d97 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -155,26 +155,23 @@ impl<'src, D> Recipe<'src, D> { context: &RecipeContext<'src, 'run>, mut evaluator: Evaluator<'src, 'run>, ) -> RunResult<'src, Option<(PathBuf, JustfileCache)>> { - let cache_filename = match &context.settings.cache_filename { - Some(cache_filename) => context.search.working_directory.join(cache_filename), - None => { - let project_dir = &context.search.working_directory; - let project_name = project_dir - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("UNKNOWN_PROJECT"); - let mut path_hash = blake3::Hasher::new(); - path_hash.update(project_dir.as_os_str().as_bytes()); - path_hash.update(context.search.justfile.as_os_str().as_bytes()); - let path_hash = &path_hash.finalize().to_hex()[..16]; - - dirs::cache_dir() - .ok_or(Error::CacheFileRead { - cache_filename: None, - })? - .join("justfiles") - .join(format!("{project_name}-{path_hash}.json")) - } + let cache_filename = { + let project_dir = &context.search.working_directory; + let project_name = project_dir + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("UNKNOWN_PROJECT"); + let mut path_hash = blake3::Hasher::new(); + path_hash.update(project_dir.as_os_str().as_bytes()); + path_hash.update(context.search.justfile.as_os_str().as_bytes()); + let path_hash = &path_hash.finalize().to_hex()[..16]; + + dirs::cache_dir() + .ok_or(Error::CacheFileRead { + cache_filename: None, + })? + .join("justfiles") + .join(format!("{project_name}-{path_hash}.json")) }; let mut cache = if !cache_filename.exists() { diff --git a/src/setting.rs b/src/setting.rs index 6d0c0f05b9..0256ff94c9 100644 --- a/src/setting.rs +++ b/src/setting.rs @@ -3,7 +3,6 @@ use super::*; #[derive(Debug, Clone)] pub(crate) enum Setting<'src> { AllowDuplicateRecipes(bool), - CacheFilename(String), DotenvFilename(String), DotenvLoad(bool), DotenvPath(String), @@ -30,10 +29,7 @@ impl<'src> Display for Setting<'src> { | Setting::Quiet(value) | Setting::WindowsPowerShell(value) => write!(f, "{value}"), Setting::Shell(shell) | Setting::WindowsShell(shell) => write!(f, "{shell}"), - Setting::CacheFilename(value) - | Setting::DotenvFilename(value) - | Setting::DotenvPath(value) - | Setting::Tempdir(value) => { + Setting::DotenvFilename(value) | Setting::DotenvPath(value) | Setting::Tempdir(value) => { write!(f, "{value:?}") } } diff --git a/src/settings.rs b/src/settings.rs index 73e9728109..f72cb5465e 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -8,7 +8,6 @@ pub(crate) const WINDOWS_POWERSHELL_ARGS: &[&str] = &["-NoLogo", "-Command"]; #[derive(Debug, PartialEq, Serialize, Default)] pub(crate) struct Settings<'src> { pub(crate) allow_duplicate_recipes: bool, - pub(crate) cache_filename: Option, pub(crate) dotenv_filename: Option, pub(crate) dotenv_load: Option, pub(crate) dotenv_path: Option, @@ -32,9 +31,6 @@ impl<'src> Settings<'src> { Setting::AllowDuplicateRecipes(allow_duplicate_recipes) => { settings.allow_duplicate_recipes = allow_duplicate_recipes; } - Setting::CacheFilename(cache_filename) => { - settings.cache_filename = Some(cache_filename); - } Setting::DotenvFilename(filename) => { settings.dotenv_filename = Some(filename); } From ed8c05dac7adf752e36049d970e1fbf8b52ebff0 Mon Sep 17 00:00:00 2001 From: Noah May Date: Wed, 6 Mar 2024 22:50:15 -0600 Subject: [PATCH 12/33] Use .as_encoded_bytes() on os strings --- src/recipe.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index 8db1f97d97..0272eb2ccb 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -1,9 +1,6 @@ use { super::*, - std::{ - os::unix::ffi::OsStrExt, - process::{ExitStatus, Stdio}, - }, + std::process::{ExitStatus, Stdio}, }; /// Return a `Error::Signal` if the process was terminated by a signal, @@ -162,8 +159,8 @@ impl<'src, D> Recipe<'src, D> { .and_then(|name| name.to_str()) .unwrap_or("UNKNOWN_PROJECT"); let mut path_hash = blake3::Hasher::new(); - path_hash.update(project_dir.as_os_str().as_bytes()); - path_hash.update(context.search.justfile.as_os_str().as_bytes()); + path_hash.update(project_dir.as_os_str().as_encoded_bytes()); + path_hash.update(context.search.justfile.as_os_str().as_encoded_bytes()); let path_hash = &path_hash.finalize().to_hex()[..16]; dirs::cache_dir() From bbf7f84ce726111901d44b5530df147057fda3fc Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 00:08:38 -0600 Subject: [PATCH 13/33] Fix tests --- tests/cached_recipes.rs | 147 +++++++++++++++++++--------------------- tests/json.rs | 20 ------ 2 files changed, 71 insertions(+), 96 deletions(-) diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index b2cf9b0a48..1136534c14 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -1,40 +1,70 @@ use super::*; -fn build_test(justfile: &str, parent: &TempDir) -> Test { - Test::with_tempdir(TempDir::new_in(&parent).expect("to create tempdir")).justfile(justfile) +struct ReuseableTest { + test: Test, + justfile: &'static str, +} + +impl ReuseableTest { + pub(crate) fn new(justfile: &'static str) -> Self { + Self { + test: Test::new().justfile(justfile), + justfile, + } + } + + fn new_with_test(justfile: &'static str, test: Test) -> Self { + Self { test, justfile } + } + + pub(crate) fn map(self, map: impl FnOnce(Test) -> Test) -> Self { + Self::new_with_test(self.justfile, map(self.test)) + } + + pub(crate) fn run(self) -> Self { + let justfile = self.justfile; + let Output { tempdir, .. } = self.test.run(); + Self::new_with_test(justfile, Test::with_tempdir(tempdir).justfile(justfile)) + } +} + +fn skipped_message<'run>(recipe_name: &str) -> String { + format!( + "===> Hash of recipe body of `{}` matches last run. Skipping...\n", + recipe_name + ) } #[test] -fn cached_recipes_are_actually_cached() { +fn cached_recipes_are_cached() { let justfile = r#" - set cache-filename := "../local_cache.json" [cached] echo: @echo cached "#; - let parent = tempdir(); - build_test(justfile, &parent).stdout("cached\n").run(); - build_test(justfile, &parent).stdout("").run(); + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper.map(|test| test.stdout("cached\n")).run(); + let _wrapper = wrapper + .map(|test| test.stderr(&skipped_message("echo"))) + .run(); } #[test] -fn uncached_recipes_are_not_uncached() { +fn uncached_recipes_are_uncached() { let justfile = r#" - set cache-filename := "../local_cache.json" @echo: echo uncached "#; - let parent = tempdir(); - build_test(justfile, &parent).stdout("uncached\n").run(); - build_test(justfile, &parent).stdout("uncached\n").run(); + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper.map(|test| test.stdout("uncached\n")).run(); + let _wrapper = wrapper.map(|test| test.stdout("uncached\n")).run(); } #[test] fn cached_recipes_are_independent() { let justfile = r#" - set cache-filename := "../local_cache.json" [cached] echo1: @@ -44,81 +74,56 @@ fn cached_recipes_are_independent() { echo2: @echo cached2 "#; - let parent = tempdir(); - build_test(justfile, &parent) - .arg("echo1") - .stdout("cached1\n") + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| test.arg("echo1").stdout("cached1\n")) + .run(); + let wrapper = wrapper + .map(|test| test.arg("echo2").stdout("cached2\n")) .run(); - build_test(justfile, &parent) - .arg("echo2") - .stdout("cached2\n") + let wrapper = wrapper + .map(|test| test.arg("echo1").stderr(&skipped_message("echo1"))) + .run(); + let _wrapper = wrapper + .map(|test| test.arg("echo2").stderr(&skipped_message("echo2"))) .run(); - build_test(justfile, &parent).arg("echo1").stdout("").run(); - build_test(justfile, &parent).arg("echo2").stdout("").run(); } #[test] fn arguments_and_variables_are_part_of_cache_hash() { let justfile = r#" - set cache-filename := "../local_cache.json" my-var := "1" [cached] echo ARG: @echo {{ARG}}{{my-var}} "#; - let parent = tempdir(); - build_test(justfile, &parent) - .args(["echo", "a"]) - .stdout("a1\n") + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| test.args(["echo", "a"]).stdout("a1\n")) .run(); - build_test(justfile, &parent) - .args(["echo", "a"]) - .stdout("") + let wrapper = wrapper + .map(|test| test.args(["echo", "a"]).stderr(&skipped_message("echo"))) .run(); - build_test(justfile, &parent) - .args(["echo", "b"]) - .stdout("b1\n") + let wrapper = wrapper + .map(|test| test.args(["echo", "b"]).stdout("b1\n")) .run(); - build_test(justfile, &parent) - .args(["echo", "b"]) - .stdout("") + let wrapper = wrapper + .map(|test| test.args(["echo", "b"]).stderr(&skipped_message("echo"))) .run(); - build_test(justfile, &parent) - .args(["my-var=2", "echo", "b"]) - .stdout("b2\n") + let wrapper = wrapper + .map(|test| test.args(["my-var=2", "echo", "b"]).stdout("b2\n")) .run(); - build_test(justfile, &parent) - .args(["my-var=2", "echo", "b"]) - .stdout("") + let _wrapper = wrapper + .map(|test| { + test + .args(["my-var=2", "echo", "b"]) + .stderr(&skipped_message("echo")) + }) .run(); } -#[test] -fn invalid_recipe_errors() { - let commands = [ - ("@echo {{`echo deja vu`}}", "a backtick expression"), - ("@echo uuid4: {{uuid()}}", "a call to `uuid`"), - ("@echo process_id: {{just_pid()}}", "a call to `just_pid`"), - ]; - - for (command, invalid) in commands { - Test::new() - .justfile(format!( - r#" - set cache-filename := "local_cache.json" - [cached] - invalid: - {command} - "# - )) - .stderr(format!("error: Cached recipe `invalid` contains {invalid}, which could run multiple times.\nYou must inline it if possible or set it to a variable outside of the recipe block: my_var := ...\n")) - .status(EXIT_FAILURE) - .run(); - } -} - #[test] fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() { assert!(false); @@ -128,13 +133,3 @@ fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() { fn cached_deps_cannot_depend_on_preceding_uncached_ones() { assert!(false); } - -#[test] -fn delete_cache_works() { - assert!(false); -} - -#[test] -fn default_cache_location_works() { - assert!(false); -} diff --git a/tests/json.rs b/tests/json.rs index d417a2ceba..e662ec5fc3 100644 --- a/tests/json.rs +++ b/tests/json.rs @@ -44,8 +44,6 @@ fn alias() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -82,7 +80,6 @@ fn assignment() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -134,7 +131,6 @@ fn body() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -198,7 +194,6 @@ fn dependencies() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -299,7 +294,6 @@ fn dependency_argument() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -363,7 +357,6 @@ fn duplicate_recipes() { }, "settings": { "allow_duplicate_recipes": true, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -408,7 +401,6 @@ fn doc_comment() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -439,7 +431,6 @@ fn empty_justfile() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -591,7 +582,6 @@ fn parameters() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -676,7 +666,6 @@ fn priors() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -721,7 +710,6 @@ fn private() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -766,7 +754,6 @@ fn quiet() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -790,7 +777,6 @@ fn settings() { case( r#" set dotenv-load - set cache-filename := "cache-filename" set dotenv-filename := "dotenv-filename" set dotenv-path := "path" set export @@ -824,7 +810,6 @@ fn settings() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": "cache-filename", "dotenv_filename": "dotenv-filename", "dotenv_load": true, "dotenv_path": "path", @@ -875,7 +860,6 @@ fn shebang() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -920,7 +904,6 @@ fn simple() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -968,7 +951,6 @@ fn attribute() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -1029,7 +1011,6 @@ fn module() { }, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, @@ -1049,7 +1030,6 @@ fn module() { "recipes": {}, "settings": { "allow_duplicate_recipes": false, - "cache_filename": null, "dotenv_filename": null, "dotenv_load": null, "dotenv_path": null, From cf3c95a5a565c52046516844dd2c5871462ba6d2 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 00:16:54 -0600 Subject: [PATCH 14/33] Make cached_recipes require unstable --- src/recipe.rs | 2 ++ tests/cached_recipes.rs | 65 ++++++++++++++++++++++++++++++++++------- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index 0272eb2ccb..aeed7f8824 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -250,6 +250,8 @@ impl<'src, D> Recipe<'src, D> { let updated_cache = if !self.cached() { None } else { + config.require_unstable("Cached recipes are currently unstable.")?; + let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); match self.get_updated_cache_if_outdated(context, evaluator)? { None => { diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index 1136534c14..39ab4619f4 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -35,6 +35,21 @@ fn skipped_message<'run>(recipe_name: &str) -> String { ) } +#[test] +fn cached_recipes_are_unstable() { + let justfile = r#" + [cached] + echo: + @echo cached + "#; + + Test::new() + .justfile(justfile) + .stderr("error: Cached recipes are currently unstable. Invoke `just` with the `--unstable` flag to enable unstable features.\n") + .status(EXIT_FAILURE) + .run(); +} + #[test] fn cached_recipes_are_cached() { let justfile = r#" @@ -44,9 +59,11 @@ fn cached_recipes_are_cached() { "#; let wrapper = ReuseableTest::new(justfile); - let wrapper = wrapper.map(|test| test.stdout("cached\n")).run(); + let wrapper = wrapper + .map(|test| test.arg("--unstable").stdout("cached\n")) + .run(); let _wrapper = wrapper - .map(|test| test.stderr(&skipped_message("echo"))) + .map(|test| test.arg("--unstable").stderr(&skipped_message("echo"))) .run(); } @@ -77,16 +94,26 @@ fn cached_recipes_are_independent() { let wrapper = ReuseableTest::new(justfile); let wrapper = wrapper - .map(|test| test.arg("echo1").stdout("cached1\n")) + .map(|test| test.arg("--unstable").arg("echo1").stdout("cached1\n")) .run(); let wrapper = wrapper - .map(|test| test.arg("echo2").stdout("cached2\n")) + .map(|test| test.arg("--unstable").arg("echo2").stdout("cached2\n")) .run(); let wrapper = wrapper - .map(|test| test.arg("echo1").stderr(&skipped_message("echo1"))) + .map(|test| { + test + .arg("--unstable") + .arg("echo1") + .stderr(&skipped_message("echo1")) + }) .run(); let _wrapper = wrapper - .map(|test| test.arg("echo2").stderr(&skipped_message("echo2"))) + .map(|test| { + test + .arg("--unstable") + .arg("echo2") + .stderr(&skipped_message("echo2")) + }) .run(); } @@ -101,23 +128,39 @@ fn arguments_and_variables_are_part_of_cache_hash() { let wrapper = ReuseableTest::new(justfile); let wrapper = wrapper - .map(|test| test.args(["echo", "a"]).stdout("a1\n")) + .map(|test| test.arg("--unstable").args(["echo", "a"]).stdout("a1\n")) .run(); let wrapper = wrapper - .map(|test| test.args(["echo", "a"]).stderr(&skipped_message("echo"))) + .map(|test| { + test + .arg("--unstable") + .args(["echo", "a"]) + .stderr(&skipped_message("echo")) + }) .run(); let wrapper = wrapper - .map(|test| test.args(["echo", "b"]).stdout("b1\n")) + .map(|test| test.arg("--unstable").args(["echo", "b"]).stdout("b1\n")) .run(); let wrapper = wrapper - .map(|test| test.args(["echo", "b"]).stderr(&skipped_message("echo"))) + .map(|test| { + test + .arg("--unstable") + .args(["echo", "b"]) + .stderr(&skipped_message("echo")) + }) .run(); let wrapper = wrapper - .map(|test| test.args(["my-var=2", "echo", "b"]).stdout("b2\n")) + .map(|test| { + test + .arg("--unstable") + .args(["my-var=2", "echo", "b"]) + .stdout("b2\n") + }) .run(); let _wrapper = wrapper .map(|test| { test + .arg("--unstable") .args(["my-var=2", "echo", "b"]) .stderr(&skipped_message("echo")) }) From 36482fd91624b81081af62ba4110f9358f43d3de Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 01:40:13 -0600 Subject: [PATCH 15/33] Put cache files in working directory by default --- src/recipe.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index aeed7f8824..9f0dfd542d 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -163,11 +163,8 @@ impl<'src, D> Recipe<'src, D> { path_hash.update(context.search.justfile.as_os_str().as_encoded_bytes()); let path_hash = &path_hash.finalize().to_hex()[..16]; - dirs::cache_dir() - .ok_or(Error::CacheFileRead { - cache_filename: None, - })? - .join("justfiles") + project_dir + .join(".justcache") .join(format!("{project_name}-{path_hash}.json")) }; From d5edf65e637320786ace319450e67a40ae2177f7 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 03:17:47 -0600 Subject: [PATCH 16/33] Added versioning to cache files --- src/cache.rs | 37 +++++++++++++++++++++++++++++++------ src/error.rs | 9 +++------ src/lib.rs | 6 +++--- src/recipe.rs | 20 ++++++++++++-------- tests/cached_recipes.rs | 32 +++++++++++++++++++++++++++----- 5 files changed, 76 insertions(+), 28 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 7974720614..be5802bda7 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -1,18 +1,32 @@ +use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::path::PathBuf; -use serde::{Deserialize, Serialize}; - use super::*; #[derive(Debug, Serialize, Deserialize)] -pub(crate) struct JustfileCache { +#[serde(tag = "version")] +pub(crate) enum JustfileCacheSerialized { + #[serde(rename = "unstable-1")] + Unstable1(JustfileCacheUnstable1), +} + +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct JustfileCacheUnstable1 { /// Only serialized for user debugging pub(crate) justfile_path: PathBuf, + /// Only serialized for user debugging pub(crate) working_directory: PathBuf, + pub(crate) recipes: HashMap, } +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct RecipeCache { + pub(crate) body_hash: String, +} + +pub(crate) type JustfileCache = JustfileCacheUnstable1; impl JustfileCache { pub(crate) fn new(search: &Search) -> Self { Self { @@ -23,7 +37,18 @@ impl JustfileCache { } } -#[derive(Debug, Serialize, Deserialize)] -pub(crate) struct RecipeCache { - pub(crate) body_hash: String, +impl From for JustfileCacheSerialized { + fn from(value: JustfileCache) -> Self { + JustfileCacheSerialized::Unstable1(value) + } +} + +impl TryFrom for JustfileCache { + type Error = Error<'static>; + + fn try_from(value: JustfileCacheSerialized) -> Result { + match value { + JustfileCacheSerialized::Unstable1(unstable1) => Ok(unstable1), + } + } } diff --git a/src/error.rs b/src/error.rs index c7e951925d..f2d2e0cfc6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -18,7 +18,8 @@ pub(crate) enum Error<'src> { output_error: OutputError, }, CacheFileRead { - cache_filename: Option, + cache_filename: PathBuf, + io_error: io::Error, }, CacheFileWrite { cache_filename: PathBuf, @@ -274,11 +275,7 @@ impl<'src> ColorDisplay for Error<'src> { }?, OutputError::Utf8(utf8_error) => write!(f, "Backtick succeeded but stdout was not utf8: {utf8_error}")?, } - CacheFileRead {cache_filename} => match cache_filename { - Some(cache_filename) => - write!(f, "Failed to read cache file: {}", cache_filename.display())?, - None => write!(f, "Failed to get default cache file")?, - } + CacheFileRead {cache_filename, io_error} => write!(f, "Failed to read cache file: {}\nio_error: {}", cache_filename.display(), io_error)?, CacheFileWrite{cache_filename, io_error} => write!(f, "Failed to write cache file ({}): {io_error}", cache_filename.display())?, ChooserInvoke { shell_binary, shell_arguments, chooser, io_error} => { let chooser = chooser.to_string_lossy(); diff --git a/src/lib.rs b/src/lib.rs index 5559bff3dc..c63285a29d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,9 +17,9 @@ pub(crate) use { crate::{ alias::Alias, analyzer::Analyzer, assignment::Assignment, assignment_resolver::AssignmentResolver, ast::Ast, attribute::Attribute, binding::Binding, - cache::JustfileCache, cache::RecipeCache, color::Color, color_display::ColorDisplay, - command_ext::CommandExt, compilation::Compilation, compile_error::CompileError, - compile_error_kind::CompileErrorKind, compiler::Compiler, + cache::JustfileCache, cache::JustfileCacheSerialized, cache::RecipeCache, color::Color, + color_display::ColorDisplay, command_ext::CommandExt, compilation::Compilation, + compile_error::CompileError, compile_error_kind::CompileErrorKind, compiler::Compiler, conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, diff --git a/src/recipe.rs b/src/recipe.rs index 9f0dfd542d..b7c89f3fde 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -171,14 +171,17 @@ impl<'src, D> Recipe<'src, D> { let mut cache = if !cache_filename.exists() { JustfileCache::new(context.search) } else { - fs::read_to_string(&cache_filename) - .or(Err(())) - .and_then(|cache| serde_json::from_str(&cache).or(Err(()))) - .or_else(|_| { - Err(Error::CacheFileRead { - cache_filename: Some(cache_filename.clone()), - }) - })? + let file_contents = fs::read_to_string(&cache_filename).or_else(|io_error| { + Err(Error::CacheFileRead { + cache_filename: cache_filename.clone(), + io_error, + }) + })?; + serde_json::from_str(&file_contents).map_or_else( + // Ignore unknown versions or corrupted cache files + |_parse_error| Ok(JustfileCache::new(context.search)), + |serialized: JustfileCacheSerialized| serialized.try_into(), + )? }; // TODO: Prevent double work/evaluating twice @@ -207,6 +210,7 @@ impl<'src, D> Recipe<'src, D> { cache_filename: PathBuf, cache: JustfileCache, ) -> RunResult<'src, ()> { + let cache: JustfileCacheSerialized = cache.into(); let cache = serde_json::to_string(&cache).or_else(|_| { Err(Error::Internal { message: format!("Failed to serialize cache: {cache:?}"), diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index 39ab4619f4..c8659b81aa 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -168,11 +168,33 @@ fn arguments_and_variables_are_part_of_cache_hash() { } #[test] -fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() { - assert!(false); +fn invalid_cache_files_are_ignored() { + let justfile = r#" + [cached] + echo: + @echo cached + "#; + + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| test.arg("--unstable").stdout("cached\n")) + .run(); + + let cache_dir = wrapper.test.tempdir.path().join(".justcache"); + let mut caches = std::fs::read_dir(cache_dir).expect("could not read cache dir"); + let cached_recipe = caches.next().expect("no recipe cache file").unwrap().path(); + std::fs::write(cached_recipe, r#"{"invalid_cache_format": true}"#).unwrap(); + + let _wrapper = wrapper + .map(|test| test.arg("--unstable").stdout("cached\n")) + .run(); } #[test] -fn cached_deps_cannot_depend_on_preceding_uncached_ones() { - assert!(false); -} +fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() {} + +#[test] +fn cached_deps_cannot_depend_on_preceding_uncached_ones() {} + +#[test] +fn failed_runs_should_not_update_cache() {} From f32cc867c7119d3bad267fc963a5e7c781be01e6 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 03:36:03 -0600 Subject: [PATCH 17/33] Make errors look the same --- src/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index f2d2e0cfc6..d3849d5b6a 100644 --- a/src/error.rs +++ b/src/error.rs @@ -275,8 +275,8 @@ impl<'src> ColorDisplay for Error<'src> { }?, OutputError::Utf8(utf8_error) => write!(f, "Backtick succeeded but stdout was not utf8: {utf8_error}")?, } - CacheFileRead {cache_filename, io_error} => write!(f, "Failed to read cache file: {}\nio_error: {}", cache_filename.display(), io_error)?, - CacheFileWrite{cache_filename, io_error} => write!(f, "Failed to write cache file ({}): {io_error}", cache_filename.display())?, + CacheFileRead {cache_filename, io_error} => write!(f, "Failed to read cache file ({}): {}", cache_filename.display(), io_error)?, + CacheFileWrite{cache_filename, io_error} => write!(f, "Failed to write cache file ({}): {}", cache_filename.display(), io_error)?, ChooserInvoke { shell_binary, shell_arguments, chooser, io_error} => { let chooser = chooser.to_string_lossy(); write!(f, "Chooser `{shell_binary} {shell_arguments} {chooser}` invocation failed: {io_error}")?; From 219c646f73ebd42b3528879f892cbf046d3bde3d Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 03:36:40 -0600 Subject: [PATCH 18/33] Revert "Add ExpressionWalker and refactor Variables" This reverts commit a649e1d0b64c43ba6713d0b6fa88648bcb834f8a. --- src/expression.rs | 4 -- src/expression_walker.rs | 83 ---------------------------------------- src/lib.rs | 14 +++---- src/variables.rs | 68 ++++++++++++++++++++++++++++---- 4 files changed, 67 insertions(+), 102 deletions(-) delete mode 100644 src/expression_walker.rs diff --git a/src/expression.rs b/src/expression.rs index cfd787c9b2..58f1441e51 100644 --- a/src/expression.rs +++ b/src/expression.rs @@ -45,10 +45,6 @@ impl<'src> Expression<'src> { pub(crate) fn variables<'expression>(&'expression self) -> Variables<'expression, 'src> { Variables::new(self) } - - pub(crate) fn walk<'expression>(&'expression self) -> ExpressionWalker<'expression, 'src> { - ExpressionWalker::new(self) - } } impl<'src> Display for Expression<'src> { diff --git a/src/expression_walker.rs b/src/expression_walker.rs deleted file mode 100644 index 72751095e7..0000000000 --- a/src/expression_walker.rs +++ /dev/null @@ -1,83 +0,0 @@ -use super::*; - -pub(crate) struct ExpressionWalker<'expression, 'src> { - stack: Vec<&'expression Expression<'src>>, -} - -impl<'expression, 'src> ExpressionWalker<'expression, 'src> { - pub(crate) fn new(root: &'expression Expression<'src>) -> ExpressionWalker<'expression, 'src> { - ExpressionWalker { stack: vec![root] } - } -} - -impl<'expression, 'src> Iterator for ExpressionWalker<'expression, 'src> { - type Item = &'expression Expression<'src>; - - fn next(&mut self) -> Option { - let top = self.stack.pop()?; - - match top { - Expression::StringLiteral { .. } - | Expression::Variable { .. } - | Expression::Backtick { .. } => {} - Expression::Call { thunk } => match thunk { - Thunk::Nullary { .. } => {} - Thunk::Unary { arg, .. } => self.stack.push(arg), - Thunk::UnaryOpt { - args: (a, opt_b), .. - } => { - self.stack.push(a); - if let Some(b) = opt_b.as_ref() { - self.stack.push(b); - } - } - Thunk::Binary { args, .. } => { - for arg in args.iter().rev() { - self.stack.push(arg); - } - } - Thunk::BinaryPlus { - args: ([a, b], rest), - .. - } => { - let first: &[&Expression] = &[a, b]; - for arg in first.iter().copied().chain(rest).rev() { - self.stack.push(arg); - } - } - Thunk::Ternary { args, .. } => { - for arg in args.iter().rev() { - self.stack.push(arg); - } - } - }, - Expression::Conditional { - lhs, - rhs, - then, - otherwise, - .. - } => { - self.stack.push(otherwise); - self.stack.push(then); - self.stack.push(rhs); - self.stack.push(lhs); - } - Expression::Concatenation { lhs, rhs } => { - self.stack.push(rhs); - self.stack.push(lhs); - } - Expression::Join { lhs, rhs } => { - self.stack.push(rhs); - if let Some(lhs) = lhs { - self.stack.push(lhs); - } - } - Expression::Group { contents } => { - self.stack.push(contents); - } - } - - Some(top) - } -} diff --git a/src/lib.rs b/src/lib.rs index c63285a29d..7b13469870 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -23,13 +23,12 @@ pub(crate) use { conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, - expression_walker::ExpressionWalker, fragment::Fragment, function::Function, - function_context::FunctionContext, interrupt_guard::InterruptGuard, - interrupt_handler::InterruptHandler, item::Item, justfile::Justfile, keyed::Keyed, - keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, - loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, output::output, - output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, - platform::Platform, platform_interface::PlatformInterface, position::Position, + fragment::Fragment, function::Function, function_context::FunctionContext, + interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, + justfile::Justfile, keyed::Keyed, keyword::Keyword, lexer::Lexer, line::Line, list::List, + load_dotenv::load_dotenv, loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, + output::output, output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, + parser::Parser, platform::Platform, platform_interface::PlatformInterface, position::Position, positional::Positional, ran::Ran, range_ext::RangeExt, recipe::Recipe, recipe_context::RecipeContext, recipe_resolver::RecipeResolver, scope::Scope, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, @@ -138,7 +137,6 @@ mod enclosure; mod error; mod evaluator; mod expression; -mod expression_walker; mod fragment; mod function; mod function_context; diff --git a/src/variables.rs b/src/variables.rs index c9456c75dc..8a17254a23 100644 --- a/src/variables.rs +++ b/src/variables.rs @@ -1,14 +1,12 @@ use super::*; pub(crate) struct Variables<'expression, 'src> { - walker: ExpressionWalker<'expression, 'src>, + stack: Vec<&'expression Expression<'src>>, } impl<'expression, 'src> Variables<'expression, 'src> { pub(crate) fn new(root: &'expression Expression<'src>) -> Variables<'expression, 'src> { - Variables { - walker: root.walk(), - } + Variables { stack: vec![root] } } } @@ -17,9 +15,65 @@ impl<'expression, 'src> Iterator for Variables<'expression, 'src> { fn next(&mut self) -> Option> { loop { - match self.walker.next()? { - Expression::Variable { name } => return Some(name.token), - _ => continue, + match self.stack.pop()? { + Expression::StringLiteral { .. } | Expression::Backtick { .. } => {} + Expression::Call { thunk } => match thunk { + Thunk::Nullary { .. } => {} + Thunk::Unary { arg, .. } => self.stack.push(arg), + Thunk::UnaryOpt { + args: (a, opt_b), .. + } => { + self.stack.push(a); + if let Some(b) = opt_b.as_ref() { + self.stack.push(b); + } + } + Thunk::Binary { args, .. } => { + for arg in args.iter().rev() { + self.stack.push(arg); + } + } + Thunk::BinaryPlus { + args: ([a, b], rest), + .. + } => { + let first: &[&Expression] = &[a, b]; + for arg in first.iter().copied().chain(rest).rev() { + self.stack.push(arg); + } + } + Thunk::Ternary { args, .. } => { + for arg in args.iter().rev() { + self.stack.push(arg); + } + } + }, + Expression::Conditional { + lhs, + rhs, + then, + otherwise, + .. + } => { + self.stack.push(otherwise); + self.stack.push(then); + self.stack.push(rhs); + self.stack.push(lhs); + } + Expression::Variable { name, .. } => return Some(name.token), + Expression::Concatenation { lhs, rhs } => { + self.stack.push(rhs); + self.stack.push(lhs); + } + Expression::Join { lhs, rhs } => { + self.stack.push(rhs); + if let Some(lhs) = lhs { + self.stack.push(lhs); + } + } + Expression::Group { contents } => { + self.stack.push(contents); + } } } } From 3a341ea801d9314baf1d8b91a8c425db235ecd9b Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 13:19:49 -0600 Subject: [PATCH 19/33] Clarify error messages --- src/cache.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index be5802bda7..58f3c77555 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -11,8 +11,10 @@ pub(crate) enum JustfileCacheSerialized { Unstable1(JustfileCacheUnstable1), } +pub(crate) type JustfileCacheUnstable1 = JustfileCache; + #[derive(Debug, Serialize, Deserialize)] -pub(crate) struct JustfileCacheUnstable1 { +pub(crate) struct JustfileCache { /// Only serialized for user debugging pub(crate) justfile_path: PathBuf, /// Only serialized for user debugging @@ -26,7 +28,6 @@ pub(crate) struct RecipeCache { pub(crate) body_hash: String, } -pub(crate) type JustfileCache = JustfileCacheUnstable1; impl JustfileCache { pub(crate) fn new(search: &Search) -> Self { Self { From 9acd827718e91b6b86eacbe256e5bcf0877cdc6f Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 7 Mar 2024 13:27:22 -0600 Subject: [PATCH 20/33] Incompatible old versions of cache should also silently be ignored --- src/cache.rs | 2 +- src/recipe.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 58f3c77555..88afdcff9e 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -45,7 +45,7 @@ impl From for JustfileCacheSerialized { } impl TryFrom for JustfileCache { - type Error = Error<'static>; + type Error = (); fn try_from(value: JustfileCacheSerialized) -> Result { match value { diff --git a/src/recipe.rs b/src/recipe.rs index b7c89f3fde..66a12f7624 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -177,11 +177,11 @@ impl<'src, D> Recipe<'src, D> { io_error, }) })?; - serde_json::from_str(&file_contents).map_or_else( - // Ignore unknown versions or corrupted cache files - |_parse_error| Ok(JustfileCache::new(context.search)), - |serialized: JustfileCacheSerialized| serialized.try_into(), - )? + // Ignore newer versions, incompatible old versions or corrupted cache files + serde_json::from_str(&file_contents) + .or(Err(())) + .and_then(|serialized: JustfileCacheSerialized| serialized.try_into().or(Err(()))) + .unwrap_or_else(|_| JustfileCache::new(context.search)) }; // TODO: Prevent double work/evaluating twice From 0812866dc3d4973682b80a04aa18e1d2539a931b Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 14 Mar 2024 14:39:29 -0500 Subject: [PATCH 21/33] Doc comments --- src/cache.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cache.rs b/src/cache.rs index 88afdcff9e..1d2c7fee00 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -4,6 +4,8 @@ use std::path::PathBuf; use super::*; +/// The version of the justfile as it is on disk. Newer cache formats are added +/// as new variants. #[derive(Debug, Serialize, Deserialize)] #[serde(tag = "version")] pub(crate) enum JustfileCacheSerialized { @@ -13,6 +15,8 @@ pub(crate) enum JustfileCacheSerialized { pub(crate) type JustfileCacheUnstable1 = JustfileCache; +/// The runtime cache format. It should be the intersection of all supported +/// serialized versions, i.e. you can convert any supported version to this. #[derive(Debug, Serialize, Deserialize)] pub(crate) struct JustfileCache { /// Only serialized for user debugging From 707844cecc8e979a1eb6f34502678cb22c1766b3 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 14 Mar 2024 15:06:53 -0500 Subject: [PATCH 22/33] Move cache_file into Search struct --- src/recipe.rs | 27 +++++--------------- src/search.rs | 69 +++++++++++++++++++++++++++------------------------ 2 files changed, 43 insertions(+), 53 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index 66a12f7624..d05a8f3749 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -152,35 +152,20 @@ impl<'src, D> Recipe<'src, D> { context: &RecipeContext<'src, 'run>, mut evaluator: Evaluator<'src, 'run>, ) -> RunResult<'src, Option<(PathBuf, JustfileCache)>> { - let cache_filename = { - let project_dir = &context.search.working_directory; - let project_name = project_dir - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or("UNKNOWN_PROJECT"); - let mut path_hash = blake3::Hasher::new(); - path_hash.update(project_dir.as_os_str().as_encoded_bytes()); - path_hash.update(context.search.justfile.as_os_str().as_encoded_bytes()); - let path_hash = &path_hash.finalize().to_hex()[..16]; - - project_dir - .join(".justcache") - .join(format!("{project_name}-{path_hash}.json")) - }; - - let mut cache = if !cache_filename.exists() { + let cache_file = &context.search.cache_file; + let mut cache = if !cache_file.exists() { JustfileCache::new(context.search) } else { - let file_contents = fs::read_to_string(&cache_filename).or_else(|io_error| { + let file_contents = fs::read_to_string(&cache_file).or_else(|io_error| { Err(Error::CacheFileRead { - cache_filename: cache_filename.clone(), + cache_filename: cache_file.clone(), io_error, }) })?; // Ignore newer versions, incompatible old versions or corrupted cache files serde_json::from_str(&file_contents) .or(Err(())) - .and_then(|serialized: JustfileCacheSerialized| serialized.try_into().or(Err(()))) + .and_then(|serialized: JustfileCacheSerialized| serialized.try_into()) .unwrap_or_else(|_| JustfileCache::new(context.search)) }; @@ -202,7 +187,7 @@ impl<'src, D> Recipe<'src, D> { body_hash: recipe_hash.to_string(), }, ); - Ok(Some((cache_filename, cache))) + Ok(Some((cache_file.clone(), cache))) } fn save_latest_cache( diff --git a/src/search.rs b/src/search.rs index c14eb55354..76ed9b1ee2 100644 --- a/src/search.rs +++ b/src/search.rs @@ -7,9 +7,32 @@ const PROJECT_ROOT_CHILDREN: &[&str] = &[".bzr", ".git", ".hg", ".svn", "_darcs" pub(crate) struct Search { pub(crate) justfile: PathBuf, pub(crate) working_directory: PathBuf, + pub(crate) cache_file: PathBuf, } impl Search { + fn new(justfile: PathBuf, working_directory: PathBuf) -> Self { + let cache_file = { + let project_name = working_directory + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("UNKNOWN_PROJECT"); + let mut path_hash = blake3::Hasher::new(); + path_hash.update(working_directory.as_os_str().as_encoded_bytes()); + path_hash.update(justfile.as_os_str().as_encoded_bytes()); + let path_hash = &path_hash.finalize().to_hex()[..16]; + + working_directory + .join(".justcache") + .join(format!("{project_name}-{path_hash}.json")) + }; + + Self { + justfile, + working_directory, + cache_file, + } + } pub(crate) fn find( search_config: &SearchConfig, invocation_directory: &Path, @@ -23,28 +46,22 @@ impl Search { let working_directory = Self::working_directory_from_justfile(&justfile)?; - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } SearchConfig::WithJustfile { justfile } => { let justfile = Self::clean(invocation_directory, justfile); let working_directory = Self::working_directory_from_justfile(&justfile)?; - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } SearchConfig::WithJustfileAndWorkingDirectory { justfile, working_directory, - } => Ok(Self { - justfile: Self::clean(invocation_directory, justfile), - working_directory: Self::clean(invocation_directory, working_directory), - }), + } => Ok(Self::new( + Self::clean(invocation_directory, justfile), + Self::clean(invocation_directory, working_directory), + )), } } @@ -53,10 +70,7 @@ impl Search { let working_directory = Self::working_directory_from_justfile(&justfile)?; - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } pub(crate) fn init( @@ -69,10 +83,7 @@ impl Search { let justfile = working_directory.join(DEFAULT_JUSTFILE_NAME); - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } SearchConfig::FromSearchDirectory { search_directory } => { @@ -82,10 +93,7 @@ impl Search { let justfile = working_directory.join(DEFAULT_JUSTFILE_NAME); - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } SearchConfig::WithJustfile { justfile } => { @@ -93,19 +101,16 @@ impl Search { let working_directory = Self::working_directory_from_justfile(&justfile)?; - Ok(Self { - justfile, - working_directory, - }) + Ok(Self::new(justfile, working_directory)) } SearchConfig::WithJustfileAndWorkingDirectory { justfile, working_directory, - } => Ok(Self { - justfile: Self::clean(invocation_directory, justfile), - working_directory: Self::clean(invocation_directory, working_directory), - }), + } => Ok(Self::new( + Self::clean(invocation_directory, justfile), + Self::clean(invocation_directory, working_directory), + )), } } From 70fe6ac7c036e67c28bcddc6b8719e8e3e1acf69 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 21 Mar 2024 14:50:22 -0500 Subject: [PATCH 23/33] Starting to implement cached dependencies --- src/cache.rs | 56 +++++++++++++++++++++++++- src/justfile.rs | 37 +++++++++++++++-- src/lib.rs | 24 +++++------ src/recipe.rs | 89 ++++++++--------------------------------- src/recipe_context.rs | 1 + tests/cached_recipes.rs | 6 +++ 6 files changed, 123 insertions(+), 90 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 1d2c7fee00..1ebf057bf2 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -33,13 +33,67 @@ pub(crate) struct RecipeCache { } impl JustfileCache { - pub(crate) fn new(search: &Search) -> Self { + fn new_empty(search: &Search) -> Self { Self { justfile_path: search.justfile.clone(), working_directory: search.working_directory.clone(), recipes: HashMap::new(), } } + + pub(crate) fn new<'run>(search: &Search) -> RunResult<'run, Self> { + let cache_file = &search.cache_file; + let this = if !cache_file.exists() { + Self::new_empty(search) + } else { + let file_contents = fs::read_to_string(&cache_file).or_else(|io_error| { + Err(Error::CacheFileRead { + cache_filename: cache_file.clone(), + io_error, + }) + })?; + // Ignore newer versions, incompatible old versions or corrupted cache files + serde_json::from_str(&file_contents) + .or(Err(())) + .and_then(|serialized: JustfileCacheSerialized| serialized.try_into()) + .unwrap_or_else(|_| Self::new_empty(search)) + }; + Ok(this) + } + + pub(crate) fn insert_recipe(&mut self, name: String, body_hash: String) { + self.recipes.insert(name, RecipeCache { body_hash }); + } + + pub(crate) fn save<'run>(self, search: &Search) -> RunResult<'run, ()> { + let cache: JustfileCacheSerialized = self.into(); + let cache = serde_json::to_string(&cache).or_else(|_| { + Err(Error::Internal { + message: format!("Failed to serialize cache: {cache:?}"), + }) + })?; + + search + .cache_file + .parent() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::Unsupported, + format!( + "Cannot create parent directory of {}", + search.cache_file.display() + ), + ) + }) + .and_then(|parent| fs::create_dir_all(parent)) + .and_then(|_| fs::write(&search.cache_file, cache)) + .or_else(|io_error| { + Err(Error::CacheFileWrite { + cache_filename: search.cache_file.clone(), + io_error, + }) + }) + } } impl From for JustfileCacheSerialized { diff --git a/src/justfile.rs b/src/justfile.rs index f8f24550af..788eb7db9d 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -272,12 +272,16 @@ impl<'src> Justfile<'src> { } let mut ran = Ran::default(); + let mut cache = JustfileCache::new(search)?; + let mut recipes_hashes = vec![]; + for invocation in invocations { let context = RecipeContext { settings: invocation.settings, config, scope: invocation.scope, search, + cache: &cache, }; Self::run_recipe( @@ -292,10 +296,15 @@ impl<'src> Justfile<'src> { &mut ran, invocation.recipe, search, + &mut recipes_hashes, )?; } - Ok(()) + for (name, body_hash) in recipes_hashes { + cache.insert_recipe(name, body_hash); + } + + cache.save(search) } pub(crate) fn get_alias(&self, name: &str) -> Option<&Alias<'src>> { @@ -408,6 +417,7 @@ impl<'src> Justfile<'src> { ran: &mut Ran<'src>, recipe: &Recipe<'src>, search: &Search, + recipe_hashes: &mut Vec<(String, String)>, ) -> RunResult<'src> { if ran.has_run(&recipe.namepath, arguments) { return Ok(()); @@ -441,11 +451,22 @@ impl<'src> Justfile<'src> { .map(|argument| evaluator.evaluate_expression(argument)) .collect::>>()?; - Self::run_recipe(&arguments, context, dotenv, ran, recipe, search)?; + Self::run_recipe( + &arguments, + context, + dotenv, + ran, + recipe, + search, + recipe_hashes, + )?; } } - recipe.run(context, dotenv, scope.child(), search, &positional)?; + let updated_hash = recipe.run(context, dotenv, scope.child(), search, &positional)?; + if let Some((name, hash)) = updated_hash { + recipe_hashes.push((name, hash)); + } if !context.config.no_dependencies { let mut ran = Ran::default(); @@ -457,7 +478,15 @@ impl<'src> Justfile<'src> { evaluated.push(evaluator.evaluate_expression(argument)?); } - Self::run_recipe(&evaluated, context, dotenv, &mut ran, recipe, search)?; + Self::run_recipe( + &evaluated, + context, + dotenv, + &mut ran, + recipe, + search, + recipe_hashes, + )?; } } diff --git a/src/lib.rs b/src/lib.rs index 7b13469870..d67f857e58 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,18 +17,18 @@ pub(crate) use { crate::{ alias::Alias, analyzer::Analyzer, assignment::Assignment, assignment_resolver::AssignmentResolver, ast::Ast, attribute::Attribute, binding::Binding, - cache::JustfileCache, cache::JustfileCacheSerialized, cache::RecipeCache, color::Color, - color_display::ColorDisplay, command_ext::CommandExt, compilation::Compilation, - compile_error::CompileError, compile_error_kind::CompileErrorKind, compiler::Compiler, - conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, - count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, - enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, - fragment::Fragment, function::Function, function_context::FunctionContext, - interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, - justfile::Justfile, keyed::Keyed, keyword::Keyword, lexer::Lexer, line::Line, list::List, - load_dotenv::load_dotenv, loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, - output::output, output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, - parser::Parser, platform::Platform, platform_interface::PlatformInterface, position::Position, + cache::JustfileCache, color::Color, color_display::ColorDisplay, command_ext::CommandExt, + compilation::Compilation, compile_error::CompileError, compile_error_kind::CompileErrorKind, + compiler::Compiler, conditional_operator::ConditionalOperator, config::Config, + config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, + dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, + expression::Expression, fragment::Fragment, function::Function, + function_context::FunctionContext, interrupt_guard::InterruptGuard, + interrupt_handler::InterruptHandler, item::Item, justfile::Justfile, keyed::Keyed, + keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, + loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, output::output, + output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, + platform::Platform, platform_interface::PlatformInterface, position::Position, positional::Positional, ran::Ran, range_ext::RangeExt, recipe::Recipe, recipe_context::RecipeContext, recipe_resolver::RecipeResolver, scope::Scope, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, diff --git a/src/recipe.rs b/src/recipe.rs index d05a8f3749..25e7c0ff58 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -143,32 +143,15 @@ impl<'src, D> Recipe<'src, D> { self.attributes.contains(&Attribute::NoQuiet) } - fn cached(&self) -> bool { + fn should_cache(&self) -> bool { self.attributes.contains(&Attribute::Cached) } - fn get_updated_cache_if_outdated<'run>( + fn get_updated_hash_if_outdated<'run>( &self, context: &RecipeContext<'src, 'run>, mut evaluator: Evaluator<'src, 'run>, - ) -> RunResult<'src, Option<(PathBuf, JustfileCache)>> { - let cache_file = &context.search.cache_file; - let mut cache = if !cache_file.exists() { - JustfileCache::new(context.search) - } else { - let file_contents = fs::read_to_string(&cache_file).or_else(|io_error| { - Err(Error::CacheFileRead { - cache_filename: cache_file.clone(), - io_error, - }) - })?; - // Ignore newer versions, incompatible old versions or corrupted cache files - serde_json::from_str(&file_contents) - .or(Err(())) - .and_then(|serialized: JustfileCacheSerialized| serialized.try_into()) - .unwrap_or_else(|_| JustfileCache::new(context.search)) - }; - + ) -> RunResult<'src, Option> { // TODO: Prevent double work/evaluating twice let mut recipe_hash = blake3::Hasher::new(); for line in &self.body { @@ -176,51 +159,15 @@ impl<'src, D> Recipe<'src, D> { } let recipe_hash = recipe_hash.finalize().to_hex(); - if let Some(previous_run) = cache.recipes.get(self.name()) { + let recipes = &context.cache.recipes; + let updated_hash = recipes.get(self.name()).and_then(|previous_run| { if previous_run.body_hash == recipe_hash.as_str() { - return Ok(None); + None + } else { + Some(recipe_hash.to_string()) } - } - cache.recipes.insert( - self.name().to_string(), - RecipeCache { - body_hash: recipe_hash.to_string(), - }, - ); - Ok(Some((cache_file.clone(), cache))) - } - - fn save_latest_cache( - &self, - cache_filename: PathBuf, - cache: JustfileCache, - ) -> RunResult<'src, ()> { - let cache: JustfileCacheSerialized = cache.into(); - let cache = serde_json::to_string(&cache).or_else(|_| { - Err(Error::Internal { - message: format!("Failed to serialize cache: {cache:?}"), - }) - })?; - - cache_filename - .parent() - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::Unsupported, - format!( - "Cannot create parent directory of {}", - cache_filename.display() - ), - ) - }) - .and_then(|parent| fs::create_dir_all(parent)) - .and_then(|_| fs::write(&cache_filename, cache)) - .or_else(|io_error| { - Err(Error::CacheFileWrite { - cache_filename, - io_error, - }) - }) + }); + Ok(updated_hash) } pub(crate) fn run<'run>( @@ -230,16 +177,17 @@ impl<'src, D> Recipe<'src, D> { scope: Scope<'src, 'run>, search: &'run Search, positional: &[String], - ) -> RunResult<'src, ()> { + ) -> RunResult<'src, Option<(String, String)>> { let config = &context.config; - let updated_cache = if !self.cached() { + let updated_hash = if !self.should_cache() { None } else { config.require_unstable("Cached recipes are currently unstable.")?; let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); - match self.get_updated_cache_if_outdated(context, evaluator)? { + match self.get_updated_hash_if_outdated(context, evaluator)? { + Some(hash) => Some(hash), None => { if config.dry_run || config.verbosity.loquacious() @@ -257,9 +205,8 @@ impl<'src, D> Recipe<'src, D> { color.suffix() ); } - return Ok(()); + return Ok(None); } - Some(cache_info) => Some(cache_info), } }; @@ -281,11 +228,7 @@ impl<'src, D> Recipe<'src, D> { self.run_linewise(context, dotenv, &scope, positional, config, evaluator)?; } - if let Some((cache_filename, cache)) = updated_cache { - self.save_latest_cache(cache_filename, cache) - } else { - Ok(()) - } + Ok(updated_hash.and_then(|hash| Some((self.name.to_string(), hash)))) } fn run_linewise<'run>( diff --git a/src/recipe_context.rs b/src/recipe_context.rs index 0e46f5f85d..7fcddcd691 100644 --- a/src/recipe_context.rs +++ b/src/recipe_context.rs @@ -1,6 +1,7 @@ use super::*; pub(crate) struct RecipeContext<'src: 'run, 'run> { + pub(crate) cache: &'run JustfileCache, pub(crate) config: &'run Config, pub(crate) scope: &'run Scope<'src, 'run>, pub(crate) search: &'run Search, diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index c8659b81aa..1cbd1eb69f 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -198,3 +198,9 @@ fn cached_deps_cannot_depend_on_preceding_uncached_ones() {} #[test] fn failed_runs_should_not_update_cache() {} + +#[test] +fn recipes_should_be_cached_when_deps_run_before() {} + +#[test] +fn recipes_should_be_cached_when_deps_run_after() {} From e0142850b9e29a070cfe1fa0ae90b7239c4c9e11 Mon Sep 17 00:00:00 2001 From: Noah May Date: Thu, 21 Mar 2024 15:00:47 -0500 Subject: [PATCH 24/33] Return instead of mutate --- src/justfile.rs | 38 ++++++++++++-------------------------- src/recipe.rs | 4 ++-- 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/src/justfile.rs b/src/justfile.rs index 788eb7db9d..7b262f27f3 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -284,7 +284,7 @@ impl<'src> Justfile<'src> { cache: &cache, }; - Self::run_recipe( + let new_hashes = Self::run_recipe( &invocation .arguments .iter() @@ -296,8 +296,8 @@ impl<'src> Justfile<'src> { &mut ran, invocation.recipe, search, - &mut recipes_hashes, )?; + recipes_hashes.extend(new_hashes); } for (name, body_hash) in recipes_hashes { @@ -417,10 +417,10 @@ impl<'src> Justfile<'src> { ran: &mut Ran<'src>, recipe: &Recipe<'src>, search: &Search, - recipe_hashes: &mut Vec<(String, String)>, - ) -> RunResult<'src> { + ) -> RunResult<'src, HashMap> { + let mut recipe_hashes = HashMap::new(); if ran.has_run(&recipe.namepath, arguments) { - return Ok(()); + return Ok(recipe_hashes); } if !context.config.yes && !recipe.confirm()? { @@ -451,21 +451,14 @@ impl<'src> Justfile<'src> { .map(|argument| evaluator.evaluate_expression(argument)) .collect::>>()?; - Self::run_recipe( - &arguments, - context, - dotenv, - ran, - recipe, - search, - recipe_hashes, - )?; + let new_hashes = Self::run_recipe(&arguments, context, dotenv, ran, recipe, search)?; + recipe_hashes.extend(new_hashes); } } let updated_hash = recipe.run(context, dotenv, scope.child(), search, &positional)?; - if let Some((name, hash)) = updated_hash { - recipe_hashes.push((name, hash)); + if let Some(body_hash) = updated_hash { + recipe_hashes.insert(recipe.name.to_string(), body_hash); } if !context.config.no_dependencies { @@ -478,20 +471,13 @@ impl<'src> Justfile<'src> { evaluated.push(evaluator.evaluate_expression(argument)?); } - Self::run_recipe( - &evaluated, - context, - dotenv, - &mut ran, - recipe, - search, - recipe_hashes, - )?; + let new_hashes = Self::run_recipe(&evaluated, context, dotenv, &mut ran, recipe, search)?; + recipe_hashes.extend(new_hashes); } } ran.ran(&recipe.namepath, arguments.to_vec()); - Ok(()) + Ok(recipe_hashes) } pub(crate) fn public_recipes(&self, source_order: bool) -> Vec<&Recipe<'src, Dependency>> { diff --git a/src/recipe.rs b/src/recipe.rs index 25e7c0ff58..768a9217e3 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -177,7 +177,7 @@ impl<'src, D> Recipe<'src, D> { scope: Scope<'src, 'run>, search: &'run Search, positional: &[String], - ) -> RunResult<'src, Option<(String, String)>> { + ) -> RunResult<'src, Option> { let config = &context.config; let updated_hash = if !self.should_cache() { @@ -228,7 +228,7 @@ impl<'src, D> Recipe<'src, D> { self.run_linewise(context, dotenv, &scope, positional, config, evaluator)?; } - Ok(updated_hash.and_then(|hash| Some((self.name.to_string(), hash)))) + Ok(updated_hash) } fn run_linewise<'run>( From e9686dfa1ca325f1f36b6ae77044ca3cbce388e2 Mon Sep 17 00:00:00 2001 From: Noah May Date: Sat, 23 Mar 2024 19:30:05 -0500 Subject: [PATCH 25/33] Cached dependencies mostly work --- src/compile_error.rs | 7 ++ src/compile_error_kind.rs | 4 ++ src/justfile.rs | 6 +- src/recipe.rs | 15 ++-- src/recipe_resolver.rs | 19 +++-- src/search.rs | 1 + src/testing.rs | 4 ++ tests/cached_recipes.rs | 146 ++++++++++++++++++++++++++++++++++---- 8 files changed, 175 insertions(+), 27 deletions(-) diff --git a/src/compile_error.rs b/src/compile_error.rs index 1457dff526..e206797b24 100644 --- a/src/compile_error.rs +++ b/src/compile_error.rs @@ -46,6 +46,13 @@ impl Display for CompileError<'_> { recipe_line.ordinal(), ), BacktickShebang => write!(f, "Backticks may not start with `#!`"), + CachedDependsOnUncached { cached, uncached } => { + write!( + f, + "Cached recipes cannot depend on preceding uncached ones, yet `{}` depends on `{}`", + cached, uncached + ) + } CircularRecipeDependency { recipe, ref circle } => { if circle.len() == 2 { write!(f, "Recipe `{recipe}` depends on itself") diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs index fbdbf2a876..13668a7dc5 100644 --- a/src/compile_error_kind.rs +++ b/src/compile_error_kind.rs @@ -11,6 +11,10 @@ pub(crate) enum CompileErrorKind<'src> { recipe_line: usize, }, BacktickShebang, + CachedDependsOnUncached { + cached: &'src str, + uncached: &'src str, + }, CircularRecipeDependency { recipe: &'src str, circle: Vec<&'src str>, diff --git a/src/justfile.rs b/src/justfile.rs index 7b262f27f3..043f1374a9 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -273,7 +273,7 @@ impl<'src> Justfile<'src> { let mut ran = Ran::default(); let mut cache = JustfileCache::new(search)?; - let mut recipes_hashes = vec![]; + let mut recipes_hashes = HashMap::new(); for invocation in invocations { let context = RecipeContext { @@ -457,11 +457,13 @@ impl<'src> Justfile<'src> { } let updated_hash = recipe.run(context, dotenv, scope.child(), search, &positional)?; + let recipe_hash_changed = updated_hash.is_some(); + if let Some(body_hash) = updated_hash { recipe_hashes.insert(recipe.name.to_string(), body_hash); } - if !context.config.no_dependencies { + if !context.config.no_dependencies && (!recipe.should_cache() || recipe_hash_changed) { let mut ran = Ran::default(); for Dependency { recipe, arguments } in recipe.dependencies.iter().skip(recipe.priors) { diff --git a/src/recipe.rs b/src/recipe.rs index 768a9217e3..97b3f1944b 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -143,7 +143,7 @@ impl<'src, D> Recipe<'src, D> { self.attributes.contains(&Attribute::NoQuiet) } - fn should_cache(&self) -> bool { + pub(crate) fn should_cache(&self) -> bool { self.attributes.contains(&Attribute::Cached) } @@ -160,13 +160,12 @@ impl<'src, D> Recipe<'src, D> { let recipe_hash = recipe_hash.finalize().to_hex(); let recipes = &context.cache.recipes; - let updated_hash = recipes.get(self.name()).and_then(|previous_run| { - if previous_run.body_hash == recipe_hash.as_str() { - None - } else { - Some(recipe_hash.to_string()) - } - }); + let updated_hash = recipes.get(self.name()).map_or_else( + || Some(recipe_hash.to_string()), + |previous_run| { + (previous_run.body_hash != recipe_hash.as_str()).then(|| recipe_hash.to_string()) + }, + ); Ok(updated_hash) } diff --git a/src/recipe_resolver.rs b/src/recipe_resolver.rs index 88e49783a4..b1d07a190d 100644 --- a/src/recipe_resolver.rs +++ b/src/recipe_resolver.rs @@ -79,13 +79,15 @@ impl<'src: 'run, 'run> RecipeResolver<'src, 'run> { stack.push(recipe.name()); + let should_cache = recipe.should_cache(); + let mut dependencies: Vec> = Vec::new(); - for dependency in &recipe.dependencies { + for (index, dependency) in recipe.dependencies.iter().enumerate() { let name = dependency.recipe.lexeme(); - if let Some(resolved) = self.resolved_recipes.get(name) { + let resolved = if let Some(resolved) = self.resolved_recipes.get(name) { // dependency already resolved - dependencies.push(Rc::clone(resolved)); + Rc::clone(resolved) } else if stack.contains(&name) { let first = stack[0]; stack.push(first); @@ -101,14 +103,23 @@ impl<'src: 'run, 'run> RecipeResolver<'src, 'run> { ); } else if let Some(unresolved) = self.unresolved_recipes.remove(name) { // resolve unresolved dependency - dependencies.push(self.resolve_recipe(stack, unresolved)?); + self.resolve_recipe(stack, unresolved)? } else { // dependency is unknown return Err(dependency.recipe.error(UnknownDependency { recipe: recipe.name(), unknown: name, })); + }; + + if index < recipe.priors && should_cache && !resolved.should_cache() { + return Err(dependency.recipe.error(CachedDependsOnUncached { + cached: recipe.name(), + uncached: resolved.name(), + })); } + + dependencies.push(resolved); } stack.pop(); diff --git a/src/search.rs b/src/search.rs index 76ed9b1ee2..fcd36c8a12 100644 --- a/src/search.rs +++ b/src/search.rs @@ -33,6 +33,7 @@ impl Search { cache_file, } } + pub(crate) fn find( search_config: &SearchConfig, invocation_directory: &Path, diff --git a/src/testing.rs b/src/testing.rs index 09ddb3e16a..19dd137d45 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -18,10 +18,14 @@ pub(crate) fn config(args: &[&str]) -> Config { pub(crate) fn search(config: &Config) -> Search { let working_directory = config.invocation_directory.clone(); let justfile = working_directory.join("justfile"); + let cache_file = working_directory + .join(".justcache") + .join("current-project.json"); Search { justfile, working_directory, + cache_file, } } diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index 1cbd1eb69f..208dcf1b32 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -55,12 +55,12 @@ fn cached_recipes_are_cached() { let justfile = r#" [cached] echo: - @echo cached + @echo caching... "#; let wrapper = ReuseableTest::new(justfile); let wrapper = wrapper - .map(|test| test.arg("--unstable").stdout("cached\n")) + .map(|test| test.arg("--unstable").stdout("caching...\n")) .run(); let _wrapper = wrapper .map(|test| test.arg("--unstable").stderr(&skipped_message("echo"))) @@ -70,8 +70,8 @@ fn cached_recipes_are_cached() { #[test] fn uncached_recipes_are_uncached() { let justfile = r#" - @echo: - echo uncached + echo: + @echo uncached "#; let wrapper = ReuseableTest::new(justfile); @@ -82,11 +82,9 @@ fn uncached_recipes_are_uncached() { #[test] fn cached_recipes_are_independent() { let justfile = r#" - [cached] echo1: @echo cached1 - [cached] echo2: @echo cached2 @@ -118,7 +116,7 @@ fn cached_recipes_are_independent() { } #[test] -fn arguments_and_variables_are_part_of_cache_hash() { +fn interpolated_values_are_part_of_cache_hash() { let justfile = r#" my-var := "1" [cached] @@ -191,16 +189,138 @@ fn invalid_cache_files_are_ignored() { } #[test] -fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() {} +fn cached_deps_cannot_depend_on_preceding_uncached_ones() { + let justfile = r#" + [cached] + cash-money: uncached + uncached: + "#; -#[test] -fn cached_deps_cannot_depend_on_preceding_uncached_ones() {} + let wrapper = ReuseableTest::new(justfile); + let _wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .stderr(unindent(r#" + error: Cached recipes cannot depend on preceding uncached ones, yet `cash-money` depends on `uncached` + ——▶ justfile:2:13 + │ + 2 │ cash-money: uncached + │ ^^^^^^^^ + "#)) + .status(EXIT_FAILURE) + }) + .run(); +} #[test] -fn failed_runs_should_not_update_cache() {} +fn subsequent_deps_run_only_when_cached_recipe_runs() { + let justfile = r#" + [cached] + cash-money: && uncached + @echo cash money + uncached: + @echo uncached cleanup + "#; + + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .arg("cash-money") + .stdout("cash money\nuncached cleanup\n") + }) + .run(); + let _wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .arg("cash-money") + .stderr(skipped_message("cash-money")) + }) + .run(); +} #[test] -fn recipes_should_be_cached_when_deps_run_before() {} +fn cached_recipes_rerun_when_deps_change_but_not_vice_versa() { + let justfile = r#" + top-var := "default-top" + mid-var := "default-middle" + bot-var := "default-bottom" + + [cached] + top: mid + @echo {{top-var}} + [cached] + mid: bot + @echo {{mid-var}} + [cached] + bot: + @echo {{bot-var}} + "#; + + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| test.arg("--unstable").arg("bot").stdout("default-bottom\n")) + .run(); + let wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .arg("top") + .stderr(skipped_message("bot")) + .stdout("default-middle\ndefault-top\n") + }) + .run(); + + let wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .args(["bot-var=change-bottom", "top"]) + .stdout("change-bottom\ndefault-middle\ndefault-top\n") + }) + .run(); + + let _wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .args(["bot-var=change-bottom", "top-var=change-top", "top"]) + .stderr([skipped_message("bot"), skipped_message("mid")].concat()) + .stdout("change-top\n") + }) + .run(); +} #[test] -fn recipes_should_be_cached_when_deps_run_after() {} +fn failed_runs_should_not_update_cache() { + let justfile = r#" + [cached] + exit EXIT_CODE: + @exit {{EXIT_CODE}} + "#; + + let wrapper = ReuseableTest::new(justfile); + let wrapper = wrapper + .map(|test| test.arg("--unstable").args(["exit", "0"])) + .run(); + let wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .args(["exit", "1"]) + .stderr("error: Recipe `exit` failed on line 3 with exit code 1\n") + .status(EXIT_FAILURE) + }) + .run(); + let _wrapper = wrapper + .map(|test| { + test + .arg("--unstable") + .args(["exit", "0"]) + .stderr(skipped_message("exit")) + }) + .run(); +} From e859bd6ed15e2dd0b6bd2851fd31ea893be0dd7c Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 14:59:27 -0500 Subject: [PATCH 26/33] Only impl Recipe::run() for recipes with resolved dependencies --- src/recipe.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/recipe.rs b/src/recipe.rs index 97b3f1944b..b2891e4998 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -146,7 +146,9 @@ impl<'src, D> Recipe<'src, D> { pub(crate) fn should_cache(&self) -> bool { self.attributes.contains(&Attribute::Cached) } +} +impl<'src> Recipe<'src, Dependency<'src>> { fn get_updated_hash_if_outdated<'run>( &self, context: &RecipeContext<'src, 'run>, From fbc87ddaebc10c9be85bef9ab92c60a774d1b403 Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 15:43:49 -0500 Subject: [PATCH 27/33] Finalize cached recipes with deps --- src/cache.rs | 10 +++++++++- src/justfile.rs | 32 +++++++++++++------------------- src/recipe.rs | 32 ++++++++++++++++++++++++++------ src/recipe_context.rs | 4 +++- 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 1ebf057bf2..25dfcf358c 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -30,6 +30,8 @@ pub(crate) struct JustfileCache { #[derive(Debug, Serialize, Deserialize)] pub(crate) struct RecipeCache { pub(crate) body_hash: String, + #[serde(skip)] + pub(crate) hash_changed: bool, } impl JustfileCache { @@ -62,7 +64,13 @@ impl JustfileCache { } pub(crate) fn insert_recipe(&mut self, name: String, body_hash: String) { - self.recipes.insert(name, RecipeCache { body_hash }); + self.recipes.insert( + name, + RecipeCache { + body_hash, + hash_changed: true, + }, + ); } pub(crate) fn save<'run>(self, search: &Search) -> RunResult<'run, ()> { diff --git a/src/justfile.rs b/src/justfile.rs index 043f1374a9..075b4460a2 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -1,4 +1,4 @@ -use {super::*, serde::Serialize}; +use {super::*, serde::Serialize, std::cell::RefCell}; #[derive(Debug)] struct Invocation<'src: 'run, 'run> { @@ -272,8 +272,7 @@ impl<'src> Justfile<'src> { } let mut ran = Ran::default(); - let mut cache = JustfileCache::new(search)?; - let mut recipes_hashes = HashMap::new(); + let cache = RefCell::new(JustfileCache::new(search)?); for invocation in invocations { let context = RecipeContext { @@ -284,7 +283,7 @@ impl<'src> Justfile<'src> { cache: &cache, }; - let new_hashes = Self::run_recipe( + Self::run_recipe( &invocation .arguments .iter() @@ -297,14 +296,9 @@ impl<'src> Justfile<'src> { invocation.recipe, search, )?; - recipes_hashes.extend(new_hashes); } - for (name, body_hash) in recipes_hashes { - cache.insert_recipe(name, body_hash); - } - - cache.save(search) + cache.into_inner().save(search) } pub(crate) fn get_alias(&self, name: &str) -> Option<&Alias<'src>> { @@ -417,10 +411,9 @@ impl<'src> Justfile<'src> { ran: &mut Ran<'src>, recipe: &Recipe<'src>, search: &Search, - ) -> RunResult<'src, HashMap> { - let mut recipe_hashes = HashMap::new(); + ) -> RunResult<'src, ()> { if ran.has_run(&recipe.namepath, arguments) { - return Ok(recipe_hashes); + return Ok(()); } if !context.config.yes && !recipe.confirm()? { @@ -451,8 +444,7 @@ impl<'src> Justfile<'src> { .map(|argument| evaluator.evaluate_expression(argument)) .collect::>>()?; - let new_hashes = Self::run_recipe(&arguments, context, dotenv, ran, recipe, search)?; - recipe_hashes.extend(new_hashes); + Self::run_recipe(&arguments, context, dotenv, ran, recipe, search)?; } } @@ -460,7 +452,10 @@ impl<'src> Justfile<'src> { let recipe_hash_changed = updated_hash.is_some(); if let Some(body_hash) = updated_hash { - recipe_hashes.insert(recipe.name.to_string(), body_hash); + context + .cache + .borrow_mut() + .insert_recipe(recipe.name.to_string(), body_hash); } if !context.config.no_dependencies && (!recipe.should_cache() || recipe_hash_changed) { @@ -473,13 +468,12 @@ impl<'src> Justfile<'src> { evaluated.push(evaluator.evaluate_expression(argument)?); } - let new_hashes = Self::run_recipe(&evaluated, context, dotenv, &mut ran, recipe, search)?; - recipe_hashes.extend(new_hashes); + Self::run_recipe(&evaluated, context, dotenv, &mut ran, recipe, search)?; } } ran.ran(&recipe.namepath, arguments.to_vec()); - Ok(recipe_hashes) + Ok(()) } pub(crate) fn public_recipes(&self, source_order: bool) -> Vec<&Recipe<'src, Dependency>> { diff --git a/src/recipe.rs b/src/recipe.rs index b2891e4998..508424cb81 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -160,15 +160,35 @@ impl<'src> Recipe<'src, Dependency<'src>> { recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); } let recipe_hash = recipe_hash.finalize().to_hex(); + let recipes = &context.cache.borrow().recipes; - let recipes = &context.cache.recipes; - let updated_hash = recipes.get(self.name()).map_or_else( - || Some(recipe_hash.to_string()), + recipes.get(self.name()).map_or_else( + || Ok(Some(recipe_hash.to_string())), |previous_run| { - (previous_run.body_hash != recipe_hash.as_str()).then(|| recipe_hash.to_string()) + let have_deps_changed = self + .dependencies + .iter() + .take(self.priors) + .map(|dep| { + recipes + .get(dep.recipe.name()) + .and_then(|previous_run| Some(previous_run.hash_changed)) + .ok_or_else(|| { + Error::internal(format!( + "prior dependency `{}` did not run before current recipe `{}`", + dep.recipe.name, self.name + )) + }) + }) + .collect::, Error>>()?; + + if have_deps_changed.iter().any(|x| *x) || previous_run.body_hash != recipe_hash.as_str() { + Ok(Some(recipe_hash.to_string())) + } else { + Ok(None) + } }, - ); - Ok(updated_hash) + ) } pub(crate) fn run<'run>( diff --git a/src/recipe_context.rs b/src/recipe_context.rs index 7fcddcd691..07185e273f 100644 --- a/src/recipe_context.rs +++ b/src/recipe_context.rs @@ -1,7 +1,9 @@ +use std::cell::RefCell; + use super::*; pub(crate) struct RecipeContext<'src: 'run, 'run> { - pub(crate) cache: &'run JustfileCache, + pub(crate) cache: &'run RefCell, pub(crate) config: &'run Config, pub(crate) scope: &'run Scope<'src, 'run>, pub(crate) search: &'run Search, From 49617303e424821e94afdc3b76d936f7ea8e17bb Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 15:47:12 -0500 Subject: [PATCH 28/33] Rename src/{cache => justfile_cache}.rs --- src/{cache.rs => justfile_cache.rs} | 0 src/lib.rs | 42 ++++++++++++++--------------- 2 files changed, 21 insertions(+), 21 deletions(-) rename src/{cache.rs => justfile_cache.rs} (100%) diff --git a/src/cache.rs b/src/justfile_cache.rs similarity index 100% rename from src/cache.rs rename to src/justfile_cache.rs diff --git a/src/lib.rs b/src/lib.rs index d67f857e58..3a93ae8439 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,26 +17,26 @@ pub(crate) use { crate::{ alias::Alias, analyzer::Analyzer, assignment::Assignment, assignment_resolver::AssignmentResolver, ast::Ast, attribute::Attribute, binding::Binding, - cache::JustfileCache, color::Color, color_display::ColorDisplay, command_ext::CommandExt, - compilation::Compilation, compile_error::CompileError, compile_error_kind::CompileErrorKind, - compiler::Compiler, conditional_operator::ConditionalOperator, config::Config, - config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, - dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, - expression::Expression, fragment::Fragment, function::Function, - function_context::FunctionContext, interrupt_guard::InterruptGuard, - interrupt_handler::InterruptHandler, item::Item, justfile::Justfile, keyed::Keyed, - keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, - loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, output::output, - output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, - platform::Platform, platform_interface::PlatformInterface, position::Position, - positional::Positional, ran::Ran, range_ext::RangeExt, recipe::Recipe, - recipe_context::RecipeContext, recipe_resolver::RecipeResolver, scope::Scope, search::Search, - search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, - settings::Settings, shebang::Shebang, shell::Shell, show_whitespace::ShowWhitespace, - source::Source, string_kind::StringKind, string_literal::StringLiteral, subcommand::Subcommand, - suggestion::Suggestion, table::Table, thunk::Thunk, token::Token, token_kind::TokenKind, - unresolved_dependency::UnresolvedDependency, unresolved_recipe::UnresolvedRecipe, - use_color::UseColor, variables::Variables, verbosity::Verbosity, warning::Warning, + color::Color, color_display::ColorDisplay, command_ext::CommandExt, compilation::Compilation, + compile_error::CompileError, compile_error_kind::CompileErrorKind, compiler::Compiler, + conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, + count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, + enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, + fragment::Fragment, function::Function, function_context::FunctionContext, + interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, + justfile::Justfile, justfile_cache::JustfileCache, keyed::Keyed, keyword::Keyword, + lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, loader::Loader, name::Name, + namepath::Namepath, ordinal::Ordinal, output::output, output_error::OutputError, + parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, platform::Platform, + platform_interface::PlatformInterface, position::Position, positional::Positional, ran::Ran, + range_ext::RangeExt, recipe::Recipe, recipe_context::RecipeContext, + recipe_resolver::RecipeResolver, scope::Scope, search::Search, search_config::SearchConfig, + search_error::SearchError, set::Set, setting::Setting, settings::Settings, shebang::Shebang, + shell::Shell, show_whitespace::ShowWhitespace, source::Source, string_kind::StringKind, + string_literal::StringLiteral, subcommand::Subcommand, suggestion::Suggestion, table::Table, + thunk::Thunk, token::Token, token_kind::TokenKind, unresolved_dependency::UnresolvedDependency, + unresolved_recipe::UnresolvedRecipe, use_color::UseColor, variables::Variables, + verbosity::Verbosity, warning::Warning, }, std::{ cmp, @@ -117,7 +117,6 @@ mod assignment_resolver; mod ast; mod attribute; mod binding; -mod cache; mod color; mod color_display; mod command_ext; @@ -144,6 +143,7 @@ mod interrupt_guard; mod interrupt_handler; mod item; mod justfile; +mod justfile_cache; mod keyed; mod keyword; mod lexer; From a17b8dd0b543da620cb7bbe9f1ed388432d9eb60 Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 16:10:01 -0500 Subject: [PATCH 29/33] Simplify JustfileCache --- src/justfile.rs | 2 +- src/justfile_cache.rs | 39 +++++---------------------------------- 2 files changed, 6 insertions(+), 35 deletions(-) diff --git a/src/justfile.rs b/src/justfile.rs index 075b4460a2..5afd2b78d1 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -298,7 +298,7 @@ impl<'src> Justfile<'src> { )?; } - cache.into_inner().save(search) + return cache.borrow().save(search); } pub(crate) fn get_alias(&self, name: &str) -> Option<&Alias<'src>> { diff --git a/src/justfile_cache.rs b/src/justfile_cache.rs index 25dfcf358c..43152b6f3f 100644 --- a/src/justfile_cache.rs +++ b/src/justfile_cache.rs @@ -4,19 +4,6 @@ use std::path::PathBuf; use super::*; -/// The version of the justfile as it is on disk. Newer cache formats are added -/// as new variants. -#[derive(Debug, Serialize, Deserialize)] -#[serde(tag = "version")] -pub(crate) enum JustfileCacheSerialized { - #[serde(rename = "unstable-1")] - Unstable1(JustfileCacheUnstable1), -} - -pub(crate) type JustfileCacheUnstable1 = JustfileCache; - -/// The runtime cache format. It should be the intersection of all supported -/// serialized versions, i.e. you can convert any supported version to this. #[derive(Debug, Serialize, Deserialize)] pub(crate) struct JustfileCache { /// Only serialized for user debugging @@ -29,8 +16,10 @@ pub(crate) struct JustfileCache { #[derive(Debug, Serialize, Deserialize)] pub(crate) struct RecipeCache { + /// The hash of the recipe body after evaluation pub(crate) body_hash: String, #[serde(skip)] + /// Has the hash changed this run. Needed for nested cached dependencies. pub(crate) hash_changed: bool, } @@ -57,7 +46,6 @@ impl JustfileCache { // Ignore newer versions, incompatible old versions or corrupted cache files serde_json::from_str(&file_contents) .or(Err(())) - .and_then(|serialized: JustfileCacheSerialized| serialized.try_into()) .unwrap_or_else(|_| Self::new_empty(search)) }; Ok(this) @@ -73,11 +61,10 @@ impl JustfileCache { ); } - pub(crate) fn save<'run>(self, search: &Search) -> RunResult<'run, ()> { - let cache: JustfileCacheSerialized = self.into(); - let cache = serde_json::to_string(&cache).or_else(|_| { + pub(crate) fn save<'run>(&self, search: &Search) -> RunResult<'run, ()> { + let cache = serde_json::to_string(self).or_else(|_| { Err(Error::Internal { - message: format!("Failed to serialize cache: {cache:?}"), + message: format!("Failed to serialize cache: {self:?}"), }) })?; @@ -103,19 +90,3 @@ impl JustfileCache { }) } } - -impl From for JustfileCacheSerialized { - fn from(value: JustfileCache) -> Self { - JustfileCacheSerialized::Unstable1(value) - } -} - -impl TryFrom for JustfileCache { - type Error = (); - - fn try_from(value: JustfileCacheSerialized) -> Result { - match value { - JustfileCacheSerialized::Unstable1(unstable1) => Ok(unstable1), - } - } -} From d12dbc3912ca3b79624b9bb9f04219f4142dd849 Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 16:30:42 -0500 Subject: [PATCH 30/33] Docs --- README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/README.md b/README.md index cb87bf0a8b..8ff0926afe 100644 --- a/README.md +++ b/README.md @@ -1503,6 +1503,7 @@ Recipes may be annotated with attributes that change their behavior. | Name | Description | |------|-------------| | `[confirm]`1.17.0 | Require confirmation prior to executing recipe. | +| `[cached]`TODO! | See [Cached Recipes](#cached-recipes) | | `[confirm("prompt")]`1.23.0 | Require confirmation prior to executing recipe with a custom prompt. | | `[linux]`1.8.0 | Enable recipe on Linux. | | `[macos]`1.8.0 | Enable recipe on MacOS. | @@ -2487,6 +2488,25 @@ polyglot: python js perl sh ruby Run `just --help` to see all the options. +### Cached Recipes + +Cached recipes only run when the recipe body changes, where the body is compared +*after `{{interpolations}}` are evaluated*. This gives you fine control for when +a recipe should rerun. **Note: This is currently an unstable feature and +requires `--unstable`**. + +```just +[cached] +build: + @# This only runs when the hash of the file changes {{sha256_file("input.c")}} + gcc input.c -o output + +[cached] +bad-example: + @# This will never rerun since the body never changes + gcc input.c -o output +``` + ### Private Recipes Recipes and aliases whose name starts with a `_` are omitted from `just --list`: From 1ca512263bea46b0ae94d62e8a36dbba1ed573ca Mon Sep 17 00:00:00 2001 From: Noah May Date: Tue, 26 Mar 2024 16:35:01 -0500 Subject: [PATCH 31/33] note to add to gitignore --- README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8ff0926afe..31c55a66d7 100644 --- a/README.md +++ b/README.md @@ -2492,8 +2492,9 @@ Run `just --help` to see all the options. Cached recipes only run when the recipe body changes, where the body is compared *after `{{interpolations}}` are evaluated*. This gives you fine control for when -a recipe should rerun. **Note: This is currently an unstable feature and -requires `--unstable`**. +a recipe should rerun. It is recommended you add `.justcache/` to your +`.gitignore`. **Note: This is currently an unstable feature and requires +`--unstable`**. ```just [cached] From db89a9a8e2a9fa8ddcae04ff4ef44b77c4277d37 Mon Sep 17 00:00:00 2001 From: Noah May Date: Mon, 15 Apr 2024 15:45:47 -0500 Subject: [PATCH 32/33] Just make the cache mutable directly, don't use refcell --- src/justfile.rs | 15 +++++++-------- src/recipe.rs | 2 +- src/recipe_context.rs | 4 +--- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/justfile.rs b/src/justfile.rs index 5afd2b78d1..c63f9b39be 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -1,4 +1,4 @@ -use {super::*, serde::Serialize, std::cell::RefCell}; +use {super::*, serde::Serialize}; #[derive(Debug)] struct Invocation<'src: 'run, 'run> { @@ -272,15 +272,15 @@ impl<'src> Justfile<'src> { } let mut ran = Ran::default(); - let cache = RefCell::new(JustfileCache::new(search)?); + let mut cache = JustfileCache::new(search)?; for invocation in invocations { - let context = RecipeContext { + let mut context = RecipeContext { settings: invocation.settings, config, scope: invocation.scope, search, - cache: &cache, + cache: &mut cache, }; Self::run_recipe( @@ -290,7 +290,7 @@ impl<'src> Justfile<'src> { .copied() .map(str::to_string) .collect::>(), - &context, + &mut context, &dotenv, &mut ran, invocation.recipe, @@ -298,7 +298,7 @@ impl<'src> Justfile<'src> { )?; } - return cache.borrow().save(search); + return cache.save(search); } pub(crate) fn get_alias(&self, name: &str) -> Option<&Alias<'src>> { @@ -406,7 +406,7 @@ impl<'src> Justfile<'src> { fn run_recipe( arguments: &[String], - context: &RecipeContext<'src, '_>, + context: &mut RecipeContext<'src, '_>, dotenv: &BTreeMap, ran: &mut Ran<'src>, recipe: &Recipe<'src>, @@ -454,7 +454,6 @@ impl<'src> Justfile<'src> { if let Some(body_hash) = updated_hash { context .cache - .borrow_mut() .insert_recipe(recipe.name.to_string(), body_hash); } diff --git a/src/recipe.rs b/src/recipe.rs index 508424cb81..2e1b2cac93 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -160,7 +160,7 @@ impl<'src> Recipe<'src, Dependency<'src>> { recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); } let recipe_hash = recipe_hash.finalize().to_hex(); - let recipes = &context.cache.borrow().recipes; + let recipes = &context.cache.recipes; recipes.get(self.name()).map_or_else( || Ok(Some(recipe_hash.to_string())), diff --git a/src/recipe_context.rs b/src/recipe_context.rs index 07185e273f..72da594072 100644 --- a/src/recipe_context.rs +++ b/src/recipe_context.rs @@ -1,9 +1,7 @@ -use std::cell::RefCell; - use super::*; pub(crate) struct RecipeContext<'src: 'run, 'run> { - pub(crate) cache: &'run RefCell, + pub(crate) cache: &'run mut JustfileCache, pub(crate) config: &'run Config, pub(crate) scope: &'run Scope<'src, 'run>, pub(crate) search: &'run Search, From aa8c2dbe9fbe8b430f8cbee15b0ab508948c0936 Mon Sep 17 00:00:00 2001 From: Noah May Date: Mon, 15 Apr 2024 16:04:41 -0500 Subject: [PATCH 33/33] Run just check --- README.md | 2 +- src/compile_error.rs | 3 +-- src/justfile_cache.rs | 33 ++++++++++++--------------- src/recipe.rs | 49 ++++++++++++++++++++--------------------- tests/cached_recipes.rs | 14 ++++++------ 5 files changed, 47 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index 31c55a66d7..c62dcb39b1 100644 --- a/README.md +++ b/README.md @@ -1503,7 +1503,7 @@ Recipes may be annotated with attributes that change their behavior. | Name | Description | |------|-------------| | `[confirm]`1.17.0 | Require confirmation prior to executing recipe. | -| `[cached]`TODO! | See [Cached Recipes](#cached-recipes) | +| `[cached]`Latest | See [Cached Recipes](#cached-recipes) | | `[confirm("prompt")]`1.23.0 | Require confirmation prior to executing recipe with a custom prompt. | | `[linux]`1.8.0 | Enable recipe on Linux. | | `[macos]`1.8.0 | Enable recipe on MacOS. | diff --git a/src/compile_error.rs b/src/compile_error.rs index e206797b24..570b132082 100644 --- a/src/compile_error.rs +++ b/src/compile_error.rs @@ -49,8 +49,7 @@ impl Display for CompileError<'_> { CachedDependsOnUncached { cached, uncached } => { write!( f, - "Cached recipes cannot depend on preceding uncached ones, yet `{}` depends on `{}`", - cached, uncached + "Cached recipes cannot depend on preceding uncached ones, yet `{cached}` depends on `{uncached}`", ) } CircularRecipeDependency { recipe, ref circle } => { diff --git a/src/justfile_cache.rs b/src/justfile_cache.rs index 43152b6f3f..7444d0b01a 100644 --- a/src/justfile_cache.rs +++ b/src/justfile_cache.rs @@ -34,19 +34,18 @@ impl JustfileCache { pub(crate) fn new<'run>(search: &Search) -> RunResult<'run, Self> { let cache_file = &search.cache_file; - let this = if !cache_file.exists() { - Self::new_empty(search) - } else { - let file_contents = fs::read_to_string(&cache_file).or_else(|io_error| { - Err(Error::CacheFileRead { + let this = if cache_file.exists() { + let file_contents = + fs::read_to_string(cache_file).map_err(|io_error| Error::CacheFileRead { cache_filename: cache_file.clone(), io_error, - }) - })?; + })?; // Ignore newer versions, incompatible old versions or corrupted cache files serde_json::from_str(&file_contents) .or(Err(())) - .unwrap_or_else(|_| Self::new_empty(search)) + .unwrap_or_else(|()| Self::new_empty(search)) + } else { + Self::new_empty(search) }; Ok(this) } @@ -62,10 +61,8 @@ impl JustfileCache { } pub(crate) fn save<'run>(&self, search: &Search) -> RunResult<'run, ()> { - let cache = serde_json::to_string(self).or_else(|_| { - Err(Error::Internal { - message: format!("Failed to serialize cache: {self:?}"), - }) + let cache = serde_json::to_string(self).map_err(|_| Error::Internal { + message: format!("Failed to serialize cache: {self:?}"), })?; search @@ -80,13 +77,11 @@ impl JustfileCache { ), ) }) - .and_then(|parent| fs::create_dir_all(parent)) - .and_then(|_| fs::write(&search.cache_file, cache)) - .or_else(|io_error| { - Err(Error::CacheFileWrite { - cache_filename: search.cache_file.clone(), - io_error, - }) + .and_then(fs::create_dir_all) + .and_then(|()| fs::write(&search.cache_file, cache)) + .map_err(|io_error| Error::CacheFileWrite { + cache_filename: search.cache_file.clone(), + io_error, }) } } diff --git a/src/recipe.rs b/src/recipe.rs index 2e1b2cac93..6f542f162e 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -154,7 +154,6 @@ impl<'src> Recipe<'src, Dependency<'src>> { context: &RecipeContext<'src, 'run>, mut evaluator: Evaluator<'src, 'run>, ) -> RunResult<'src, Option> { - // TODO: Prevent double work/evaluating twice let mut recipe_hash = blake3::Hasher::new(); for line in &self.body { recipe_hash.update(evaluator.evaluate_line(line, false)?.as_bytes()); @@ -172,7 +171,7 @@ impl<'src> Recipe<'src, Dependency<'src>> { .map(|dep| { recipes .get(dep.recipe.name()) - .and_then(|previous_run| Some(previous_run.hash_changed)) + .map(|previous_run| previous_run.hash_changed) .ok_or_else(|| { Error::internal(format!( "prior dependency `{}` did not run before current recipe `{}`", @@ -201,34 +200,34 @@ impl<'src> Recipe<'src, Dependency<'src>> { ) -> RunResult<'src, Option> { let config = &context.config; - let updated_hash = if !self.should_cache() { - None - } else { + let updated_hash = if self.should_cache() { config.require_unstable("Cached recipes are currently unstable.")?; let evaluator = Evaluator::recipe_evaluator(config, dotenv, &scope, context.settings, search); - match self.get_updated_hash_if_outdated(context, evaluator)? { - Some(hash) => Some(hash), - None => { - if config.dry_run - || config.verbosity.loquacious() - || !((context.settings.quiet && !self.no_quiet()) || config.verbosity.quiet()) - { - let color = if config.highlight { - config.color.command(config.command_color) - } else { - config.color - }; - eprintln!( - "{}===> Hash of recipe body of `{}` matches last run. Skipping...{}", - color.prefix(), - self.name, - color.suffix() - ); - } - return Ok(None); + + let hash = self.get_updated_hash_if_outdated(context, evaluator)?; + if hash.is_none() { + if config.dry_run + || config.verbosity.loquacious() + || !((context.settings.quiet && !self.no_quiet()) || config.verbosity.quiet()) + { + let color = if config.highlight { + config.color.command(config.command_color) + } else { + config.color + }; + eprintln!( + "{}===> Hash of recipe body of `{}` matches last run. Skipping...{}", + color.prefix(), + self.name, + color.suffix() + ); } + return Ok(None); } + hash + } else { + None }; if config.verbosity.loquacious() { diff --git a/tests/cached_recipes.rs b/tests/cached_recipes.rs index 208dcf1b32..3c1d72aaf8 100644 --- a/tests/cached_recipes.rs +++ b/tests/cached_recipes.rs @@ -28,7 +28,7 @@ impl ReuseableTest { } } -fn skipped_message<'run>(recipe_name: &str) -> String { +fn skipped_message(recipe_name: &str) -> String { format!( "===> Hash of recipe body of `{}` matches last run. Skipping...\n", recipe_name @@ -63,7 +63,7 @@ fn cached_recipes_are_cached() { .map(|test| test.arg("--unstable").stdout("caching...\n")) .run(); let _wrapper = wrapper - .map(|test| test.arg("--unstable").stderr(&skipped_message("echo"))) + .map(|test| test.arg("--unstable").stderr(skipped_message("echo"))) .run(); } @@ -102,7 +102,7 @@ fn cached_recipes_are_independent() { test .arg("--unstable") .arg("echo1") - .stderr(&skipped_message("echo1")) + .stderr(skipped_message("echo1")) }) .run(); let _wrapper = wrapper @@ -110,7 +110,7 @@ fn cached_recipes_are_independent() { test .arg("--unstable") .arg("echo2") - .stderr(&skipped_message("echo2")) + .stderr(skipped_message("echo2")) }) .run(); } @@ -133,7 +133,7 @@ fn interpolated_values_are_part_of_cache_hash() { test .arg("--unstable") .args(["echo", "a"]) - .stderr(&skipped_message("echo")) + .stderr(skipped_message("echo")) }) .run(); let wrapper = wrapper @@ -144,7 +144,7 @@ fn interpolated_values_are_part_of_cache_hash() { test .arg("--unstable") .args(["echo", "b"]) - .stderr(&skipped_message("echo")) + .stderr(skipped_message("echo")) }) .run(); let wrapper = wrapper @@ -160,7 +160,7 @@ fn interpolated_values_are_part_of_cache_hash() { test .arg("--unstable") .args(["my-var=2", "echo", "b"]) - .stderr(&skipped_message("echo")) + .stderr(skipped_message("echo")) }) .run(); }