diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index f904de91..ee5d22ce 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -500,8 +500,7 @@ pub struct FeedbackOptions { /// Provide this option with a path to which a summary comment is written. /// - /// If the given path is relative, then it shall be relative to the given - /// [`--repo-root`](#-r-repo-root) path. + /// A relative path is relative to the current working directory. #[cfg_attr( feature = "bin", arg( @@ -517,8 +516,7 @@ pub struct FeedbackOptions { /// written in the [SARIF 2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html) /// format. /// - /// If the given path is relative, then it shall be relative to the given - /// [`--repo-root`](#-r-repo-root) path. + /// A relative path is relative to the current working directory. /// /// The SARIF file only includes clang-tidy diagnostics that are also /// reported in other forms of feedback. So, source filters like diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index e0f201dc..84d95306 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -12,6 +12,14 @@ use crate::clang_tools::clang_tidy::CompilationUnit; use git_bot_feedback::FileFilter; +/// Resolve a path given on the command line against the current working directory. +/// +/// The path is kept as given if the working directory cannot be read. +#[cfg(feature = "bin")] +fn absolute_or_given(path: &Path) -> PathBuf { + std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()) +} + /// An enum to describe `--lines-changed-only` CLI option's behavior. #[derive(PartialEq, Clone, Debug, Default)] pub enum LinesChangedOnly { @@ -361,12 +369,20 @@ impl From<&Cli> for FeedbackInput { pr_review: args.feedback_options.pr_review, passive_reviews: args.feedback_options.passive_reviews, repo_root: args.source_options.repo_root.clone(), - summary_output_file: args.feedback_options.summary_output_file.clone(), - sarif_file: args.feedback_options.sarif_file.clone(), + summary_output_file: args + .feedback_options + .summary_output_file + .as_deref() + .map(absolute_or_given), + sarif_file: args + .feedback_options + .sarif_file + .as_deref() + .map(absolute_or_given), gen_patch: args.feedback_options.gen_patch || args.feedback_options.gen_patch_path.is_some(), patch_path: match &args.feedback_options.gen_patch_path { - Some(path) => std::path::absolute(path).unwrap_or_else(|_| path.to_path_buf()), + Some(path) => absolute_or_given(path), None => ClangParams::default_patch_path(&args.source_options.repo_root), }, } @@ -488,6 +504,30 @@ mod test { assert!(ClangParams::from(&cli).gen_patch); } + #[test] + fn relative_output_files() { + let cli = Cli::parse_from([ + "cpp-linter", + "--repo-root", + "sub", + "--summary-output-file", + "summary.md", + "--sarif-file", + "out/results.sarif", + ]); + let feedback_input = FeedbackInput::from(&cli); + // a relative path is relative to the working directory, not the repo root + let cwd = std::env::current_dir().unwrap(); + assert_eq!( + feedback_input.summary_output_file, + Some(cwd.join("summary.md")) + ); + assert_eq!( + feedback_input.sarif_file, + Some(cwd.join("out/results.sarif")) + ); + } + #[test] fn gen_patch_path_from_env() { unsafe { diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index be0c35aa..a6654c03 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -206,12 +206,7 @@ impl RestClient { if feedback_inputs.step_summary { self.client.append_step_summary(&summary)?; } - if let Some(summary_output_file) = &feedback_inputs.summary_output_file { - let output_file = if summary_output_file.is_absolute() { - summary_output_file - } else { - &feedback_inputs.repo_root.join(summary_output_file) - }; + if let Some(output_file) = &feedback_inputs.summary_output_file { if let Some(parent) = output_file.parent() { std::fs::create_dir_all(parent).map_err(|e| ClientError::MkDirFailed { file_path: output_file.clone(), @@ -227,12 +222,7 @@ impl RestClient { } comment = Some(summary); } - if let Some(sarif_file) = &feedback_inputs.sarif_file { - let output_file = if sarif_file.is_absolute() { - sarif_file - } else { - &feedback_inputs.repo_root.join(sarif_file) - }; + if let Some(output_file) = &feedback_inputs.sarif_file { SarifLog::new( files, clang_versions.tidy_version.as_ref(), diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index c55fec9d..1b161f3c 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -252,18 +252,20 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { ); } - let summary_out_file_path = if test_params.relative_out_files { - std::path::PathBuf::from(SUMMARY_OUT_FILE_NAME) + // A relative output path is resolved against the working directory (not the repo root). + // `_out_dir` keeps the directory alive until the test ends. + let (_out_dir, out_dir_path) = if test_params.relative_out_files { + let out_dir = tempfile::tempdir_in("./").unwrap(); + let cwd = env::current_dir().unwrap(); + let relative = out_dir.path().strip_prefix(&cwd).unwrap().to_path_buf(); + (out_dir, relative) } else { - tmp_dir.path().join(SUMMARY_OUT_FILE_NAME) - }; - - // a relative path is resolved against the repo root - let sarif_file_path = if test_params.relative_out_files { - std::path::PathBuf::from(SARIF_FILE_NAME) - } else { - tmp_dir.path().join(SARIF_FILE_NAME) + let out_dir = tempfile::tempdir().unwrap(); + let path = out_dir.path().to_path_buf(); + (out_dir, path) }; + let summary_out_file_path = out_dir_path.join(SUMMARY_OUT_FILE_NAME); + let sarif_file_path = out_dir_path.join(SARIF_FILE_NAME); let mut args = vec![ "cpp-linter".to_string(), @@ -334,12 +336,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { assert!(apply_patch_status.success()); } - let summary_out_file_abs_path = if test_params.relative_out_files { - tmp_dir.path().join(SUMMARY_OUT_FILE_NAME) - } else { - summary_out_file_path.clone() - }; - let summary_content = std::fs::read_to_string(&summary_out_file_abs_path).unwrap(); + let summary_content = std::fs::read_to_string(&summary_out_file_path).unwrap(); assert!(summary_content.contains(COMMENT_MARKER)); assert!(summary_content.contains(USER_OUTREACH)); @@ -352,8 +349,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { .parse::() .unwrap(); let sarif: serde_json::Value = - serde_json::from_str(&fs::read_to_string(tmp_dir.path().join(SARIF_FILE_NAME)).unwrap()) - .unwrap(); + serde_json::from_str(&fs::read_to_string(&sarif_file_path).unwrap()).unwrap(); assert_eq!(sarif["version"], SARIF_VERSION); let schema: serde_json::Value = serde_json::from_str(include_str!("sarif-schema-2.1.0.json")).unwrap();