Remove panics from xml writing (#1075)

Author: johnynek

bazelfe-bazel-wrapper/src/bazel_command_line_parser/mod.rs

@@ -329,7 +329,7 @@ pub fn parse_bazel_command_line(
             if cur_options.is_empty() {
                 break 'outer;
             } else {
-                action_options.extend(cur_options.into_iter());
+                action_options.extend(cur_options);
             }
         }
         action_args.extend(command_line_iter.cloned());

bazelfe-bazel-wrapper/src/bazel_subprocess_wrapper/mod.rs

@@ -148,7 +148,7 @@ async fn execute_tokio_subprocess(
                 break;
             }
             if show_output {
-                if let Err(_) = stdout.write_all(&buffer[0..bytes_read]).await {
+                if stdout.write_all(&buffer[0..bytes_read]).await.is_err() {
                     break;
                 }
             }
@@ -165,7 +165,7 @@ async fn execute_tokio_subprocess(
                 break;
             }
             if show_output {
-                if let Err(_) = stderr.write_all(&buffer[0..bytes_read]).await {
+                if stderr.write_all(&buffer[0..bytes_read]).await.is_err() {
                     break;
                 }
             }

bazelfe-bazel-wrapper/src/bep/build_events/build_event_server.rs

@@ -91,10 +91,12 @@ pub mod bazel_event {
 
             let aborted: Option<Evt> = {
                 let abort_info = v.payload.as_ref().and_then(|e| match e {
-                    build_event_stream::build_event::Payload::Aborted(cfg) => Some((
-                        build_event_stream::aborted::AbortReason::from_i32(cfg.reason),
-                        cfg.description.clone(),
-                    )),
+                    build_event_stream::build_event::Payload::Aborted(cfg) => Some(
+                        match build_event_stream::aborted::AbortReason::try_from(cfg.reason) {
+                            Ok(ar) => (Some(ar), cfg.description.clone()),
+                            Err(_) => (None, cfg.description.clone()),
+                        },
+                    ),
                     _ => None,
                 });
                 let target_label_opt = v.id.as_ref().and_then(|e| e.id.as_ref()).and_then(label_of);
@@ -449,7 +451,7 @@ where
                         let sequence_number = build_event.sequence_number;
                     yield PublishBuildToolEventStreamResponse {
                         stream_id: build_event.stream_id.clone(),
-                        sequence_number: sequence_number
+                        sequence_number
                     };
 
             }

bazelfe-bazel-wrapper/src/bep/build_events/hydrated_stream.rs

@@ -13,7 +13,6 @@ use super::build_event_server::BuildEventAction;
 use bazelfe_protos::build_event_stream::NamedSetOfFiles;
 use bazelfe_protos::*;
 use std::path::PathBuf;
-use std::pin::pin;
 
 pub trait HasFiles {
     fn files(&self) -> Vec<build_event_stream::File>;
@@ -324,6 +323,24 @@ impl HydratedInfo {
         });
         next_rx
     }
+
+    pub fn label(&self) -> Option<&str> {
+        match self {
+            HydratedInfo::BazelAbort(ba) =>
+            // this is a dance to return the ref
+            {
+                match &ba.label {
+                    Some(s) => Some(s.as_str()),
+                    None => None,
+                }
+            }
+            HydratedInfo::ActionFailed(af) => Some(af.label.as_str()),
+            HydratedInfo::Progress(_) => None,
+            HydratedInfo::TestResult(tri) => Some(tri.test_summary_event.label.as_str()),
+            HydratedInfo::ActionSuccess(asucc) => Some(asucc.label.as_str()),
+            HydratedInfo::TargetComplete(tc) => Some(tc.label.as_str()),
+        }
+    }
 }
 
 #[cfg(test)]
@@ -334,7 +351,7 @@ mod tests {
     #[tokio::test]
     async fn test_no_history() {
         let (tx, rx) = async_channel::unbounded();
-        let mut child_rx = pin!(HydratedInfo::build_transformer(rx));
+        let mut child_rx = std::pin::pin!(HydratedInfo::build_transformer(rx));
 
         tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent {
             event: bazel_event::Evt::ActionCompleted(bazel_event::ActionCompletedEvt {
@@ -363,7 +380,7 @@ mod tests {
     #[tokio::test]
     async fn test_with_files() {
         let (tx, rx) = async_channel::unbounded();
-        let mut child_rx = pin!(HydratedInfo::build_transformer(rx));
+        let mut child_rx = std::pin::pin!(HydratedInfo::build_transformer(rx));
 
         tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent {
             event: bazel_event::Evt::ActionCompleted(bazel_event::ActionCompletedEvt {
@@ -413,7 +430,7 @@ mod tests {
     #[tokio::test]
     async fn test_with_history() {
         let (tx, rx) = async_channel::unbounded();
-        let mut child_rx = pin!(HydratedInfo::build_transformer(rx));
+        let mut child_rx = std::pin::pin!(HydratedInfo::build_transformer(rx));
 
         tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent {
             event: bazel_event::Evt::TargetConfigured(bazel_event::TargetConfiguredEvt {
@@ -472,7 +489,7 @@ mod tests {
     #[tokio::test]
     async fn state_resets_on_new_build() {
         let (tx, rx) = async_channel::unbounded();
-        let mut child_rx = pin!(HydratedInfo::build_transformer(rx));
+        let mut child_rx = std::pin::pin!(HydratedInfo::build_transformer(rx));
 
         tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent {
             event: bazel_event::Evt::TargetConfigured(bazel_event::TargetConfiguredEvt {

bazelfe-bazel-wrapper/src/bep/target_completed_tracker/mod.rs

@@ -16,13 +16,10 @@ impl<T> super::BazelEventHandler<T> for TargetCompletedTracker {
         _bazel_run_id: usize,
         event: &hydrated_stream::HydratedInfo,
     ) -> Vec<T> {
-        match event {
-            hydrated_stream::HydratedInfo::TargetComplete(tce) => {
-                let mut guard = self.expected_targets.lock().await;
-                guard.remove(&tce.label);
-            }
-            _ => (),
-        };
+        if let hydrated_stream::HydratedInfo::TargetComplete(tce) = event {
+            let mut guard = self.expected_targets.lock().await;
+            guard.remove(&tce.label);
+        }
         Vec::default()
     }
 }

bazelfe-core/src/bazel_runner/bazel_runner_inst.rs

@@ -70,7 +70,7 @@ impl BazelRunner {
         if let Some(bazel_command_line_parser::Action::Custom(cust_str)) =
             self.bazel_command_line.action.as_ref()
         {
-            if CustomAction::AutoTest == parse_custom_action(&cust_str)? {
+            if CustomAction::AutoTest == parse_custom_action(cust_str)? {
                 self.config.daemon_config.enabled = true;
             }
         }

bazelfe-core/src/bazel_runner/processor_activity.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;
 
 use crate::hydrated_stream_processors::process_bazel_failures::{TargetStory, TargetStoryAction};
 
+#[derive(Default)]
 pub struct ProcessorActivity {
     pub jvm_segments_indexed: u32,
     pub actions_taken: u32,
@@ -63,12 +64,3 @@ impl ProcessorActivity {
         self.actions_taken += o.actions_taken;
     }
 }
-impl Default for ProcessorActivity {
-    fn default() -> Self {
-        ProcessorActivity {
-            jvm_segments_indexed: 0,
-            actions_taken: 0,
-            target_story_actions: HashMap::new(),
-        }
-    }
-}

bazelfe-core/src/bazel_runner/test_file_to_target/mod.rs

@@ -58,7 +58,7 @@ pub async fn run<B: BazelQuery>(
         };
 
         let query_for_target = bazel_query
-            .execute(&vec![
+            .execute(&[
                 String::from("query"),
                 format!(
                     "rdeps(//{}/..., {},1)",

bazelfe-core/src/bep_junit/bep_junit_app.rs

@@ -1,7 +1,7 @@
 use bazelfe_bazel_wrapper::bep::build_events::build_event_server::BuildEventAction;
 use bazelfe_core::bep_junit::{
     emit_backup_error_data, emit_junit_xml_from_aborted_action, emit_junit_xml_from_failed_action,
-    label_to_junit_relative_path, suites_with_error_from_xml,
+    label_to_junit_relative_path,
 };
 
 use bazelfe_bazel_wrapper::bep::build_events::hydrated_stream::{HydratedInfo, HydratorState};
@@ -10,6 +10,7 @@ use clap::Parser;
 use prost::Message;
 use std::collections::VecDeque;
 use std::error::Error;
+use std::os::unix::fs::MetadataExt;
 use std::path::{Path, PathBuf};
 
 #[derive(Parser, Debug)]
@@ -77,19 +78,26 @@ async fn main() -> Result<(), Box<dyn Error>> {
     let mut failed_actions = Vec::default();
     let mut aborted_actions = Vec::default();
     let mut failed_tests = Vec::default();
+    let mut failed_xml_writes = Vec::default();
     for build_event in hydrated_infos.into_iter() {
-        match build_event {
+        match &build_event {
             HydratedInfo::BazelAbort(abort_info) => {
-                emit_junit_xml_from_aborted_action(
+                aborted_actions.push(abort_info.label.clone());
+                match emit_junit_xml_from_aborted_action(
                     &abort_info,
                     aborted_actions.len(),
                     &opt.junit_output_path,
-                );
-                aborted_actions.push(abort_info.label);
+                ) {
+                    Ok(_) => (),
+                    Err(e) => failed_xml_writes.push((build_event, e)),
+                }
             }
             HydratedInfo::ActionFailed(action_failed) => {
-                emit_junit_xml_from_failed_action(&action_failed, &opt.junit_output_path);
-                failed_actions.push(action_failed.label);
+                failed_actions.push(action_failed.label.clone());
+                match emit_junit_xml_from_failed_action(action_failed, &opt.junit_output_path) {
+                    Ok(_) => (),
+                    Err(e) => failed_xml_writes.push((build_event, e)),
+                }
             }
             HydratedInfo::TestResult(r) => {
                 let is_failure = r.test_summary_event.test_status.didnt_pass();
@@ -99,35 +107,60 @@ async fn main() -> Result<(), Box<dyn Error>> {
                 let output_folder = opt.junit_output_path.join(label_to_junit_relative_path(
                     r.test_summary_event.label.as_str(),
                 ));
-                std::fs::create_dir_all(&output_folder).expect("Make dir failed");
-
-                let files: Vec<&str> = r
-                    .test_summary_event
-                    .output_files
-                    .iter()
-                    .flat_map(|e| match e {
-                        bazelfe_protos::build_event_stream::file::File::Uri(uri) => {
-                            let p = uri
-                                .strip_prefix("file://")
-                                .unwrap_or_else(|| panic!("Wasn't a local file for {}", uri));
-                            if p.ends_with("/test.xml") {
-                                Some(p)
-                            } else {
-                                None
+
+                match std::fs::create_dir_all(&output_folder) {
+                    Ok(_) => {
+                        let files: Vec<&str> = r
+                            .test_summary_event
+                            .output_files
+                            .iter()
+                            .flat_map(|e| match e {
+                                bazelfe_protos::build_event_stream::file::File::Uri(uri) => {
+                                    let p = uri.strip_prefix("file://").unwrap_or_else(|| {
+                                        panic!("Wasn't a local file for {}", uri)
+                                    });
+                                    if p.ends_with("/test.xml") {
+                                        Some(p)
+                                    } else {
+                                        None
+                                    }
+                                }
+                                bazelfe_protos::build_event_stream::file::File::Contents(_) => None,
+                            })
+                            .collect();
+
+                        for (idx, f) in files.into_iter().enumerate() {
+                            match std::fs::metadata(f) {
+                          Ok(m) => {
+                            if m.size() > 0 {
+                              let output_file = output_folder.join(format!("test.{}.xml", idx));
+                              match std::fs::copy(f, output_file) {
+                                Ok(_) => (),
+                                Err(e) =>
+                                  println!("could not access metadata for test result {} at file {}.\nError {}",
+                                    r.test_summary_event.label,
+                                    f,
+                                    e)
+                              }
                             }
+                          }
+                          Err(e) =>
+                            println!("could not access metadata for test result {} at file {}.\nError {}",
+                              r.test_summary_event.label,
+                              f,
+                              e)
                         }
-                        bazelfe_protos::build_event_stream::file::File::Contents(_) => None,
-                    })
-                    .collect();
-
-                if is_failure {
-                    // Some failures don't get to the phase of writing junit output
-                    // this ensures we write something
-                    emit_backup_error_data(&r, &opt.junit_output_path);
-                }
-                for (idx, f) in files.into_iter().enumerate() {
-                    let output_file = output_folder.join(format!("test.{}.xml", idx));
-                    std::fs::copy(f, output_file).unwrap();
+                        }
+                        if is_failure {
+                            // Some failures don't get to the phase of writing junit output
+                            // this ensures we write something
+                            match emit_backup_error_data(&r, &opt.junit_output_path) {
+                                Ok(_) => (),
+                                Err(err) => failed_xml_writes.push((build_event, err)),
+                            }
+                        }
+                    }
+                    Err(e) => failed_xml_writes.push((build_event, e.into())),
                 }
             }
             HydratedInfo::Progress(_) => (),
@@ -136,32 +169,53 @@ async fn main() -> Result<(), Box<dyn Error>> {
         }
     }
 
-    if failed_actions.is_empty() && failed_tests.is_empty() && aborted_actions.is_empty() {
-        println!("Have zero failures, all successful.")
+    if failed_actions.is_empty()
+        && failed_tests.is_empty()
+        && aborted_actions.is_empty()
+        && failed_xml_writes.is_empty()
+    {
+        println!("Have zero failures, all successful.");
+        Ok(())
     } else {
         if !failed_actions.is_empty() {
             println!("Have {} failed actions", failed_actions.len());
-            for a in failed_actions.iter() {
+            for a in failed_actions {
                 println!("  - {}", a);
             }
         }
 
         if !failed_tests.is_empty() {
             println!("Have {} failed tests", failed_tests.len());
-            for a in failed_tests.iter() {
+            failed_tests.sort();
+            for a in failed_tests {
                 println!("  - {}", a);
             }
         }
 
         if !aborted_actions.is_empty() {
             println!("Have {} aborted actions", aborted_actions.len());
-            for a in aborted_actions.iter() {
+            aborted_actions.sort();
+            for a in aborted_actions {
+                println!("  - {}", a.unwrap_or_else(|| "Unknown".to_string()));
+            }
+        }
+
+        if !failed_xml_writes.is_empty() {
+            println!("Got {} xml write failures", failed_xml_writes.len());
+            failed_xml_writes.sort_by(|p1, p2| p1.0.label().cmp(&p2.0.label()));
+            for (r, err) in failed_xml_writes {
                 println!(
-                    "  - {}",
-                    a.to_owned().unwrap_or_else(|| "Unknown".to_string())
+                    "Target label = {} failed to write: {}",
+                    r.label().unwrap_or("<unknown>"),
+                    err
                 );
             }
         }
+
+        let err: Box<dyn Error> = Box::new(std::io::Error::new(
+            std::io::ErrorKind::Other,
+            "non-zero error count.",
+        ));
+        Err(err)
     }
-    Ok(())
 }