Skip to content

Commit

Permalink
fix: Fix infer inputs including directories. (#1789)
Browse files Browse the repository at this point in the history 
* Rework expanding.

* Update changelog.
  • Loading branch information
@milesj
milesj authored Jan 12, 2025
1 parent 596664b commit 3d42f59
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 107 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@
- Added file locks for certain operations to avoid race collisions when multiple `moon` commands are
ran in parallel.

#### 🐞 Fixes

- Fixed an issue where inferred inputs would include directories that would log a warning and fail
to be hashed.

#### ⚙️ Internal

- Updated Rust to v1.84.
Expand Down
64 changes: 46 additions & 18 deletions crates/task-expander/src/task_expander.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,23 @@ impl<'graph> TaskExpander<'graph> {
);

// Resolve in this order!
self.expand_env(&mut task)?;
self.expand_deps(&mut task)?;
self.expand_inputs(&mut task)?;
self.expand_outputs(&mut task)?;
self.expand_args(&mut task)?;

if task.script.is_some() {
self.expand_script(&mut task)?;
} else {
self.expand_command(&mut task)?;
}

self.expand_env(&mut task)?;
self.expand_deps(&mut task)?;
self.expand_inputs(&mut task)?;
self.expand_outputs(&mut task)?;
self.expand_args(&mut task)?;
task.state.expanded = true;

// Run post-expand operations
self.move_input_dirs_to_globs(&mut task);
self.remove_input_output_overlaps(&mut task);

Ok(task)
}

Expand Down Expand Up @@ -228,22 +231,47 @@ impl<'graph> TaskExpander<'graph> {
// Expand outputs to file system paths
let result = self.token.expand_outputs(task)?;

// Aggregate paths first before globbing, as they are literal
for file in result.files {
// Outputs must *not* be considered an input,
// so if there's an input that matches an output,
// remove it! Is there a better way to do this?
task.output_files.extend(result.files);
task.output_globs.extend(result.globs);

Ok(())
}

// Input directories are not allowed, as VCS hashing only operates on files.
// If we can confirm it's a directory, move it into a glob!
pub fn move_input_dirs_to_globs(&mut self, task: &mut Task) {
let mut to_remove = vec![];

for file in &task.input_files {
// If this dir is actually an output dir, just remove it
if task.output_files.contains(file) {
to_remove.push(file.to_owned());
continue;
}

// Otherwise check if it's a dir and not a file
let abs_file = file.to_path(&self.context.workspace_root);

if abs_file.exists() && abs_file.is_dir() {
task.input_globs.insert(file.join("**/*"));
to_remove.push(file.to_owned());
}
}

for file in to_remove {
task.input_files.remove(&file);
task.output_files.insert(file);
}
}

// Aggregate globs second so we can match against the paths
for glob in result.globs {
// Same treatment here!
task.input_globs.remove(&glob);
task.output_globs.insert(glob);
// Outputs must not be considered an input, otherwise the content
// hash will constantly change, and the cache will always be busted
pub fn remove_input_output_overlaps(&mut self, task: &mut Task) {
for file in &task.output_files {
task.input_files.remove(file);
}

Ok(())
for glob in &task.output_globs {
task.input_globs.remove(glob);
}
}
}
33 changes: 12 additions & 21 deletions crates/task-expander/src/token_expander.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,9 @@ impl<'graph> TokenExpander<'graph> {
while self.has_token_function(&script) {
let result = self.replace_function(task, &script)?;

if let Some(token) = result.token {
if task.options.infer_inputs {
task.input_files.extend(result.files.clone());
task.input_globs.extend(result.globs.clone());
}
self.infer_inputs(task, &result);

if let Some(token) = result.token {
let mut items = vec![];

for file in result.files {
Expand Down Expand Up @@ -168,10 +165,7 @@ impl<'graph> TokenExpander<'graph> {
if self.has_token_function(&arg) {
let result = self.replace_function(task, &arg)?;

if task.options.infer_inputs {
task.input_files.extend(result.files.clone());
task.input_globs.extend(result.globs.clone());
}
self.infer_inputs(task, &result);

for file in result.files {
args.push(self.resolve_path_for_task(task, file)?);
Expand Down Expand Up @@ -214,10 +208,7 @@ impl<'graph> TokenExpander<'graph> {
let result = self.replace_function(task, &value)?;
let mut items = vec![];

if task.options.infer_inputs {
task.input_files.extend(result.files.clone());
task.input_globs.extend(result.globs.clone());
}
self.infer_inputs(task, &result);

for file in result.files {
items.push(self.resolve_path_for_task(task, file)?);
Expand Down Expand Up @@ -287,15 +278,8 @@ impl<'graph> TokenExpander<'graph> {
task,
input.to_workspace_relative(&self.project.source),
)?;
let abs_file = file.to_path(&self.context.workspace_root);

// This is a special case that converts "foo" to "foo/**/*",
// when the input is a directory. This is necessary for VCS hashing.
if abs_file.exists() && abs_file.is_dir() {
result.globs.push(file.join("**/*"));
} else {
result.files.push(file);
}
result.files.push(file);
}
InputPath::ProjectGlob(_) | InputPath::WorkspaceGlob(_) => {
let glob = self.create_path_for_task(
Expand Down Expand Up @@ -727,4 +711,11 @@ impl<'graph> TokenExpander<'graph> {
path::to_virtual_string(diff_paths(&abs_path, &self.project.root).unwrap_or(abs_path))
}
}

fn infer_inputs(&self, task: &mut Task, result: &ExpandedResult) {
if task.options.infer_inputs {
task.input_files.extend(result.files.clone());
task.input_globs.extend(result.globs.clone());
}
}
}
110 changes: 67 additions & 43 deletions crates/task-expander/tests/task_expander_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,73 @@ fn create_path_set(inputs: Vec<&str>) -> FxHashSet<WorkspaceRelativePathBuf> {
mod task_expander {
use super::*;

#[test]
fn doesnt_overlap_input_file() {
let sandbox = create_sandbox("file-group");
let project = create_project(sandbox.path());

let mut task = create_task();
task.outputs.push(OutputPath::ProjectFile("out".into()));
task.input_files.insert("project/source/out".into());

let context = create_context(sandbox.path());
let task = TaskExpander::new(&project, &context)
.expand(&mut task)
.unwrap();

assert!(task.input_files.is_empty());
assert_eq!(
task.output_files,
create_path_set(vec!["project/source/out"])
);
}

#[test]
fn doesnt_overlap_input_glob() {
let sandbox = create_sandbox("file-group");
let project = create_project(sandbox.path());

let mut task = create_task();
task.outputs
.push(OutputPath::ProjectGlob("out/**/*".into()));
task.input_globs.insert("project/source/out/**/*".into());

let context = create_context(sandbox.path());
let task = TaskExpander::new(&project, &context)
.expand(&mut task)
.unwrap();

assert!(task.input_globs.is_empty());
assert_eq!(
task.output_globs,
create_path_set(vec!["project/source/out/**/*"])
);
}

#[test]
fn converts_dirs_to_globs() {
let sandbox = create_sandbox("file-group");

// Dir has to exist!
sandbox.create_file("project/source/dir/file", "");

let project = create_project(sandbox.path());

let mut task = create_task();
task.inputs = vec![InputPath::ProjectFile("dir".into())];

let context = create_context(sandbox.path());
let task = TaskExpander::new(&project, &context)
.expand(&mut task)
.unwrap();

assert!(task.input_files.is_empty());
assert_eq!(
task.input_globs,
create_path_set(vec!["project/source/dir/**/*"])
);
}

mod expand_command {
use super::*;

Expand Down Expand Up @@ -827,48 +894,5 @@ mod task_expander {
);
assert_eq!(task.output_files, create_path_set(vec!["project/index.js"]));
}

#[test]
fn doesnt_overlap_input_file() {
let sandbox = create_sandbox("file-group");
let project = create_project(sandbox.path());

let mut task = create_task();
task.outputs.push(OutputPath::ProjectFile("out".into()));
task.input_files.insert("project/source/out".into());

let context = create_context(sandbox.path());
TaskExpander::new(&project, &context)
.expand_outputs(&mut task)
.unwrap();

assert!(task.input_files.is_empty());
assert_eq!(
task.output_files,
create_path_set(vec!["project/source/out"])
);
}

#[test]
fn doesnt_overlap_input_glob() {
let sandbox = create_sandbox("file-group");
let project = create_project(sandbox.path());

let mut task = create_task();
task.outputs
.push(OutputPath::ProjectGlob("out/**/*".into()));
task.input_globs.insert("project/source/out/**/*".into());

let context = create_context(sandbox.path());
TaskExpander::new(&project, &context)
.expand_outputs(&mut task)
.unwrap();

assert!(task.input_globs.is_empty());
assert_eq!(
task.output_globs,
create_path_set(vec!["project/source/out/**/*"])
);
}
}
}
25 changes: 0 additions & 25 deletions crates/task-expander/tests/token_expander_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1310,31 +1310,6 @@ mod token_expander {
}
);
}

#[test]
fn converts_dirs_to_globs() {
let sandbox = create_empty_sandbox();

// Dir has to exist!
sandbox.create_file("project/source/dir/file", "");

let project = create_project(sandbox.path());
let mut task = create_task();

task.inputs = vec![InputPath::ProjectFile("dir".into())];

let context = create_context(sandbox.path());
let mut expander = TokenExpander::new(&project, &context);

assert_eq!(
expander.expand_inputs(&task).unwrap(),
ExpandedResult {
files: vec![],
globs: vec![WorkspaceRelativePathBuf::from("project/source/dir/**/*"),],
..ExpandedResult::default()
}
);
}
}

mod outputs {
Expand Down

0 comments on commit 3d42f59

Please sign in to comment.