feat(prepare): update mtime of outputs after command is run#8243
feat(prepare): update mtime of outputs after command is run#8243
Conversation
Summary of ChangesHello @halms, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a touch_outputs feature to handle cases where prepare commands are no-ops. The implementation adds the necessary configuration, updates the prepare engine to touch output files after a successful run, and includes corresponding documentation and tests. The changes are well-implemented. I have a couple of suggestions to improve the readability and debuggability of the new code.
There was a problem hiding this comment.
Pull request overview
This PR adds a touch_outputs configuration option (defaulting to true) to prepare providers that updates the mtime of output files/directories after a successful prepare command run. This solves an issue where no-op commands (like uv sync when dependencies are already satisfied) don't modify any files, causing mise to continuously consider outputs stale.
Changes:
- Added
touch_outputsboolean config field toPrepareProviderConfigwith a default value oftrue - Added
touch_outputs()trait method toPrepareProviderwith a default implementation returningtrue - Implemented output mtime updating logic in
PrepareEnginethat touches outputs after successful command execution - Updated all 12 provider implementations to expose the config value through the trait method
- Added comprehensive e2e tests for both
touch_outputs = trueandtouch_outputs = falsescenarios - Updated documentation to explain the new feature and its motivation
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/prepare/rule.rs | Added touch_outputs field to config struct with default_true() helper function |
| src/prepare/mod.rs | Added touch_outputs() trait method with default implementation |
| src/prepare/engine.rs | Implemented touch_outputs() helper that updates mtimes after successful runs; integrated into parallel execution flow |
| src/prepare/providers/yarn.rs | Implemented trait method to expose config value |
| src/prepare/providers/uv.rs | Implemented trait method to expose config value |
| src/prepare/providers/poetry.rs | Implemented trait method to expose config value |
| src/prepare/providers/pnpm.rs | Implemented trait method to expose config value |
| src/prepare/providers/pip.rs | Implemented trait method to expose config value |
| src/prepare/providers/npm.rs | Implemented trait method to expose config value |
| src/prepare/providers/go.rs | Implemented trait method to expose config value |
| src/prepare/providers/custom.rs | Implemented trait method to expose config value |
| src/prepare/providers/composer.rs | Implemented trait method to expose config value |
| src/prepare/providers/bundler.rs | Implemented trait method to expose config value |
| src/prepare/providers/bun.rs | Implemented trait method to expose config value |
| e2e/cli/test_prepare | Added tests verifying touch behavior for both enabled and disabled states |
| docs/dev-tools/prepare.md | Updated documentation table and added explanation of the touch_outputs feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
halms
force-pushed
the
feat/touch-after-prepare
branch
3 times, most recently
from
521c1a7 to
742f420
Compare
halms
force-pushed
the
feat/touch-after-prepare
branch
from
742f420 to
39f1ef0
Compare
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug, Clone, Default, Deserialize, Serialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct PrepareProviderConfig { | ||
| /// Whether to auto-run this provider before mise x/run (default: false) | ||
| #[serde(default)] | ||
| pub auto: bool, | ||
| /// Command to run when stale (required for custom, optional override for built-in) | ||
| pub run: Option<String>, | ||
| /// Files/patterns to check for changes (required for custom, auto-detected for built-in) | ||
| #[serde(default)] | ||
| pub sources: Vec<String>, | ||
| /// Files/directories that should be newer than sources (required for custom, auto-detected for built-in) | ||
| #[serde(default)] | ||
| pub outputs: Vec<String>, | ||
| /// Environment variables to set | ||
| #[serde(default)] | ||
| pub env: BTreeMap<String, String>, | ||
| /// Working directory | ||
| pub dir: Option<String>, | ||
| /// Optional description | ||
| pub description: Option<String>, | ||
| /// Whether to update mtime of output files/dirs after a successful run (default: true) | ||
| /// This is useful when the prepare command is a no-op (e.g., `uv sync` when all is well) | ||
| /// so that the outputs appear fresh for subsequent freshness checks. | ||
| #[serde(default = "default_true")] | ||
| pub touch_outputs: bool, |
There was a problem hiding this comment.
PrepareProviderConfig derives Default, but the new touch_outputs field will default to false via bool’s default, while deserialization/doc text says the default should be true. This makes PrepareProviderConfig::default() inconsistent with config defaults and could lead to surprising behavior in code paths that construct configs programmatically. Consider implementing Default manually (or otherwise ensuring touch_outputs is true in the struct’s Default) to match the documented/configured default.
| let run_results = | ||
| parallel::parallel(to_run_with_context, |(job, mpr, toolset_env)| async move { | ||
| let pr = mpr.add(&job.cmd.description); | ||
| match Self::execute_prepare_static(&job.cmd, &toolset_env) { | ||
| Ok(()) => { | ||
| pr.finish_with_message(format!("{} done", cmd.description)); | ||
| if job.touch { | ||
| Self::touch_outputs(&job.outputs); | ||
| } | ||
| pr.finish_with_message(format!("{} done", job.cmd.description)); | ||
| // Return outputs along with result so we can clear stale status | ||
| // after ALL providers complete successfully | ||
| Ok((PrepareStepResult::Ran(id), outputs)) | ||
| Ok((PrepareStepResult::Ran(job.id), job.outputs)) | ||
| } |
There was a problem hiding this comment.
touch_outputs is applied immediately after an individual provider succeeds inside the parallel execution closure. Since parallel::parallel aborts remaining tasks and returns an error on the first failure, this can leave some outputs “touched” even though the overall mise prepare run failed. Consider deferring touching until after run_results is successfully collected (i.e., after all providers completed successfully), similar to how stale status clearing is handled.
There was a problem hiding this comment.
Code Review
The pull request successfully implements the touch_outputs feature for prepare providers, which ensures that output modification times are updated after a successful run. This is particularly effective for commands like uv sync that might result in a no-op, preventing mise from repeatedly considering the outputs stale. The update to the freshness checking logic to include the directory's own modification time is a key improvement that makes this feature work as intended. The implementation is clean, well-integrated into the parallel execution engine, and includes comprehensive E2E tests.
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
This review is fully AI-generated.
Nice PR — the motivation is solid and the implementation is clean. A few notes:
The newest_file_in_dir change is the most consequential part. Previously the directory's own mtime was only used as a fallback at max_depth == 0. Now it's always included, which is necessary for the touch feature to work but subtly changes freshness semantics for all directory outputs. A directory's mtime changes when entries are added/removed directly in it, so this could theoretically make a provider appear fresh in edge cases — though in practice it's fine since the dir mtime would still need to be newer than sources. The existing e2e test fix (adding touch -t on .venv dir after mkdir) confirms this behavioral change.
Boilerplate across 11 provider files — every provider gets an identical fn touch_outputs(&self) -> bool { self.config.touch_outputs }. Since all providers have a config: PrepareProviderConfig field, this could be a macro or a helper to reduce repetition. Not a blocker.
Otherwise looks good to merge — filetime is already a dep, tests cover both the default-on and opt-out cases, and the docs are updated.
While working on #8243, it looked like there was quite some boilerplate repeated throughout prepare providers. This PR factors q bit out into a common `ProviderBase`. It also removes the default-true-ambiguity raised by a review in #8243, by making this `Option<bool>` and resolving None as true in the `ProviderBase`. This would now also allow having different per-provider defaults for `touch_outputs`.
2 participants
Motivation for this feature in
uvcase:Something is updated in
pyproject.toml(e.g., just some tool config) that doesn't result in any changes to be done to.venv.In that case
uv syncis a no-op and doesn't update any files in.venv.Thus, mise continues to consider the output stale.
This PR adds/changes:
touch_outputsconfig to prepare providers, defaulting to trueoutputsso that no longer considered staleoutputscontains a directory, include the directory's own mtime in the latest check