Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sourcebundles): Skip invalid sources #861

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Aug 9, 2024

Fixes #860

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/fix-sourcebundle branch 2 times, most recently from 186ea0a to 639fcc3 Compare August 9, 2024 14:07
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/fix-sourcebundle branch from 639fcc3 to 04d9e2a Compare August 9, 2024 14:14
@p0358
Copy link

p0358 commented Aug 9, 2024

Hmm, it would be nice to note the file being skipped at least in the debug logs. I fear I might have some leftover source files that might actually have some singular improper UTF-8 inserted due to copy-paste, as was the case a single time in the past, and then such a .cpp file would be silently skipped without any way of ever knowing...

@Swatinem
Copy link
Member

Unfortunately, we do not really have a good story around surfacing these errors as warnings, but I do agree that silently skipping these files might not be the best solution here.

@p0358
Copy link

p0358 commented Aug 19, 2024

I can actually imagine why putting them as warnings wouldn't be desirable (potential huge spam). But in opt-in debug-level log why not? I'm not sure if there's really any other convenient way to figure out which files are causing these problems, and I'm curious if it just picks up something else, or if there's some problem in one of actual source code files (we had a situation like that before where compiler would yield a warning about invalid UTF-8 sequence copy-pasted somewhere, which is why I'm so curious...).

@loewenheim loewenheim self-assigned this Aug 19, 2024
@szokeasaurusrex
Copy link
Member Author

@loewenheim @Swatinem would it perhaps be possible to return a list of all the files we had to skip over due to encoding failures? Then, the calling code (in this case, Sentry CLI) could decide how to handle these (warn, error, etc).

@Swatinem
Copy link
Member

I think that sounds reasonable, but I believe that also requires breaking changes in symbolic.

@szokeasaurusrex
Copy link
Member Author

Yeah, that was my concern. We could always introduce a new variant of write_object_with_filter with a change signature that supports such a return value, then keep the original write_object_with_filter around with its current signature for backwards compatibility. Not sure this is the best path though

@loewenheim
Copy link
Contributor

I would go with that option then, yeah.

@szokeasaurusrex szokeasaurusrex merged commit b9c8405 into master Aug 28, 2024
13 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/fix-sourcebundle branch August 28, 2024 12:13
@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Aug 28, 2024

@p0358 we are going to work on getting this logging to work in separate issues: #863 and getsentry/sentry-cli#2135

szokeasaurusrex added a commit that referenced this pull request Aug 30, 2024
#861 missed another spot where the ReadFailed error can cause the write function to fail; this commit fixes that.

Fixes #860
szokeasaurusrex added a commit that referenced this pull request Sep 4, 2024
* 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

* 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

* meta: Update changelog

* apply suggestions from code review

Co-authored-by: Jan Michael Auer <jan.auer@sentry.io>

---------

Co-authored-by: Jan Michael Auer <jan.auer@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading debug information files fails in v2.33.1
4 participants