Skip to content

Commit

Permalink
fix: task definition serialization (#6306)
Browse files Browse the repository at this point in the history
### Description

A bunch of tiny changes that bring the JSON serialization to be closer
to Go.

We go for a similar approach to Go of having a new structure for
serialization in order to match Go's distinction between `nil` and
`[]string{}`.

### Testing Instructions

More rust codepath integration tests pass. Many failing ones now have
smaller diffs e.g. `dry_json/single_package*.t` only differ by
`hashOfExternalDeps`


Closes TURBO-1548

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski authored Nov 1, 2023
1 parent adf821b commit 1b9fca8
Show file tree
Hide file tree
Showing 11 changed files with 217 additions and 121 deletions.
4 changes: 2 additions & 2 deletions crates/turborepo-env/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ impl EnvironmentVariableMap {
hasher.update(v.as_bytes());
let hash = hasher.finalize();
let hexed_hash = hex::encode(hash);
format!("{}=", hexed_hash)
format!("{k}={hexed_hash}")
} else {
format!("{}=", k)
format!("{k}=")
}
})
.collect()
Expand Down
19 changes: 16 additions & 3 deletions crates/turborepo-lib/src/config/turbo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,7 @@ impl TryFrom<RawTaskDefinition> for BookkeepingTaskDefinition {

Ok(dot_env)
})
.transpose()?
.unwrap_or_default();
.transpose()?;

if raw_task.output_mode.is_some() {
defined_fields.insert("OutputMode".to_string());
Expand Down Expand Up @@ -709,6 +708,20 @@ mod tests {
task_definition: TaskDefinitionStable::default()
}
)]
#[test_case(
r#"{ "dotEnv": [] }"#,
RawTaskDefinition {
dot_env: Some(Vec::new()),
..RawTaskDefinition::default()
},
BookkeepingTaskDefinition {
defined_fields: ["DotEnv".to_string()].into_iter().collect(),
experimental_fields: HashSet::new(),
experimental: TaskDefinitionExperiments::default(),
task_definition: TaskDefinitionStable { dot_env: Some(Vec::new()), ..Default::default() }
}
; "empty dotenv"
)]
#[test_case(
r#"{
"dependsOn": ["cli#build"],
Expand Down Expand Up @@ -747,7 +760,7 @@ mod tests {
experimental_fields: HashSet::new(),
experimental: TaskDefinitionExperiments {},
task_definition: TaskDefinitionStable {
dot_env: vec![RelativeUnixPathBuf::new("package/a/.env").unwrap()],
dot_env: Some(vec![RelativeUnixPathBuf::new("package/a/.env").unwrap()]),
env: vec!["OS".to_string()],
outputs: TaskOutputs {
inclusions: vec!["package/a/dist".to_string()],
Expand Down
12 changes: 6 additions & 6 deletions crates/turborepo-lib/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ impl WorkspaceInfo {
}

pub fn get_external_deps_hash(&self) -> String {
let mut transitive_deps = Vec::with_capacity(
self.transitive_dependencies
.as_ref()
.map_or(0, |deps| deps.len()),
);
let Some(transitive_dependencies) = &self.transitive_dependencies else {
return "".into();
};

let mut transitive_deps = Vec::with_capacity(transitive_dependencies.len());

for dependency in self.transitive_dependencies.iter().flatten() {
for dependency in transitive_dependencies.iter() {
transitive_deps.push(dependency.clone());
}

Expand Down
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/run/summary/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ impl<'a> ExecutionSummary<'a> {
}

fn successful(&self) -> usize {
self.success + self.attempted
self.success + self.cached
}
}

Expand Down
36 changes: 25 additions & 11 deletions crates/turborepo-lib/src/run/summary/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub struct RunSummary<'a> {
global_hash_summary: GlobalHashSummary<'a>,
#[serde(skip_serializing_if = "Option::is_none")]
execution: Option<ExecutionSummary<'a>>,
packages: HashSet<WorkspaceName>,
packages: Vec<WorkspaceName>,
env_mode: EnvMode,
framework_inference: bool,
tasks: Vec<TaskSummary>,
Expand Down Expand Up @@ -236,7 +236,7 @@ impl RunTracker {
id: Ksuid::new(None, None),
version: RUN_SUMMARY_SCHEMA_VERSION.to_string(),
turbo_version: self.version,
packages,
packages: packages.into_iter().sorted().collect(),
execution: Some(execution_summary),
env_mode: run_opts.env_mode.into(),
framework_inference: run_opts.framework_inference,
Expand Down Expand Up @@ -540,7 +540,10 @@ impl<'a> RunSummary<'a> {
ui,
GREY,
" Outputs\t=\t{}",
task.shared.outputs.join(", ")
task.shared
.outputs
.as_ref()
.map_or_else(String::new, |outputs| outputs.join(", "))
)?;
cwriteln!(
tab_writer,
Expand Down Expand Up @@ -575,7 +578,10 @@ impl<'a> RunSummary<'a> {
ui,
GREY,
" .env Files Considered\t=\t{}",
task.shared.dot_env.len()
task.shared
.dot_env
.as_ref()
.map_or(0, |dot_env| dot_env.len())
)?;

cwriteln!(
Expand Down Expand Up @@ -609,14 +615,19 @@ impl<'a> RunSummary<'a> {
.environment_variables
.specified
.pass_through_env
.join(", ")
.as_ref()
.map_or_else(String::new, |pass_through_env| pass_through_env.join(", "))
)?;
cwriteln!(
tab_writer,
ui,
GREY,
" Passed Through Env Vars Values\t=\t{}",
task.shared.environment_variables.pass_through.join(", ")
task.shared
.environment_variables
.pass_through
.as_ref()
.map_or_else(String::new, |vars| vars.join(", "))
)?;

// If there's an error, we can silently ignore it, we don't need to block the
Expand All @@ -640,13 +651,16 @@ impl<'a> RunSummary<'a> {
fn format_json(&mut self) -> Result<String, Error> {
self.normalize();

if self.monorepo {
Ok(serde_json::to_string_pretty(&self)?)
let mut rendered_json = if self.monorepo {
serde_json::to_string_pretty(&self)
} else {
// Deref coercion used to get an immutable reference from the mutable reference.
let monorepo_rsm = SinglePackageRunSummary::from(&*self);
Ok(serde_json::to_string_pretty(&monorepo_rsm)?)
}
serde_json::to_string_pretty(&monorepo_rsm)
}?;
// Go produces an extra newline at the end of the JSON
rendered_json.push('\n');
Ok(rendered_json)
}

fn normalize(&mut self) {
Expand All @@ -658,7 +672,7 @@ impl<'a> RunSummary<'a> {
// For single packages, we don't need the packages
// and each task summary needs some cleaning
if !self.monorepo {
self.packages.drain();
self.packages.clear();
}

self.tasks.sort_by(|a, b| a.task_id.cmp(&b.task_id));
Expand Down
Loading

0 comments on commit 1b9fca8

Please sign in to comment.