From 53103e47e318c94f1408109f510202ba1c97af2c Mon Sep 17 00:00:00 2001 From: zakstucke <44890343+zakstucke@users.noreply.github.com> Date: Tue, 27 Feb 2024 09:26:51 +0200 Subject: [PATCH] Var sc bugfix when used in post and pre exists (#20) --- py_rust/src/config/tasks.rs | 4 +- py_rust/src/config/validate.rs | 8 ---- py_rust/src/state/active_state.rs | 8 ++++ py_rust/src/state/parent_state.rs | 16 ++++++-- py_rust/tests/test_tasks.py | 62 +++++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 14 deletions(-) diff --git a/py_rust/src/config/tasks.rs b/py_rust/src/config/tasks.rs index f53f553..b351522 100644 --- a/py_rust/src/config/tasks.rs +++ b/py_rust/src/config/tasks.rs @@ -14,7 +14,7 @@ use crate::{ }, }; -static IN_TASK_ENV_VAR: &str = "ZETCH_IN_TASK"; +pub static IN_TASK_ENV_VAR: &str = "ZETCH_IN_TASK"; pub fn parent_task_active() -> bool { std::env::var(IN_TASK_ENV_VAR).is_ok() @@ -29,7 +29,7 @@ impl Task { /// Run the task, post tasks will be given the env var to the post ctx path. fn run(&self, config_filepath: &Path, cached_config_loc: Option<&Path>) -> Result<(), Zerr> { // Make sure no recursion: - if std::env::var(IN_TASK_ENV_VAR).is_ok() { + if parent_task_active() { return Err(zerr!( Zerr::TaskRecursionError, "Tasks being run recursively. Make sure you're not running a zetch command that triggers tasks from inside tasks.\nE.g. 'zetch render'.\n\nHint: 'zetch render|var' commands in 'pre' tasks won't work with due to recursive task constraints,\n however, 'zetch var' does work in 'post' tasks thanks to some internal magic." diff --git a/py_rust/src/config/validate.rs b/py_rust/src/config/validate.rs index 8c13a33..97f0a2a 100644 --- a/py_rust/src/config/validate.rs +++ b/py_rust/src/config/validate.rs @@ -137,14 +137,6 @@ fn format_err(err: Box) -> String { } else { err.get_title() }; - info!( - "title: {}, detail {:?}, path: {}, code: {}, source: {:?}", - err.get_title(), - err.get_detail(), - err.get_path(), - err.get_code(), - err.source() - ); let mut loc_parts = err .get_path() diff --git a/py_rust/src/state/active_state.rs b/py_rust/src/state/active_state.rs index 1b042b9..1a11d4a 100644 --- a/py_rust/src/state/active_state.rs +++ b/py_rust/src/state/active_state.rs @@ -4,6 +4,8 @@ use std::{ }; use bitbazaar::timeit; +use parking_lot::Mutex; +use tempfile::NamedTempFile; use super::parent_state::load_parent_state; use crate::{args::Command, config::conf::Config, prelude::*}; @@ -25,6 +27,10 @@ pub struct State { /// True if --superlight pub superlight: bool, + + // Storing the cached state file to prevent being dropped too early: + // Bit of a hack, but mutex easier than making state mutable where this needs to be set: + pub cached_state_file: Mutex>, } impl State { @@ -42,6 +48,7 @@ impl State { final_config_path: parent_shared_state.final_config_path, light: false, superlight: false, + cached_state_file: Mutex::new(None), } } else { let final_config_path = build_final_config_path( @@ -88,6 +95,7 @@ impl State { final_config_path, light, superlight, + cached_state_file: Mutex::new(None), } }; diff --git a/py_rust/src/state/parent_state.rs b/py_rust/src/state/parent_state.rs index dc1ed52..91b80df 100644 --- a/py_rust/src/state/parent_state.rs +++ b/py_rust/src/state/parent_state.rs @@ -8,7 +8,10 @@ use tempfile::NamedTempFile; use super::State; use crate::{ - config::{conf::Config, tasks::parent_task_active}, + config::{ + conf::Config, + tasks::{parent_task_active, IN_TASK_ENV_VAR}, + }, prelude::*, }; @@ -26,15 +29,18 @@ pub struct StoredParentState { /// /// Returns the PathBuf to the temporary file. pub fn store_parent_state(state: &State) -> Result { - let state = StoredParentState { + let stored_state = StoredParentState { conf: state.conf.clone(), ctx: state.ctx.clone(), final_config_path: state.final_config_path.clone(), }; let temp = NamedTempFile::new().change_context(Zerr::InternalError)?; - serde_json::to_writer(&temp, &state).change_context(Zerr::InternalError)?; - Ok(temp.path().to_path_buf()) + serde_json::to_writer(&temp, &stored_state).change_context(Zerr::InternalError)?; + let buf = temp.path().to_path_buf(); + // Keep stored to prevent dropping and getting cleaned up too early: + state.cached_state_file.lock().replace(temp); + Ok(buf) } /// Load the cached state if it's available, return None otherwise. @@ -51,6 +57,8 @@ pub fn load_parent_state() -> Result, Zerr> { return Ok(Some( serde_json::from_str(&contents).change_context(Zerr::InternalError)?, )); + } else { + warn!("Nested zetch task seems to be running, tried loading parent state from {}, but it doesn't exist. You may have orphaned {}/{} environment variables.", path.display(), IN_TASK_ENV_VAR, CACHED_STATE_ENV_VAR); } } Ok(None) diff --git a/py_rust/tests/test_tasks.py b/py_rust/tests/test_tasks.py index a3d0a29..efeec4a 100644 --- a/py_rust/tests/test_tasks.py +++ b/py_rust/tests/test_tasks.py @@ -149,3 +149,65 @@ def test_tasks_invalid( ) with pytest.raises(ValueError, match=err_expected): cli.render(manager.root_dir, conf_file) + + +@pytest.mark.parametrize( + "desc, config, cb", + [ + # Important task! Written to catch bug. + # Make sure var works in post, and still everything works when pre exists too. (pre should work with read/put/del but not var): + ( + "basic_no_cmd", + { + "tasks": { + "pre": [ + { + "commands": [ + # TODO after next bb update, replace the below with the commented out: + "echo foo", + # Random stuff, just not var as that wouldn't work in pre: + # 'echo \'{"ree": "randomo"}\' > file.json', + # "zetch read file.json ree", + # "zetch put file.json value bar", + # "zetch del file.json ree", + ] + } + ], + "post": [ + { + "commands": [ + 'echo \'{"ree": "randomo"}\' > file.json', + "zetch read file.json ree", + "zetch put file.json value bar", + "zetch del file.json ree", + "zetch var FOO", + ] + } + ], + }, + "context": {"static": {"FOO": {"value": "bar"}}}, + }, + lambda man: lambda: check_file( + os.path.join(man.root_dir, "file.json"), '{\n "value": "bar"\n}' + ), + ), + ], +) +def test_tasks_various( + desc: str, + config: InputConfig, + # Setup callable returns either None or a callable that will be called after the test to run any other checks. + cb: "tp.Optional[tp.Callable[[TmpFileManager], tp.Optional[tp.Callable[[], tp.Any]]]]", +): + with TmpFileManager() as manager: + conf_file = manager.tmpfile( + zetch._toml_create(config), + full_name="zetch.config.toml", + ) + + post_check = cb(manager) if cb else None + + cli.render(manager.root_dir, conf_file) + + if post_check: + post_check()