From c343fb849aa9ab510ec42fb2f15f07fc812930be Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 30 Aug 2024 14:07:48 +0200 Subject: [PATCH 1/4] feat(sourcebundle): Add callback to handle skipped files Add a callback to SourceBundleWriter that is called every time we skip adding a file to the bundle due to a ReadFailed error. Closes #863 --- symbolic-debuginfo/src/sourcebundle.rs | 75 +++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/symbolic-debuginfo/src/sourcebundle.rs b/symbolic-debuginfo/src/sourcebundle.rs index 0e00774f..25a65f4c 100644 --- a/symbolic-debuginfo/src/sourcebundle.rs +++ b/symbolic-debuginfo/src/sourcebundle.rs @@ -46,6 +46,7 @@ use std::borrow::Cow; use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::error::Error; use std::fmt; +use std::fmt::{Display, Formatter}; use std::fs::{File, OpenOptions}; use std::io::{BufReader, BufWriter, Read, Seek, Write}; use std::path::Path; @@ -1035,6 +1036,35 @@ fn sanitize_bundle_path(path: &str) -> String { sanitized } +/// Contains information about a file skipped in the SourceBundleWriter +#[derive(Debug)] +pub struct SkippedFileInfo<'a> { + path: &'a str, + reason: &'a str, +} + +impl<'a> SkippedFileInfo<'a> { + fn new(path: &'a str, reason: &'a str) -> Self { + Self { path, reason } + } + + /// Get the path of the skipped file + pub fn path(&self) -> &str { + self.path + } + + /// Get the human-readable reason why the file was skipped + pub fn reason(&self) -> &str { + self.reason + } +} + +impl Display for SkippedFileInfo<'_> { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "Skipped file {} due to: {}", self.path, self.reason) + } +} + /// Writer to create [`SourceBundles`]. /// /// Writers can either [create a new file] or be created from an [existing file]. Then, use @@ -1070,6 +1100,7 @@ where manifest: SourceBundleManifest, writer: ZipWriter, collect_il2cpp: bool, + skipped_file_callback: Box, } fn default_file_options() -> SimpleFileOptions { @@ -1097,6 +1128,7 @@ where manifest: SourceBundleManifest::new(), writer: ZipWriter::new(writer), collect_il2cpp: false, + skipped_file_callback: Box::new(|_| ()), }) } @@ -1212,6 +1244,42 @@ where Ok(()) } + /// Calls add_file, and handles any ReadFailed errors by calling the skipped_file_callback. + fn add_file_skip_read_failed( + &mut self, + path: S, + file: R, + info: SourceFileInfo, + ) -> Result<(), SourceBundleError> + where + S: AsRef, + R: Read, + { + let result = self.add_file(&path, file, info); + + if let Err(e) = &result { + if e.kind == SourceBundleErrorKind::ReadFailed { + let reason = e.to_string(); + let skipped_info = SkippedFileInfo::new(path.as_ref(), &reason); + (self.skipped_file_callback)(skipped_info); + + return Ok(()); + } + } + + result + } + + /// Set a callback, which is called for every file that is skipped from being included in the + /// source bundle. The callback receives information about the file being skipped. + pub fn with_skipped_file_callback( + mut self, + callback: impl FnMut(SkippedFileInfo) + 'static, + ) -> Self { + self.skipped_file_callback = Box::new(callback); + self + } + /// Writes a single object into the bundle. /// /// Returns `Ok(true)` if any source files were added to the bundle, or `Ok(false)` if no @@ -1314,12 +1382,7 @@ where info.set_ty(SourceFileType::Source); info.set_path(filename.clone()); - if let Err(e) = self.add_file(bundle_path, source, info) { - // Skip sources that are not UTF-8 - if e.kind != SourceBundleErrorKind::ReadFailed { - return Err(e); - }; - } + self.add_file_skip_read_failed(bundle_path, source, info)? } } From 676940df234fee714ea7ee2f20d0f27beb91f786 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 30 Aug 2024 14:11:05 +0200 Subject: [PATCH 2/4] fix(sourcebundle): Skip all invalid sources #861 missed another spot where the ReadFailed error can cause the write function to fail; this commit fixes that. Fixes #860 --- symbolic-debuginfo/src/sourcebundle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/symbolic-debuginfo/src/sourcebundle.rs b/symbolic-debuginfo/src/sourcebundle.rs index 25a65f4c..07a30bc5 100644 --- a/symbolic-debuginfo/src/sourcebundle.rs +++ b/symbolic-debuginfo/src/sourcebundle.rs @@ -1365,7 +1365,7 @@ where collect_il2cpp_sources(&source, &mut referenced_files); } - self.add_file(bundle_path, source.as_slice(), info)?; + self.add_file_skip_read_failed(bundle_path, source.as_slice(), info)?; } files_handled.insert(filename); From d3480552074de9a12223f9b2af17f86e3113b58c Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Fri, 30 Aug 2024 14:20:32 +0200 Subject: [PATCH 3/4] meta: Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e442c455..13e0a585 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Add callback to handle skipped files ([#864](https://github.com/getsentry/symbolic/pull/864)) + ## 12.10.1 - Skip invalid sources ([#861](https://github.com/getsentry/symbolic/pull/861)) From 6fb5a49bbaf9137c6b38bdcb7f28156ff5a66d96 Mon Sep 17 00:00:00 2001 From: Daniel Szoke Date: Wed, 4 Sep 2024 13:42:14 +0200 Subject: [PATCH 4/4] apply suggestions from code review Co-authored-by: Jan Michael Auer --- CHANGELOG.md | 2 +- symbolic-debuginfo/src/sourcebundle.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13e0a585..7b001e04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- Add callback to handle skipped files ([#864](https://github.com/getsentry/symbolic/pull/864)) +- Add callback to `symbolic::debuginfo::sourcebundle::SourceBundleWriter` which handles files skipped while writing to the source bundle. ([#864](https://github.com/getsentry/symbolic/pull/864)) ## 12.10.1 diff --git a/symbolic-debuginfo/src/sourcebundle.rs b/symbolic-debuginfo/src/sourcebundle.rs index 07a30bc5..4ab5d05a 100644 --- a/symbolic-debuginfo/src/sourcebundle.rs +++ b/symbolic-debuginfo/src/sourcebundle.rs @@ -1048,7 +1048,7 @@ impl<'a> SkippedFileInfo<'a> { Self { path, reason } } - /// Get the path of the skipped file + /// Returns the path of the skipped file. pub fn path(&self) -> &str { self.path }