From 53486089ce3198cd0a92b557502f2fcc24d79a44 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 18 Dec 2023 13:58:29 -0800 Subject: [PATCH 01/11] [naga xtask] Move validation subcommands into their own module. --- naga/xtask/src/main.rs | 257 +------------------------------------ naga/xtask/src/validate.rs | 256 ++++++++++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+), 251 deletions(-) create mode 100644 naga/xtask/src/validate.rs diff --git a/naga/xtask/src/main.rs b/naga/xtask/src/main.rs index d360042aa8..1ab6489128 100644 --- a/naga/xtask/src/main.rs +++ b/naga/xtask/src/main.rs @@ -1,19 +1,13 @@ -use std::{ - io::{BufRead, BufReader}, - path::Path, - process::{ExitCode, Stdio}, -}; +use std::process::ExitCode; -use anyhow::{bail, Context}; +use anyhow::Context; use cli::Args; use crate::{ - cli::{Subcommand, ValidateHlslCommand, ValidateSubcommand}, - fs::{open_file, remove_dir_all}, - glob::visit_files, + cli::Subcommand, + fs::remove_dir_all, path::join_path, process::{which, EasyCommand}, - result::{ErrorStatus, LogIfError}, }; mod cli; @@ -22,6 +16,7 @@ mod glob; mod path; mod process; mod result; +mod validate; fn main() -> ExitCode { env_logger::builder() @@ -42,8 +37,6 @@ fn main() -> ExitCode { } fn run(args: Args) -> anyhow::Result<()> { - let snapshots_base_out = join_path(["tests", "out"]); - let Args { subcommand } = args; assert!(which("cargo").is_ok()); @@ -75,244 +68,6 @@ fn run(args: Args) -> anyhow::Result<()> { } EasyCommand::simple("cargo", ["bench"]).success() } - Subcommand::Validate(cmd) => { - let ack_visiting = |path: &Path| log::info!("Validating {}", path.display()); - let err_status = match cmd { - ValidateSubcommand::Spirv => { - let spirv_as = "spirv-as"; - which(spirv_as)?; - - let spirv_val = "spirv-val"; - which(spirv_val)?; - - visit_files(snapshots_base_out, "spv/*.spvasm", |path| { - ack_visiting(path); - let second_line = { - let mut file = BufReader::new(open_file(path)?); - let mut buf = String::new(); - file.read_line(&mut buf).with_context(|| { - format!("failed to read first line from {path:?}") - })?; - buf.clear(); - file.read_line(&mut buf).with_context(|| { - format!("failed to read second line from {path:?}") - })?; - buf - }; - let expected_header_prefix = "; Version: "; - let Some(version) = - second_line.strip_prefix(expected_header_prefix) else { - bail!( - "no {expected_header_prefix:?} header found in {path:?}" - ); - }; - let file = open_file(path)?; - let mut spirv_as_cmd = EasyCommand::new(spirv_as, |cmd| { - cmd.stdin(Stdio::from(file)) - .stdout(Stdio::piped()) - .arg("--target-env") - .arg(format!("spv{version}")) - .args(["-", "-o", "-"]) - }); - let child = spirv_as_cmd - .spawn() - .with_context(|| format!("failed to spawn {cmd:?}"))?; - EasyCommand::new(spirv_val, |cmd| cmd.stdin(child.stdout.unwrap())) - .success() - }) - } - ValidateSubcommand::Metal => { - let xcrun = "xcrun"; - which(xcrun)?; - visit_files(snapshots_base_out, "msl/*.msl", |path| { - ack_visiting(path); - let first_line = { - let mut file = BufReader::new(open_file(path)?); - let mut buf = String::new(); - file.read_line(&mut buf) - .with_context(|| format!("failed to read header from {path:?}"))?; - buf - }; - let expected_header_prefix = "// language: "; - let Some(language) = - first_line.strip_prefix(expected_header_prefix) else { - bail!( - "no {expected_header_prefix:?} header found in {path:?}" - ); - }; - let language = language.strip_suffix('\n').unwrap_or(language); - - let file = open_file(path)?; - EasyCommand::new(xcrun, |cmd| { - cmd.stdin(Stdio::from(file)) - .args(["-sdk", "macosx", "metal", "-mmacosx-version-min=10.11"]) - .arg(format!("-std=macos-{language}")) - .args(["-x", "metal", "-", "-o", "/dev/null"]) - }) - .success() - }) - } - ValidateSubcommand::Glsl => { - let glslang_validator = "glslangValidator"; - which(glslang_validator)?; - let mut err_status = ErrorStatus::NoFailuresFound; - for (glob, type_arg) in [ - ("glsl/*.Vertex.glsl", "vert"), - ("glsl/*.Fragment.glsl", "frag"), - ("glsl/*.Compute.glsl", "comp"), - ] { - let type_err_status = visit_files(&snapshots_base_out, glob, |path| { - ack_visiting(path); - let file = open_file(path)?; - EasyCommand::new(glslang_validator, |cmd| { - cmd.stdin(Stdio::from(file)) - .args(["--stdin", "-S"]) - .arg(type_arg) - }) - .success() - }); - err_status = err_status.merge(type_err_status); - } - err_status - } - ValidateSubcommand::Dot => { - let dot = "dot"; - which(dot)?; - visit_files(snapshots_base_out, "dot/*.dot", |path| { - ack_visiting(path); - let file = open_file(path)?; - EasyCommand::new(dot, |cmd| { - cmd.stdin(Stdio::from(file)).stdout(Stdio::null()) - }) - .success() - }) - } - ValidateSubcommand::Wgsl => { - let mut paths = vec![]; - let mut error_status = visit_files(snapshots_base_out, "wgsl/*.wgsl", |path| { - paths.push(path.to_owned()); - Ok(()) - }); - EasyCommand::new("cargo", |cmd| { - cmd.args(["run", "-p", "naga-cli", "--", "--bulk-validate"]).args(paths) - }).success() - .log_if_err_found(&mut error_status); - error_status - } - ValidateSubcommand::Hlsl(cmd) => { - let visit_hlsl = |consume_config_item: &mut dyn FnMut( - &Path, - hlsl_snapshots::ConfigItem, - ) - -> anyhow::Result<()>| { - visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { - ack_visiting(path); - let hlsl_snapshots::Config { - vertex, - fragment, - compute, - } = hlsl_snapshots::Config::from_path(path.with_extension("ron"))?; - let mut status = ErrorStatus::NoFailuresFound; - [vertex, fragment, compute] - .into_iter() - .flatten() - .for_each(|shader| { - consume_config_item(path, shader).log_if_err_found(&mut status); - }); - match status { - ErrorStatus::NoFailuresFound => Ok(()), - ErrorStatus::OneOrMoreFailuresFound => bail!( - "one or more shader HLSL shader tests failed for {}", - path.display() - ), - } - }) - }; - let validate = |bin, file: &_, config_item, params: &[_]| { - let hlsl_snapshots::ConfigItem { - entry_point, - target_profile, - } = config_item; - EasyCommand::new(bin, |cmd| { - cmd.arg(file) - .arg("-T") - .arg(&target_profile) - .arg("-E") - .arg(&entry_point) - .args(params) - .stdout(Stdio::null()) - }) - .success() - .with_context(|| { - format!( - "failed to validate entry point {entry_point:?} with profile \ - {target_profile:?}" - ) - }) - }; - match cmd { - ValidateHlslCommand::Dxc => { - let bin = "dxc"; - which(bin)?; - visit_hlsl(&mut |file, config_item| { - // Reference: - // . - validate( - bin, - file, - config_item, - &[ - "-Wno-parentheses-equality", - "-Zi", - "-Qembed_debug", - "-Od", - "-HV", - "2018", - ], - ) - }) - } - ValidateHlslCommand::Fxc => { - let bin = "fxc"; - which(bin)?; - visit_hlsl(&mut |file, config_item| { - let Some(Ok(shader_model_major_version)) = config_item - .target_profile - .split('_') - .nth(1) - .map(|segment| segment.parse::()) else { - bail!( - "expected target profile of the form \ - `{{model}}_{{major}}_{{minor}}`, found invalid target \ - profile {:?} in file {}", - config_item.target_profile, - file.display() - ) - }; - // NOTE: This isn't implemented by `fxc.exe`; see - // . - if shader_model_major_version < 6 { - // Reference: - // . - validate(bin, file, config_item, &["-Zi", "-Od"]) - } else { - log::debug!( - "skipping config. item {config_item:?} because the \ - shader model major version is > 6" - ); - Ok(()) - } - }) - } - } - } - }; - match err_status { - ErrorStatus::NoFailuresFound => Ok(()), - ErrorStatus::OneOrMoreFailuresFound => { - bail!("failed to validate one or more files, see above output for more details") - } - } - } + Subcommand::Validate(cmd) => validate::validate(cmd), } } diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs new file mode 100644 index 0000000000..a8b27b5a1a --- /dev/null +++ b/naga/xtask/src/validate.rs @@ -0,0 +1,256 @@ +use std::{ + io::{BufRead, BufReader}, + path::Path, + process::Stdio, +}; + +use anyhow::{bail, Context}; + +use crate::{ + cli::{ValidateHlslCommand, ValidateSubcommand}, + fs::open_file, + glob::visit_files, + path::join_path, + process::{which, EasyCommand}, + result::{ErrorStatus, LogIfError}, +}; + +pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { + let snapshots_base_out = join_path(["tests", "out"]); + let ack_visiting = |path: &Path| log::info!("Validating {}", path.display()); + let err_status = match cmd { + ValidateSubcommand::Spirv => { + let spirv_as = "spirv-as"; + which(spirv_as)?; + + let spirv_val = "spirv-val"; + which(spirv_val)?; + + visit_files(snapshots_base_out, "spv/*.spvasm", |path| { + ack_visiting(path); + let second_line = { + let mut file = BufReader::new(open_file(path)?); + let mut buf = String::new(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read first line from {path:?}"))?; + buf.clear(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read second line from {path:?}"))?; + buf + }; + let expected_header_prefix = "; Version: "; + let Some(version) = + second_line.strip_prefix(expected_header_prefix) else { + bail!( + "no {expected_header_prefix:?} header found in {path:?}" + ); + }; + let file = open_file(path)?; + let mut spirv_as_cmd = EasyCommand::new(spirv_as, |cmd| { + cmd.stdin(Stdio::from(file)) + .stdout(Stdio::piped()) + .arg("--target-env") + .arg(format!("spv{version}")) + .args(["-", "-o", "-"]) + }); + let child = spirv_as_cmd + .spawn() + .with_context(|| format!("failed to spawn {cmd:?}"))?; + EasyCommand::new(spirv_val, |cmd| cmd.stdin(child.stdout.unwrap())).success() + }) + } + ValidateSubcommand::Metal => { + let xcrun = "xcrun"; + which(xcrun)?; + visit_files(snapshots_base_out, "msl/*.msl", |path| { + ack_visiting(path); + let first_line = { + let mut file = BufReader::new(open_file(path)?); + let mut buf = String::new(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read header from {path:?}"))?; + buf + }; + let expected_header_prefix = "// language: "; + let Some(language) = + first_line.strip_prefix(expected_header_prefix) else { + bail!( + "no {expected_header_prefix:?} header found in {path:?}" + ); + }; + let language = language.strip_suffix('\n').unwrap_or(language); + + let file = open_file(path)?; + EasyCommand::new(xcrun, |cmd| { + cmd.stdin(Stdio::from(file)) + .args(["-sdk", "macosx", "metal", "-mmacosx-version-min=10.11"]) + .arg(format!("-std=macos-{language}")) + .args(["-x", "metal", "-", "-o", "/dev/null"]) + }) + .success() + }) + } + ValidateSubcommand::Glsl => { + let glslang_validator = "glslangValidator"; + which(glslang_validator)?; + let mut err_status = ErrorStatus::NoFailuresFound; + for (glob, type_arg) in [ + ("glsl/*.Vertex.glsl", "vert"), + ("glsl/*.Fragment.glsl", "frag"), + ("glsl/*.Compute.glsl", "comp"), + ] { + let type_err_status = visit_files(&snapshots_base_out, glob, |path| { + ack_visiting(path); + let file = open_file(path)?; + EasyCommand::new(glslang_validator, |cmd| { + cmd.stdin(Stdio::from(file)) + .args(["--stdin", "-S"]) + .arg(type_arg) + }) + .success() + }); + err_status = err_status.merge(type_err_status); + } + err_status + } + ValidateSubcommand::Dot => { + let dot = "dot"; + which(dot)?; + visit_files(snapshots_base_out, "dot/*.dot", |path| { + ack_visiting(path); + let file = open_file(path)?; + EasyCommand::new(dot, |cmd| { + cmd.stdin(Stdio::from(file)).stdout(Stdio::null()) + }) + .success() + }) + } + ValidateSubcommand::Wgsl => { + let mut paths = vec![]; + let mut error_status = visit_files(snapshots_base_out, "wgsl/*.wgsl", |path| { + paths.push(path.to_owned()); + Ok(()) + }); + EasyCommand::new("cargo", |cmd| { + cmd.args(["run", "-p", "naga-cli", "--", "--bulk-validate"]) + .args(paths) + }) + .success() + .log_if_err_found(&mut error_status); + error_status + } + ValidateSubcommand::Hlsl(cmd) => { + let visit_hlsl = |consume_config_item: &mut dyn FnMut( + &Path, + hlsl_snapshots::ConfigItem, + ) + -> anyhow::Result<()>| { + visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { + ack_visiting(path); + let hlsl_snapshots::Config { + vertex, + fragment, + compute, + } = hlsl_snapshots::Config::from_path(path.with_extension("ron"))?; + let mut status = ErrorStatus::NoFailuresFound; + [vertex, fragment, compute] + .into_iter() + .flatten() + .for_each(|shader| { + consume_config_item(path, shader).log_if_err_found(&mut status); + }); + match status { + ErrorStatus::NoFailuresFound => Ok(()), + ErrorStatus::OneOrMoreFailuresFound => bail!( + "one or more shader HLSL shader tests failed for {}", + path.display() + ), + } + }) + }; + let validate = |bin, file: &_, config_item, params: &[_]| { + let hlsl_snapshots::ConfigItem { + entry_point, + target_profile, + } = config_item; + EasyCommand::new(bin, |cmd| { + cmd.arg(file) + .arg("-T") + .arg(&target_profile) + .arg("-E") + .arg(&entry_point) + .args(params) + .stdout(Stdio::null()) + }) + .success() + .with_context(|| { + format!( + "failed to validate entry point {entry_point:?} with profile \ + {target_profile:?}" + ) + }) + }; + match cmd { + ValidateHlslCommand::Dxc => { + let bin = "dxc"; + which(bin)?; + visit_hlsl(&mut |file, config_item| { + // Reference: + // . + validate( + bin, + file, + config_item, + &[ + "-Wno-parentheses-equality", + "-Zi", + "-Qembed_debug", + "-Od", + "-HV", + "2018", + ], + ) + }) + } + ValidateHlslCommand::Fxc => { + let bin = "fxc"; + which(bin)?; + visit_hlsl(&mut |file, config_item| { + let Some(Ok(shader_model_major_version)) = config_item + .target_profile + .split('_') + .nth(1) + .map(|segment| segment.parse::()) else { + bail!( + "expected target profile of the form \ + `{{model}}_{{major}}_{{minor}}`, found invalid target \ + profile {:?} in file {}", + config_item.target_profile, + file.display() + ) + }; + // NOTE: This isn't implemented by `fxc.exe`; see + // . + if shader_model_major_version < 6 { + // Reference: + // . + validate(bin, file, config_item, &["-Zi", "-Od"]) + } else { + log::debug!( + "skipping config. item {config_item:?} because the \ + shader model major version is > 6" + ); + Ok(()) + } + }) + } + } + } + }; + match err_status { + ErrorStatus::NoFailuresFound => Ok(()), + ErrorStatus::OneOrMoreFailuresFound => { + bail!("failed to validate one or more files, see above output for more details") + } + } +} From 25e453519d7a024cebc53db3d04261df7a2cab3d Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:46:49 -0800 Subject: [PATCH 02/11] [naga xtask] Break validation subcommands into functions. --- naga/xtask/src/validate.rs | 396 ++++++++++++++++++++----------------- 1 file changed, 217 insertions(+), 179 deletions(-) diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index a8b27b5a1a..1dee217758 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -15,9 +15,12 @@ use crate::{ result::{ErrorStatus, LogIfError}, }; +fn ack_visiting(path: &Path) { + log::info!("Validating {}", path.display()); +} + pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let snapshots_base_out = join_path(["tests", "out"]); - let ack_visiting = |path: &Path| log::info!("Validating {}", path.display()); let err_status = match cmd { ValidateSubcommand::Spirv => { let spirv_as = "spirv-as"; @@ -27,67 +30,14 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { which(spirv_val)?; visit_files(snapshots_base_out, "spv/*.spvasm", |path| { - ack_visiting(path); - let second_line = { - let mut file = BufReader::new(open_file(path)?); - let mut buf = String::new(); - file.read_line(&mut buf) - .with_context(|| format!("failed to read first line from {path:?}"))?; - buf.clear(); - file.read_line(&mut buf) - .with_context(|| format!("failed to read second line from {path:?}"))?; - buf - }; - let expected_header_prefix = "; Version: "; - let Some(version) = - second_line.strip_prefix(expected_header_prefix) else { - bail!( - "no {expected_header_prefix:?} header found in {path:?}" - ); - }; - let file = open_file(path)?; - let mut spirv_as_cmd = EasyCommand::new(spirv_as, |cmd| { - cmd.stdin(Stdio::from(file)) - .stdout(Stdio::piped()) - .arg("--target-env") - .arg(format!("spv{version}")) - .args(["-", "-o", "-"]) - }); - let child = spirv_as_cmd - .spawn() - .with_context(|| format!("failed to spawn {cmd:?}"))?; - EasyCommand::new(spirv_val, |cmd| cmd.stdin(child.stdout.unwrap())).success() + validate_spirv(path, spirv_as, spirv_val) }) } ValidateSubcommand::Metal => { let xcrun = "xcrun"; which(xcrun)?; visit_files(snapshots_base_out, "msl/*.msl", |path| { - ack_visiting(path); - let first_line = { - let mut file = BufReader::new(open_file(path)?); - let mut buf = String::new(); - file.read_line(&mut buf) - .with_context(|| format!("failed to read header from {path:?}"))?; - buf - }; - let expected_header_prefix = "// language: "; - let Some(language) = - first_line.strip_prefix(expected_header_prefix) else { - bail!( - "no {expected_header_prefix:?} header found in {path:?}" - ); - }; - let language = language.strip_suffix('\n').unwrap_or(language); - - let file = open_file(path)?; - EasyCommand::new(xcrun, |cmd| { - cmd.stdin(Stdio::from(file)) - .args(["-sdk", "macosx", "metal", "-mmacosx-version-min=10.11"]) - .arg(format!("-std=macos-{language}")) - .args(["-x", "metal", "-", "-o", "/dev/null"]) - }) - .success() + validate_metal(path, xcrun) }) } ValidateSubcommand::Glsl => { @@ -100,14 +50,7 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { ("glsl/*.Compute.glsl", "comp"), ] { let type_err_status = visit_files(&snapshots_base_out, glob, |path| { - ack_visiting(path); - let file = open_file(path)?; - EasyCommand::new(glslang_validator, |cmd| { - cmd.stdin(Stdio::from(file)) - .args(["--stdin", "-S"]) - .arg(type_arg) - }) - .success() + validate_glsl(path, type_arg, glslang_validator) }); err_status = err_status.merge(type_err_status); } @@ -117,12 +60,7 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let dot = "dot"; which(dot)?; visit_files(snapshots_base_out, "dot/*.dot", |path| { - ack_visiting(path); - let file = open_file(path)?; - EasyCommand::new(dot, |cmd| { - cmd.stdin(Stdio::from(file)).stdout(Stdio::null()) - }) - .success() + validate_dot(path, dot) }) } ValidateSubcommand::Wgsl => { @@ -131,121 +69,25 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { paths.push(path.to_owned()); Ok(()) }); - EasyCommand::new("cargo", |cmd| { - cmd.args(["run", "-p", "naga-cli", "--", "--bulk-validate"]) - .args(paths) - }) - .success() - .log_if_err_found(&mut error_status); + validate_wgsl(&paths).log_if_err_found(&mut error_status); error_status } - ValidateSubcommand::Hlsl(cmd) => { - let visit_hlsl = |consume_config_item: &mut dyn FnMut( - &Path, - hlsl_snapshots::ConfigItem, - ) - -> anyhow::Result<()>| { + ValidateSubcommand::Hlsl(cmd) => match cmd { + ValidateHlslCommand::Dxc => { + let bin = "dxc"; + which(bin)?; visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { - ack_visiting(path); - let hlsl_snapshots::Config { - vertex, - fragment, - compute, - } = hlsl_snapshots::Config::from_path(path.with_extension("ron"))?; - let mut status = ErrorStatus::NoFailuresFound; - [vertex, fragment, compute] - .into_iter() - .flatten() - .for_each(|shader| { - consume_config_item(path, shader).log_if_err_found(&mut status); - }); - match status { - ErrorStatus::NoFailuresFound => Ok(()), - ErrorStatus::OneOrMoreFailuresFound => bail!( - "one or more shader HLSL shader tests failed for {}", - path.display() - ), - } + visit_hlsl_shaders(path, bin, validate_hlsl_with_dxc) }) - }; - let validate = |bin, file: &_, config_item, params: &[_]| { - let hlsl_snapshots::ConfigItem { - entry_point, - target_profile, - } = config_item; - EasyCommand::new(bin, |cmd| { - cmd.arg(file) - .arg("-T") - .arg(&target_profile) - .arg("-E") - .arg(&entry_point) - .args(params) - .stdout(Stdio::null()) - }) - .success() - .with_context(|| { - format!( - "failed to validate entry point {entry_point:?} with profile \ - {target_profile:?}" - ) + } + ValidateHlslCommand::Fxc => { + let bin = "fxc"; + which(bin)?; + visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { + visit_hlsl_shaders(path, bin, validate_hlsl_with_fxc) }) - }; - match cmd { - ValidateHlslCommand::Dxc => { - let bin = "dxc"; - which(bin)?; - visit_hlsl(&mut |file, config_item| { - // Reference: - // . - validate( - bin, - file, - config_item, - &[ - "-Wno-parentheses-equality", - "-Zi", - "-Qembed_debug", - "-Od", - "-HV", - "2018", - ], - ) - }) - } - ValidateHlslCommand::Fxc => { - let bin = "fxc"; - which(bin)?; - visit_hlsl(&mut |file, config_item| { - let Some(Ok(shader_model_major_version)) = config_item - .target_profile - .split('_') - .nth(1) - .map(|segment| segment.parse::()) else { - bail!( - "expected target profile of the form \ - `{{model}}_{{major}}_{{minor}}`, found invalid target \ - profile {:?} in file {}", - config_item.target_profile, - file.display() - ) - }; - // NOTE: This isn't implemented by `fxc.exe`; see - // . - if shader_model_major_version < 6 { - // Reference: - // . - validate(bin, file, config_item, &["-Zi", "-Od"]) - } else { - log::debug!( - "skipping config. item {config_item:?} because the \ - shader model major version is > 6" - ); - Ok(()) - } - }) - } } - } + }, }; match err_status { ErrorStatus::NoFailuresFound => Ok(()), @@ -254,3 +96,199 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { } } } + +fn validate_spirv(path: &Path, spirv_as: &str, spirv_val: &str) -> anyhow::Result<()> { + ack_visiting(path); + let second_line = { + let mut file = BufReader::new(open_file(path)?); + let mut buf = String::new(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read first line from {path:?}"))?; + buf.clear(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read second line from {path:?}"))?; + buf + }; + let expected_header_prefix = "; Version: "; + let Some(version) = + second_line.strip_prefix(expected_header_prefix) else { + bail!( + "no {expected_header_prefix:?} header found in {path:?}" + ); + }; + let file = open_file(path)?; + let mut spirv_as_cmd = EasyCommand::new(spirv_as, |cmd| { + cmd.stdin(Stdio::from(file)) + .stdout(Stdio::piped()) + .arg("--target-env") + .arg(format!("spv{version}")) + .args(["-", "-o", "-"]) + }); + let child = spirv_as_cmd + .spawn() + .with_context(|| format!("failed to spawn {spirv_as_cmd:?}"))?; + EasyCommand::new(spirv_val, |cmd| cmd.stdin(child.stdout.unwrap())).success() +} + +fn validate_metal(path: &Path, xcrun: &str) -> anyhow::Result<()> { + ack_visiting(path); + let first_line = { + let mut file = BufReader::new(open_file(path)?); + let mut buf = String::new(); + file.read_line(&mut buf) + .with_context(|| format!("failed to read header from {path:?}"))?; + buf + }; + let expected_header_prefix = "// language: "; + let Some(language) = + first_line.strip_prefix(expected_header_prefix) else { + bail!( + "no {expected_header_prefix:?} header found in {path:?}" + ); + }; + let language = language.strip_suffix('\n').unwrap_or(language); + + let file = open_file(path)?; + EasyCommand::new(xcrun, |cmd| { + cmd.stdin(Stdio::from(file)) + .args(["-sdk", "macosx", "metal", "-mmacosx-version-min=10.11"]) + .arg(format!("-std=macos-{language}")) + .args(["-x", "metal", "-", "-o", "/dev/null"]) + }) + .success() +} + +fn validate_glsl(path: &Path, type_arg: &str, glslang_validator: &str) -> anyhow::Result<()> { + ack_visiting(path); + let file = open_file(path)?; + EasyCommand::new(glslang_validator, |cmd| { + cmd.stdin(Stdio::from(file)) + .args(["--stdin", "-S"]) + .arg(type_arg) + }) + .success() +} + +fn validate_dot(path: &Path, dot: &str) -> anyhow::Result<()> { + ack_visiting(path); + let file = open_file(path)?; + EasyCommand::new(dot, |cmd| { + cmd.stdin(Stdio::from(file)).stdout(Stdio::null()) + }) + .success() +} + +fn validate_wgsl(paths: &[std::path::PathBuf]) -> anyhow::Result<()> { + EasyCommand::new("cargo", |cmd| { + cmd.args(["run", "-p", "naga-cli", "--", "--bulk-validate"]) + .args(paths) + }) + .success() +} + +fn visit_hlsl_shaders( + path: &Path, + bin: &str, + mut consume_config_item: impl FnMut(&Path, hlsl_snapshots::ConfigItem, &str) -> anyhow::Result<()>, +) -> anyhow::Result<()> { + ack_visiting(path); + let hlsl_snapshots::Config { + vertex, + fragment, + compute, + } = hlsl_snapshots::Config::from_path(path.with_extension("ron"))?; + let mut status = ErrorStatus::NoFailuresFound; + for shader in [vertex, fragment, compute].into_iter().flatten() { + consume_config_item(path, shader, bin).log_if_err_found(&mut status); + } + match status { + ErrorStatus::NoFailuresFound => Ok(()), + ErrorStatus::OneOrMoreFailuresFound => bail!( + "one or more shader HLSL shader tests failed for {}", + path.display() + ), + } +} + +fn validate_hlsl_with_dxc( + file: &Path, + config_item: hlsl_snapshots::ConfigItem, + dxc: &str, +) -> anyhow::Result<()> { + // Reference: + // . + validate_hlsl( + file, + dxc, + config_item, + &[ + "-Wno-parentheses-equality", + "-Zi", + "-Qembed_debug", + "-Od", + "-HV", + "2018", + ], + ) +} + +fn validate_hlsl_with_fxc( + file: &Path, + config_item: hlsl_snapshots::ConfigItem, + fxc: &str, +) -> anyhow::Result<()> { + let Some(Ok(shader_model_major_version)) = config_item + .target_profile + .split('_') + .nth(1) + .map(|segment| segment.parse::()) else { + bail!( + "expected target profile of the form \ + `{{model}}_{{major}}_{{minor}}`, found invalid target \ + profile {:?} in file {}", + config_item.target_profile, + file.display() + ) + }; + // NOTE: This isn't implemented by `fxc.exe`; see + // . + if shader_model_major_version < 6 { + // Reference: + // . + validate_hlsl(file, fxc, config_item, &["-Zi", "-Od"]) + } else { + log::debug!( + "skipping config. item {config_item:?} because the \ + shader model major version is > 6" + ); + Ok(()) + } +} + +fn validate_hlsl( + file: &Path, + bin: &str, + config_item: hlsl_snapshots::ConfigItem, + params: &[&str], +) -> anyhow::Result<()> { + let hlsl_snapshots::ConfigItem { + entry_point, + target_profile, + } = config_item; + EasyCommand::new(bin, |cmd| { + cmd.arg(file) + .arg("-T") + .arg(&target_profile) + .arg("-E") + .arg(&entry_point) + .args(params) + .stdout(Stdio::null()) + }) + .success() + .with_context(|| { + format!( + "failed to validate entry point {entry_point:?} with profile \ + {target_profile:?}" + ) + }) +} From d33df26f346a2a83c9fe8a53df7d0bb2658d800a Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:47:37 -0800 Subject: [PATCH 03/11] [naga xtask] Produce output only when an error occurs. --- naga/xtask/src/process.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/naga/xtask/src/process.rs b/naga/xtask/src/process.rs index 37a24f1b6e..b70504f768 100644 --- a/naga/xtask/src/process.rs +++ b/naga/xtask/src/process.rs @@ -35,13 +35,15 @@ impl EasyCommand { pub fn success(&mut self) -> anyhow::Result<()> { let Self { inner } = self; log::debug!("running {inner:?}"); - let status = inner - .status() + let output = inner + .output() .with_context(|| format!("failed to run {self}"))?; ensure!( - status.success(), - "{self} failed to run; exit code: {:?}", - status.code() + output.status.success(), + "{self} failed to run; exit code: {:?}\nstdout:\n{}\nstderr:\n{}", + output.status.code(), + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), ); Ok(()) } From d5541e204a1c8047880f7517961f2aee367e925a Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:48:14 -0800 Subject: [PATCH 04/11] [naga xtask] Collect validation jobs in a Vec before running them. --- naga/xtask/src/glob.rs | 49 ++++++------ naga/xtask/src/validate.rs | 148 ++++++++++++++++++++++++------------- 2 files changed, 118 insertions(+), 79 deletions(-) diff --git a/naga/xtask/src/glob.rs b/naga/xtask/src/glob.rs index d69891e78a..4d2e1daf80 100644 --- a/naga/xtask/src/glob.rs +++ b/naga/xtask/src/glob.rs @@ -1,37 +1,34 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; use anyhow::Context; use glob::glob; -use crate::result::{ErrorStatus, LogIfError}; +/// Apply `f` to each file matching `pattern` in `top_dir`. +/// +/// Pass files as `anyhow::Result` values, to carry errors from +/// directory iteration and metadata checking. +pub(crate) fn for_each_file( + top_dir: impl AsRef, + pattern: impl AsRef, + mut f: impl FnMut(anyhow::Result), +) { + fn filter_files(glob: &str, result: glob::GlobResult) -> anyhow::Result> { + let path = result.with_context(|| format!("error while iterating over glob {glob:?}"))?; + let metadata = path + .metadata() + .with_context(|| format!("failed to fetch metadata for {path:?}"))?; + Ok(metadata.is_file().then_some(path)) + } -pub(crate) fn visit_files( - path: impl AsRef, - glob_expr: &str, - mut f: impl FnMut(&Path) -> anyhow::Result<()>, -) -> ErrorStatus { - let path = path.as_ref(); - let glob_expr = path.join(glob_expr); - let glob_expr = glob_expr.to_str().unwrap(); + let pattern_in_dir = top_dir.as_ref().join(pattern.as_ref()); + let pattern_in_dir = pattern_in_dir.to_str().unwrap(); - let mut status = ErrorStatus::NoFailuresFound; - glob(glob_expr) + glob(pattern_in_dir) .context("glob pattern {path:?} is invalid") .unwrap() - .for_each(|path_res| { - if let Some(path) = path_res - .with_context(|| format!("error while iterating over glob {path:?}")) - .log_if_err_found(&mut status) - { - if path - .metadata() - .with_context(|| format!("failed to fetch metadata for {path:?}")) - .log_if_err_found(&mut status) - .map_or(false, |m| m.is_file()) - { - f(&path).log_if_err_found(&mut status); - } + .for_each(|result| { + if let Some(result) = filter_files(pattern_in_dir, result).transpose() { + f(result); } }); - status } diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index 1dee217758..cf914df005 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -9,10 +9,8 @@ use anyhow::{bail, Context}; use crate::{ cli::{ValidateHlslCommand, ValidateSubcommand}, fs::open_file, - glob::visit_files, path::join_path, process::{which, EasyCommand}, - result::{ErrorStatus, LogIfError}, }; fn ack_visiting(path: &Path) { @@ -20,8 +18,48 @@ fn ack_visiting(path: &Path) { } pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { + let mut jobs = vec![]; + collect_validation_jobs(&mut jobs, cmd)?; + + let mut all_good = true; + for job in jobs { + if let Err(error) = job() { + all_good = false; + eprintln!("{:#}", error); + } + } + + if !all_good { + bail!("failed to validate one or more files, see above output for more details") + } + Ok(()) +} + +type Job = Box anyhow::Result<()>>; + +fn push_job_for_each_file( + top_dir: impl AsRef, + pattern: impl AsRef, + jobs: &mut Vec, + f: impl FnOnce(std::path::PathBuf) -> anyhow::Result<()> + Clone + 'static, +) { + crate::glob::for_each_file(top_dir, pattern, move |path_result| { + // Let each job closure stand on its own. + let f = f.clone(); + jobs.push(Box::new(|| f(path_result?))); + }); +} + +/// Call `f` to extend `jobs`, but if `f` itself fails, push a job that reports that. +fn try_push_job(jobs: &mut Vec, f: impl FnOnce(&mut Vec) -> anyhow::Result<()>) { + if let Err(error) = f(jobs) { + jobs.push(Box::new(|| Err(error))); + } +} + +fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyhow::Result<()> { let snapshots_base_out = join_path(["tests", "out"]); - let err_status = match cmd { + match cmd { ValidateSubcommand::Spirv => { let spirv_as = "spirv-as"; which(spirv_as)?; @@ -29,72 +67,76 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let spirv_val = "spirv-val"; which(spirv_val)?; - visit_files(snapshots_base_out, "spv/*.spvasm", |path| { - validate_spirv(path, spirv_as, spirv_val) - }) + push_job_for_each_file(snapshots_base_out, "spv/*.spvasm", jobs, |path| { + validate_spirv(&path, spirv_as, spirv_val) + }); } ValidateSubcommand::Metal => { let xcrun = "xcrun"; which(xcrun)?; - visit_files(snapshots_base_out, "msl/*.msl", |path| { - validate_metal(path, xcrun) - }) + push_job_for_each_file(snapshots_base_out, "msl/*.msl", jobs, |path| { + validate_metal(&path, xcrun) + }); } ValidateSubcommand::Glsl => { let glslang_validator = "glslangValidator"; which(glslang_validator)?; - let mut err_status = ErrorStatus::NoFailuresFound; for (glob, type_arg) in [ ("glsl/*.Vertex.glsl", "vert"), ("glsl/*.Fragment.glsl", "frag"), ("glsl/*.Compute.glsl", "comp"), ] { - let type_err_status = visit_files(&snapshots_base_out, glob, |path| { - validate_glsl(path, type_arg, glslang_validator) + push_job_for_each_file(&snapshots_base_out, glob, jobs, |path| { + validate_glsl(&path, type_arg, glslang_validator) }); - err_status = err_status.merge(type_err_status); } - err_status } ValidateSubcommand::Dot => { let dot = "dot"; which(dot)?; - visit_files(snapshots_base_out, "dot/*.dot", |path| { - validate_dot(path, dot) - }) + push_job_for_each_file(snapshots_base_out, "dot/*.dot", jobs, |path| { + validate_dot(&path, dot) + }); } ValidateSubcommand::Wgsl => { let mut paths = vec![]; - let mut error_status = visit_files(snapshots_base_out, "wgsl/*.wgsl", |path| { - paths.push(path.to_owned()); - Ok(()) + crate::glob::for_each_file(snapshots_base_out, "wgsl/*.wgsl", |path_result| { + try_push_job(jobs, |_| { + paths.push(path_result?.to_owned()); + Ok(()) + }) }); - validate_wgsl(&paths).log_if_err_found(&mut error_status); - error_status + if !paths.is_empty() { + jobs.push(Box::new(move || validate_wgsl(&paths))); + } } - ValidateSubcommand::Hlsl(cmd) => match cmd { - ValidateHlslCommand::Dxc => { - let bin = "dxc"; - which(bin)?; - visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { - visit_hlsl_shaders(path, bin, validate_hlsl_with_dxc) - }) + ValidateSubcommand::Hlsl(cmd) => { + let bin; + let validator: fn(&Path, hlsl_snapshots::ConfigItem, &str) -> anyhow::Result<()>; + match cmd { + ValidateHlslCommand::Dxc => { + bin = "dxc"; + which(bin)?; + validator = validate_hlsl_with_dxc; + } + ValidateHlslCommand::Fxc => { + bin = "fxc"; + which(bin)?; + validator = validate_hlsl_with_fxc; + } } - ValidateHlslCommand::Fxc => { - let bin = "fxc"; - which(bin)?; - visit_files(snapshots_base_out, "hlsl/*.hlsl", |path| { - visit_hlsl_shaders(path, bin, validate_hlsl_with_fxc) + + crate::glob::for_each_file(snapshots_base_out, "hlsl/*.hlsl", |path_result| { + try_push_job(jobs, |jobs| { + let path = path_result?; + push_job_for_each_hlsl_config_item(&path, bin, jobs, validator)?; + Ok(()) }) - } - }, - }; - match err_status { - ErrorStatus::NoFailuresFound => Ok(()), - ErrorStatus::OneOrMoreFailuresFound => { - bail!("failed to validate one or more files, see above output for more details") + }); } - } + }; + + Ok(()) } fn validate_spirv(path: &Path, spirv_as: &str, spirv_val: &str) -> anyhow::Result<()> { @@ -186,10 +228,13 @@ fn validate_wgsl(paths: &[std::path::PathBuf]) -> anyhow::Result<()> { .success() } -fn visit_hlsl_shaders( +fn push_job_for_each_hlsl_config_item( path: &Path, bin: &str, - mut consume_config_item: impl FnMut(&Path, hlsl_snapshots::ConfigItem, &str) -> anyhow::Result<()>, + jobs: &mut Vec, + validator: impl FnMut(&Path, hlsl_snapshots::ConfigItem, &str) -> anyhow::Result<()> + + Clone + + 'static, ) -> anyhow::Result<()> { ack_visiting(path); let hlsl_snapshots::Config { @@ -197,17 +242,14 @@ fn visit_hlsl_shaders( fragment, compute, } = hlsl_snapshots::Config::from_path(path.with_extension("ron"))?; - let mut status = ErrorStatus::NoFailuresFound; for shader in [vertex, fragment, compute].into_iter().flatten() { - consume_config_item(path, shader, bin).log_if_err_found(&mut status); - } - match status { - ErrorStatus::NoFailuresFound => Ok(()), - ErrorStatus::OneOrMoreFailuresFound => bail!( - "one or more shader HLSL shader tests failed for {}", - path.display() - ), + // Let each job closure stand on its own. + let mut validator = validator.clone(); + let path = path.to_owned(); + let bin = bin.to_owned(); + jobs.push(Box::new(move || validator(&path, shader, &bin))); } + Ok(()) } fn validate_hlsl_with_dxc( From e7899c118d9f243f2a011314fe15bff94e2e4d71 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:50:13 -0800 Subject: [PATCH 05/11] [naga xtask] Remove `ack_visiting` function. --- naga/xtask/src/validate.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index cf914df005..b1f112b63e 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -13,10 +13,6 @@ use crate::{ process::{which, EasyCommand}, }; -fn ack_visiting(path: &Path) { - log::info!("Validating {}", path.display()); -} - pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let mut jobs = vec![]; collect_validation_jobs(&mut jobs, cmd)?; @@ -140,7 +136,6 @@ fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyh } fn validate_spirv(path: &Path, spirv_as: &str, spirv_val: &str) -> anyhow::Result<()> { - ack_visiting(path); let second_line = { let mut file = BufReader::new(open_file(path)?); let mut buf = String::new(); @@ -173,7 +168,6 @@ fn validate_spirv(path: &Path, spirv_as: &str, spirv_val: &str) -> anyhow::Resul } fn validate_metal(path: &Path, xcrun: &str) -> anyhow::Result<()> { - ack_visiting(path); let first_line = { let mut file = BufReader::new(open_file(path)?); let mut buf = String::new(); @@ -201,7 +195,6 @@ fn validate_metal(path: &Path, xcrun: &str) -> anyhow::Result<()> { } fn validate_glsl(path: &Path, type_arg: &str, glslang_validator: &str) -> anyhow::Result<()> { - ack_visiting(path); let file = open_file(path)?; EasyCommand::new(glslang_validator, |cmd| { cmd.stdin(Stdio::from(file)) @@ -212,7 +205,6 @@ fn validate_glsl(path: &Path, type_arg: &str, glslang_validator: &str) -> anyhow } fn validate_dot(path: &Path, dot: &str) -> anyhow::Result<()> { - ack_visiting(path); let file = open_file(path)?; EasyCommand::new(dot, |cmd| { cmd.stdin(Stdio::from(file)).stdout(Stdio::null()) @@ -236,7 +228,6 @@ fn push_job_for_each_hlsl_config_item( + Clone + 'static, ) -> anyhow::Result<()> { - ack_visiting(path); let hlsl_snapshots::Config { vertex, fragment, From 04fe2a6289d88da107e30b628f3c3fed6758fb2c Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:50:42 -0800 Subject: [PATCH 06/11] [naga xtask] Use `indicatif` crate to report validation progress. --- naga/xtask/Cargo.lock | 132 +++++++++++++++++++++++++++++++++++++ naga/xtask/Cargo.toml | 1 + naga/xtask/src/validate.rs | 9 ++- 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/naga/xtask/Cargo.lock b/naga/xtask/Cargo.lock index 88ece956bd..6d2d134590 100644 --- a/naga/xtask/Cargo.lock +++ b/naga/xtask/Cargo.lock @@ -14,12 +14,31 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[package]] +name = "console" +version = "0.15.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c926e00cc70edefdc64d3a5ff31cc65bb97a3460097762bd23afb4d8145fccf8" +dependencies = [ + "encode_unicode", + "lazy_static", + "libc", + "unicode-width", + "windows-sys", +] + [[package]] name = "either" version = "1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" +[[package]] +name = "encode_unicode" +version = "0.3.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a357d28ed41a50f9c765dbfe56cbc04a64e53e5fc58ba79fbc34c10ef3df831f" + [[package]] name = "env_logger" version = "0.10.0" @@ -43,6 +62,34 @@ dependencies = [ "nanoserde", ] +[[package]] +name = "indicatif" +version = "0.17.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb28741c9db9a713d93deb3bb9515c20788cef5815265bee4980e87bde7e0f25" +dependencies = [ + "console", + "instant", + "number_prefix", + "portable-atomic", + "unicode-width", +] + +[[package]] +name = "instant" +version = "0.1.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a5bbe824c507c5da5956355e86a746d82e0e1464f65d862cc5e71da70e94b2c" +dependencies = [ + "cfg-if", +] + +[[package]] +name = "lazy_static" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646" + [[package]] name = "libc" version = "0.2.140" @@ -73,6 +120,12 @@ version = "0.1.19" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed7a94da6c6181c35d043fc61c43ac96d3a5d739e7b8027f77650ba41504d6ab" +[[package]] +name = "number_prefix" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" + [[package]] name = "once_cell" version = "1.17.1" @@ -85,12 +138,24 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5be167a7af36ee22fe3115051bc51f6e6c7054c9348e28deb4f49bd6f705a315" +[[package]] +name = "portable-atomic" +version = "1.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7170ef9988bc169ba16dd36a7fa041e5c4cbeb6a35b76d4c03daded371eae7c0" + [[package]] name = "shell-words" version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "24188a676b6ae68c3b2cb3a01be17fbf7240ce009799bb56d5b1409051e78fde" +[[package]] +name = "unicode-width" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e51733f11c9c4f72aa0c160008246859e340b00807569a0da0e7a1079b27ba85" + [[package]] name = "which" version = "4.4.0" @@ -102,6 +167,72 @@ dependencies = [ "once_cell", ] +[[package]] +name = "windows-sys" +version = "0.45.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" +dependencies = [ + "windows-targets", +] + +[[package]] +name = "windows-targets" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e5180c00cd44c9b1c88adb3693291f1cd93605ded80c250a75d472756b4d071" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "597a5118570b68bc08d8d59125332c54f1ba9d9adeedeef5b99b02ba2b0698f8" + +[[package]] +name = "windows_aarch64_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e08e8864a60f06ef0d0ff4ba04124db8b0fb3be5776a5cd47641e942e58c4d43" + +[[package]] +name = "windows_i686_gnu" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c61d927d8da41da96a81f029489353e68739737d3beca43145c8afec9a31a84f" + +[[package]] +name = "windows_i686_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "44d840b6ec649f480a41c8d80f9c65108b92d89345dd94027bfe06ac444d1060" + +[[package]] +name = "windows_x86_64_gnu" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8de912b8b8feb55c064867cf047dda097f92d51efad5b491dfb98f6bbb70cb36" + +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26d41b46a36d453748aedef1486d5c7a85db22e56aff34643984ea85514e94a3" + +[[package]] +name = "windows_x86_64_msvc" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9aec5da331524158c6d1a4ac0ab1541149c0b9505fde06423b02f5ef0106b9f0" + [[package]] name = "xtask" version = "0.1.0" @@ -110,6 +241,7 @@ dependencies = [ "env_logger", "glob", "hlsl-snapshots", + "indicatif", "log", "pico-args", "shell-words", diff --git a/naga/xtask/Cargo.toml b/naga/xtask/Cargo.toml index 579c521ea5..f1b5ff8a73 100644 --- a/naga/xtask/Cargo.toml +++ b/naga/xtask/Cargo.toml @@ -9,6 +9,7 @@ anyhow = "1" env_logger = { version = "0.10.0", default-features = false } glob = "0.3.1" hlsl-snapshots = { path = "../hlsl-snapshots"} +indicatif = "0.17" log = "0.4.17" pico-args = "0.5.0" shell-words = "1.1.0" diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index b1f112b63e..1d907525f2 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -17,14 +17,21 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let mut jobs = vec![]; collect_validation_jobs(&mut jobs, cmd)?; + let progress_bar = indicatif::ProgressBar::new(jobs.len() as u64); + let mut all_good = true; for job in jobs { if let Err(error) = job() { all_good = false; - eprintln!("{:#}", error); + progress_bar.suspend(|| { + eprintln!("{:#}", error); + }); } + progress_bar.inc(1); } + progress_bar.finish_and_clear(); + if !all_good { bail!("failed to validate one or more files, see above output for more details") } From cd6262dcc117c91537a9374a09d4557737f8b88e Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:51:08 -0800 Subject: [PATCH 07/11] [naga xtask] Run validation jobs in parallel, using jobserver. --- naga/xtask/Cargo.lock | 27 +++++++++++++ naga/xtask/Cargo.toml | 2 + naga/xtask/src/jobserver.rs | 34 ++++++++++++++++ naga/xtask/src/main.rs | 3 ++ naga/xtask/src/validate.rs | 77 ++++++++++++++++++++++++++----------- 5 files changed, 120 insertions(+), 23 deletions(-) create mode 100644 naga/xtask/src/jobserver.rs diff --git a/naga/xtask/Cargo.lock b/naga/xtask/Cargo.lock index 6d2d134590..a1727a8970 100644 --- a/naga/xtask/Cargo.lock +++ b/naga/xtask/Cargo.lock @@ -54,6 +54,12 @@ version = "0.3.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" +[[package]] +name = "hermit-abi" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d77f7ec81a6d05a3abb01ab6eb7590f6083d08449fe5a1c8b1e620283546ccb7" + [[package]] name = "hlsl-snapshots" version = "0.1.0" @@ -84,6 +90,15 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "jobserver" +version = "0.1.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8c37f63953c4c63420ed5fd3d6d398c719489b9f872b9fa683262f8edd363c7d" +dependencies = [ + "libc", +] + [[package]] name = "lazy_static" version = "1.4.0" @@ -120,6 +135,16 @@ version = "0.1.19" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed7a94da6c6181c35d043fc61c43ac96d3a5d739e7b8027f77650ba41504d6ab" +[[package]] +name = "num_cpus" +version = "1.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4161fcb6d602d4d2081af7c3a45852d875a03dd337a6bfdd6e06407b61342a43" +dependencies = [ + "hermit-abi", + "libc", +] + [[package]] name = "number_prefix" version = "0.4.0" @@ -242,7 +267,9 @@ dependencies = [ "glob", "hlsl-snapshots", "indicatif", + "jobserver", "log", + "num_cpus", "pico-args", "shell-words", "which", diff --git a/naga/xtask/Cargo.toml b/naga/xtask/Cargo.toml index f1b5ff8a73..159e54a586 100644 --- a/naga/xtask/Cargo.toml +++ b/naga/xtask/Cargo.toml @@ -10,7 +10,9 @@ env_logger = { version = "0.10.0", default-features = false } glob = "0.3.1" hlsl-snapshots = { path = "../hlsl-snapshots"} indicatif = "0.17" +jobserver = "0.1" log = "0.4.17" +num_cpus = "1.16" pico-args = "0.5.0" shell-words = "1.1.0" which = "4.4.0" diff --git a/naga/xtask/src/jobserver.rs b/naga/xtask/src/jobserver.rs new file mode 100644 index 0000000000..f035f81b75 --- /dev/null +++ b/naga/xtask/src/jobserver.rs @@ -0,0 +1,34 @@ +//! Running jobs in parallel, with a controlled degree of concurrency. + +use std::sync::OnceLock; + +use jobserver::Client; + +static JOB_SERVER: OnceLock = OnceLock::new(); + +pub fn init() { + JOB_SERVER.get_or_init(|| { + // Try to connect to a jobserver inherited from our parent. + if let Some(client) = unsafe { Client::from_env() } { + log::debug!("connected to inherited jobserver client"); + client + } else { + // Otherwise, start our own jobserver. + log::debug!("no inherited jobserver client; creating a new jobserver"); + Client::new(num_cpus::get()).expect("failed to create jobserver") + } + }); +} + +/// Wait until it is okay to start a new job, and then spawn a thread running `body`. +pub fn start_job_thread(body: F) -> anyhow::Result<()> +where + F: FnOnce() + Send + 'static, +{ + let acquired = JOB_SERVER.get().unwrap().acquire()?; + std::thread::spawn(move || { + body(); + drop(acquired); + }); + Ok(()) +} diff --git a/naga/xtask/src/main.rs b/naga/xtask/src/main.rs index 1ab6489128..aed7f48c71 100644 --- a/naga/xtask/src/main.rs +++ b/naga/xtask/src/main.rs @@ -13,6 +13,7 @@ use crate::{ mod cli; mod fs; mod glob; +mod jobserver; mod path; mod process; mod result; @@ -25,6 +26,8 @@ fn main() -> ExitCode { .format_indent(Some(0)) .init(); + jobserver::init(); + let args = Args::parse(); match run(args) { diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index 1d907525f2..cb27786c5b 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -13,15 +13,37 @@ use crate::{ process::{which, EasyCommand}, }; +type Job = Box anyhow::Result<()> + Send + std::panic::UnwindSafe + 'static>; + pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { let mut jobs = vec![]; collect_validation_jobs(&mut jobs, cmd)?; let progress_bar = indicatif::ProgressBar::new(jobs.len() as u64); + let (tx_results, rx_results) = std::sync::mpsc::channel(); + let enqueuing_thread = std::thread::spawn(move || -> anyhow::Result<()> { + for job in jobs { + let tx_results = tx_results.clone(); + crate::jobserver::start_job_thread(move || { + let result = match std::panic::catch_unwind(|| job()) { + Ok(result) => result, + Err(payload) => Err(match payload.downcast_ref::<&str>() { + Some(message) => { + anyhow::anyhow!("Validation job thread panicked: {}", message) + } + None => anyhow::anyhow!("Validation job thread panicked"), + }), + }; + tx_results.send(result).unwrap(); + })?; + } + Ok(()) + }); + let mut all_good = true; - for job in jobs { - if let Err(error) = job() { + for result in rx_results { + if let Err(error) = result { all_good = false; progress_bar.suspend(|| { eprintln!("{:#}", error); @@ -35,29 +57,12 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { if !all_good { bail!("failed to validate one or more files, see above output for more details") } - Ok(()) -} - -type Job = Box anyhow::Result<()>>; - -fn push_job_for_each_file( - top_dir: impl AsRef, - pattern: impl AsRef, - jobs: &mut Vec, - f: impl FnOnce(std::path::PathBuf) -> anyhow::Result<()> + Clone + 'static, -) { - crate::glob::for_each_file(top_dir, pattern, move |path_result| { - // Let each job closure stand on its own. - let f = f.clone(); - jobs.push(Box::new(|| f(path_result?))); - }); -} -/// Call `f` to extend `jobs`, but if `f` itself fails, push a job that reports that. -fn try_push_job(jobs: &mut Vec, f: impl FnOnce(&mut Vec) -> anyhow::Result<()>) { - if let Err(error) = f(jobs) { - jobs.push(Box::new(|| Err(error))); + if let Err(error) = enqueuing_thread.join().unwrap() { + bail!("Error enqueuing jobs:\n{:#}", error); } + + Ok(()) } fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyhow::Result<()> { @@ -142,6 +147,30 @@ fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyh Ok(()) } +fn push_job_for_each_file( + top_dir: impl AsRef, + pattern: impl AsRef, + jobs: &mut Vec, + f: impl FnOnce(std::path::PathBuf) -> anyhow::Result<()> + + Clone + + Send + + std::panic::UnwindSafe + + 'static, +) { + crate::glob::for_each_file(top_dir, pattern, move |path_result| { + // Let each job closure stand on its own. + let f = f.clone(); + jobs.push(Box::new(|| f(path_result?))); + }); +} + +/// Call `f` to extend `jobs`, but if `f` itself fails, push a job that reports that. +fn try_push_job(jobs: &mut Vec, f: impl FnOnce(&mut Vec) -> anyhow::Result<()>) { + if let Err(error) = f(jobs) { + jobs.push(Box::new(|| Err(error))); + } +} + fn validate_spirv(path: &Path, spirv_as: &str, spirv_val: &str) -> anyhow::Result<()> { let second_line = { let mut file = BufReader::new(open_file(path)?); @@ -233,6 +262,8 @@ fn push_job_for_each_hlsl_config_item( jobs: &mut Vec, validator: impl FnMut(&Path, hlsl_snapshots::ConfigItem, &str) -> anyhow::Result<()> + Clone + + Send + + std::panic::UnwindSafe + 'static, ) -> anyhow::Result<()> { let hlsl_snapshots::Config { From 517cde939b42f12b61af9476c470bf3d479f8cfe Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 19 Dec 2023 13:51:40 -0800 Subject: [PATCH 08/11] [naga xtask] Add `validate all` subcommand. --- CHANGELOG.md | 2 ++ naga/xtask/src/cli.rs | 9 ++++++++- naga/xtask/src/validate.rs | 21 +++++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa98cd6f1e..995d3f039d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,8 @@ Wgpu now exposes backend feature for the Direct3D 12 (`dx12`) and Metal (`metal` - Add `--bulk-validate` option to Naga CLI. By @jimblandy in [#4871](https://github.com/gfx-rs/wgpu/pull/4871). +- Naga's `cargo xtask validate` now runs validation jobs in parallel, using the [jobserver](https://crates.io/crates/jobserver) protocol to limit concurrency, and offers a `validate all` subcommand, which runs all available validation types. By @jimblandy in [#4902](https://github.com/gfx-rs/wgpu/pull/4902). + ### Changes - Arcanization of wgpu core resources: By @gents83 in [#3626](https://github.com/gfx-rs/wgpu/pull/3626) and thanks also to @jimblandy, @nical, @Wumpf, @Elabajaba & @cwfitzgerald diff --git a/naga/xtask/src/cli.rs b/naga/xtask/src/cli.rs index 3b5b1ed69f..cd510124c6 100644 --- a/naga/xtask/src/cli.rs +++ b/naga/xtask/src/cli.rs @@ -77,6 +77,8 @@ impl Subcommand { } } +// If you add a new validation subcommand, be sure to update the code +// that processes `All`. #[derive(Debug)] pub(crate) enum ValidateSubcommand { Spirv, @@ -85,6 +87,7 @@ pub(crate) enum ValidateSubcommand { Dot, Wgsl, Hlsl(ValidateHlslCommand), + All, } impl ValidateSubcommand { @@ -114,7 +117,11 @@ impl ValidateSubcommand { ensure_remaining_args_empty(args)?; Ok(Self::Wgsl) } - "hlsl" => return Ok(Self::Hlsl(ValidateHlslCommand::parse(args)?)), + "hlsl" => Ok(Self::Hlsl(ValidateHlslCommand::parse(args)?)), + "all" => { + ensure_remaining_args_empty(args)?; + Ok(Self::All) + } other => { bail!("unrecognized `validate` subcommand {other:?}; see `--help` for more details") } diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index cb27786c5b..287d05b759 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -142,6 +142,27 @@ fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyh }) }); } + ValidateSubcommand::All => { + collect_validation_jobs(jobs, ValidateSubcommand::Wgsl)?; + collect_validation_jobs(jobs, ValidateSubcommand::Spirv)?; + collect_validation_jobs(jobs, ValidateSubcommand::Glsl)?; + collect_validation_jobs(jobs, ValidateSubcommand::Dot)?; + + #[cfg(any(target_os = "macos", target_os = "ios"))] + collect_validation_jobs(jobs, ValidateSubcommand::Metal)?; + + // The FXC compiler is only available on Windows. + // + // The DXC compiler can be built and run on any platform, + // but they don't make Linux releases and it's not clear + // what Git commit actually works on Linux, so restrict + // that to Windows as well. + #[cfg(windows)] + { + collect_validation_jobs(jobs, ValidateSubcommand::Hlsl(ValidateHlslCommand::Dxc))?; + collect_validation_jobs(jobs, ValidateSubcommand::Hlsl(ValidateHlslCommand::Fxc))?; + } + } }; Ok(()) From 4ab26d2978335567db59d292efbf4fdcb001c4f5 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 25 Dec 2023 17:21:44 -0800 Subject: [PATCH 09/11] [naga xtask] Use `log::error` in preference to `eprintln`. --- naga/xtask/src/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index 287d05b759..967e9cfa59 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -46,7 +46,7 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { if let Err(error) = result { all_good = false; progress_bar.suspend(|| { - eprintln!("{:#}", error); + log::error!("{:#}", error); }); } progress_bar.inc(1); From b22b5654d118b7370273595e87b55155ce940717 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 25 Dec 2023 17:25:23 -0800 Subject: [PATCH 10/11] [naga xtask] Use `anyhow::ensure!` where appropriate. --- naga/xtask/src/validate.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index 967e9cfa59..bed71b05d8 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -54,9 +54,10 @@ pub(crate) fn validate(cmd: ValidateSubcommand) -> anyhow::Result<()> { progress_bar.finish_and_clear(); - if !all_good { - bail!("failed to validate one or more files, see above output for more details") - } + anyhow::ensure!( + all_good, + "failed to validate one or more files, see above output for more details" + ); if let Err(error) = enqueuing_thread.join().unwrap() { bail!("Error enqueuing jobs:\n{:#}", error); From efb563e242a6189eb29ddcfcd88954e05310af23 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Wed, 27 Dec 2023 12:39:32 -0800 Subject: [PATCH 11/11] [naga xtask] Add and use `ValidateSubcommand::all`. Add an `all` method to `ValidateSubcommand`, so that we can just use matches elsewhere, and let the compile catch missing or duplicated cases for us. --- naga/xtask/src/cli.rs | 13 +++++++++++++ naga/xtask/src/validate.rs | 39 ++++++++++++++++++++------------------ 2 files changed, 34 insertions(+), 18 deletions(-) diff --git a/naga/xtask/src/cli.rs b/naga/xtask/src/cli.rs index cd510124c6..d41b86ba2f 100644 --- a/naga/xtask/src/cli.rs +++ b/naga/xtask/src/cli.rs @@ -127,6 +127,19 @@ impl ValidateSubcommand { } } } + + pub(crate) fn all() -> impl Iterator { + [ + Self::Spirv, + Self::Metal, + Self::Glsl, + Self::Dot, + Self::Wgsl, + Self::Hlsl(ValidateHlslCommand::Dxc), + Self::Hlsl(ValidateHlslCommand::Fxc), + ] + .into_iter() + } } #[derive(Debug)] diff --git a/naga/xtask/src/validate.rs b/naga/xtask/src/validate.rs index bed71b05d8..394b7b00d4 100644 --- a/naga/xtask/src/validate.rs +++ b/naga/xtask/src/validate.rs @@ -144,24 +144,27 @@ fn collect_validation_jobs(jobs: &mut Vec, cmd: ValidateSubcommand) -> anyh }); } ValidateSubcommand::All => { - collect_validation_jobs(jobs, ValidateSubcommand::Wgsl)?; - collect_validation_jobs(jobs, ValidateSubcommand::Spirv)?; - collect_validation_jobs(jobs, ValidateSubcommand::Glsl)?; - collect_validation_jobs(jobs, ValidateSubcommand::Dot)?; - - #[cfg(any(target_os = "macos", target_os = "ios"))] - collect_validation_jobs(jobs, ValidateSubcommand::Metal)?; - - // The FXC compiler is only available on Windows. - // - // The DXC compiler can be built and run on any platform, - // but they don't make Linux releases and it's not clear - // what Git commit actually works on Linux, so restrict - // that to Windows as well. - #[cfg(windows)] - { - collect_validation_jobs(jobs, ValidateSubcommand::Hlsl(ValidateHlslCommand::Dxc))?; - collect_validation_jobs(jobs, ValidateSubcommand::Hlsl(ValidateHlslCommand::Fxc))?; + for subcmd in ValidateSubcommand::all() { + let should_validate = match &subcmd { + ValidateSubcommand::Wgsl + | ValidateSubcommand::Spirv + | ValidateSubcommand::Glsl + | ValidateSubcommand::Dot => true, + ValidateSubcommand::Metal => cfg!(any(target_os = "macos", target_os = "ios")), + // The FXC compiler is only available on Windows. + // + // The DXC compiler can be built and run on any platform, + // but they don't make Linux releases and it's not clear + // what Git commit actually works on Linux, so restrict + // that to Windows as well. + ValidateSubcommand::Hlsl( + ValidateHlslCommand::Dxc | ValidateHlslCommand::Fxc, + ) => cfg!(os = "windows"), + ValidateSubcommand::All => continue, + }; + if should_validate { + collect_validation_jobs(jobs, subcmd)?; + } } } };