feat: Allow uploading sources for debug files #773

Swatinem · 2022-06-01T15:28:07Z

No description provided.

github-actions · 2022-06-01T15:28:53Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 80158ab

vaind

Nice!

should we also add tests? We already test that the binaries are being uploaded for all supported platforms: #763

To do that we would likely need to

enable the new option in the integration test project (...-update-sentry.ps1 script)
maybe update the symbol-upload-server.py - if the source-code upload uses a new API
update common.ps1 CheckSymbolServerOutput() with to check that the right files were uploaded.

Swatinem · 2022-06-01T16:05:01Z

I think the source bundles are the same as other debug files in that sense. I can bug @bitsandfoxes to help me set up the integration tests so I can get something up and running.

vaind · 2022-06-01T16:09:25Z

I think the source bundles are the same as other debug files in that sense. I can bug @bitsandfoxes to help me set up the integration tests so I can get something up and running.

I'll give it a quick go myself, since I've done the symbol upload tests I know these pieces by heart

vaind · 2022-06-01T19:45:46Z

Hmm, the sources don't seem to be easily distinguishable from binary files. Here's the diff of uploading the same directory (windows desktop build), with upload-sources enabled in one case. I deduce GameAssembly.pdb are being uploaded...

Swatinem · 2022-06-02T10:01:00Z

In sentry, all these files have the same name, with the same debug_id. But they have different contents, like here:

So I think this is perfectly fine. Although I agree its confusing. We do use .src.zip in some places, although its not a valid zip file either because it has its own header prepended before the actual zip contents.

Swatinem · 2022-06-02T10:04:58Z

One thing to watch out for. Some time ago we had the problem that source bundles were being re-uploaded all the time, because they were different all the time because of unstable sorting order internally. Now we should have a stable sorting order. But it seems that at least my local sentry-cli still re-uploads these files over and over again, so that bug is not fixed yet :-(

The reason I say this is because you would probably end up with different chunk hashes, and thus wouldn’t be able to use snapshot testing for this.

vaind · 2022-06-02T10:14:47Z

One thing to watch out for. Some time ago we had the problem that source bundles were being re-uploaded all the time, because they were different all the time because of unstable sorting order internally. Now we should have a stable sorting order. But it seems that at least my local sentry-cli still re-uploads these files over and over again, so that bug is not fixed yet :-(

The reason I say this is because you would probably end up with different chunk hashes, and thus wouldn’t be able to use snapshot testing for this.

So far the symbol upload tests didn't use the hash, just the ~~content~~ file name... not sure what to do about the source upload yet, I may need to change the test-server response so that the CLI actually uploads the content so that I know what came in. Right now the server always responds with a message indicating all chunks are already uploaded.

Swatinem · 2022-06-02T10:17:49Z

Isn’t that a bit contradictory? Using the content, but then the contents are not actually uploaded because the server says chunks already exist?

Btw, this was the issue with source bundles: getsentry/sentry-cli#831 The problem is rather timestamps -_- So maybe there is a fairly easy workaround for this in symbolic.

vaind · 2022-06-02T10:28:28Z

sorry, brain lag there - I was going to say "file name" instead of "content", I've fixed the comment.

vaind · 2022-06-07T12:45:55Z

I've added the tests (took way more effort than anticipated... as usual), though only based on the number of uploaded files as I couldn't (in a self-imposed time box) figure out unpacking & analyzing the content of the uploaded files. Should be still good to catch issues in the future: 21cb45b

bitsandfoxes · 2022-06-07T13:18:33Z

src/Sentry.Unity.Editor.iOS/SentryXcodeProject.cs


 process_upload_symbols() {{
-    ./{SentryCli.SentryCliMacOS} --log-level=info upload-dif $BUILT_PRODUCTS_DIR 2>&1 | tee ./sentry-symbols-upload.log
+    ./{0} --log-level=info upload-dif {1} $BUILT_PRODUCTS_DIR 2>&1 | tee ./sentry-symbols-upload.log


That's a nifty way of getting some additional arguments in there!

src/Sentry.Unity.Editor/Android/DebugSymbolUpload.cs

test/Scripts.Integration.Test/common.ps1

Swatinem · 2022-06-07T13:35:38Z

❤️ thanks so much for taking this over! @vaind

bitsandfoxes · 2022-06-07T13:42:29Z

test/Sentry.Unity.Editor.Tests/Android/DebugSymbolUploadTests.cs

@@ -35,7 +35,7 @@ public Fixture()
            }

            public DebugSymbolUpload GetSut() => new(new UnityLogger(new SentryOptions(), LoggerInterceptor),
-                UnityProjectPath, GradleProjectPath, IsExporting, Application);
+                null, UnityProjectPath, GradleProjectPath, IsExporting, Application);


Should we add unit tests for the path that adds --include-sources?

bitsandfoxes · 2022-06-07T13:43:13Z

test/Sentry.Unity.Editor.iOS.Tests/SentryXcodeProjectTests.cs

@@ -129,7 +129,7 @@ public void AddBuildPhaseSymbolUpload_CleanXcodeProject_BuildPhaseSymbolUploadAd
            xcodeProject.ReadFromProjectFile();

            var didContainUploadPhase = xcodeProject.MainTargetContainsSymbolUploadBuildPhase();
-            xcodeProject.AddBuildPhaseSymbolUpload(_fixture.Options.DiagnosticLogger);
+            xcodeProject.AddBuildPhaseSymbolUpload(_fixture.Options.DiagnosticLogger, new SentryCliOptions());


Since uploadSources is false by default, should we add unit tests for the path that adds --include-sources?

bruno-garcia

Other than missing tests LGTM

src/Sentry.Unity.Editor/ConfigurationWindow/DebugSymbolsTab.cs

Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io> Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

vaind · 2022-06-07T18:23:11Z

merging as is to get the changes for #785 , I'll add unit tests & update CI to run alternatively with/without sources in a follow up PR

Swatinem requested review from bruno-garcia, bitsandfoxes and vaind as code owners June 1, 2022 15:28

vaind reviewed Jun 1, 2022

View reviewed changes

vaind force-pushed the feat/include-sources branch 17 times, most recently from 0531955 to a9fc9d8 Compare June 6, 2022 19:01

Swatinem and others added 4 commits June 7, 2022 09:09

feat: Allow uploading sources for debug files

5a1c8ca

add changelog

b28a8b0

chore: symbol upload test - check count

e60ad94

chore: enable debug log in integration-test build

4e7ae57

vaind force-pushed the feat/include-sources branch from a9fc9d8 to 6165918 Compare June 7, 2022 07:11

test: symbol upload with sources

081b5c9

vaind force-pushed the feat/include-sources branch from 6165918 to 6c034ad Compare June 7, 2022 09:19

vaind mentioned this pull request Jun 7, 2022

Integration-test building without sentry SDK only on main branch #785

Merged

test: symbol upload with sources

21cb45b

vaind force-pushed the feat/include-sources branch from 6c034ad to 21cb45b Compare June 7, 2022 10:46

bitsandfoxes reviewed Jun 7, 2022

View reviewed changes

bruno-garcia approved these changes Jun 7, 2022

View reviewed changes

src/Sentry.Unity.Editor/ConfigurationWindow/DebugSymbolsTab.cs Outdated Show resolved Hide resolved

bitsandfoxes approved these changes Jun 7, 2022

View reviewed changes

Apply suggestions from code review

80158ab

Co-authored-by: Stefan Jandl <stefan.jandl@sentry.io> Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

vaind merged commit 318529d into main Jun 7, 2022

vaind deleted the feat/include-sources branch June 7, 2022 18:23

vaind mentioned this pull request Jun 8, 2022

Followup for #773 - update tests for the include-sources option #789

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow uploading sources for debug files #773

feat: Allow uploading sources for debug files #773

Swatinem commented Jun 1, 2022

github-actions bot commented Jun 1, 2022 •

edited

Loading

vaind left a comment •

edited

Loading

Swatinem commented Jun 1, 2022

vaind commented Jun 1, 2022

vaind commented Jun 1, 2022

Swatinem commented Jun 2, 2022

Swatinem commented Jun 2, 2022

vaind commented Jun 2, 2022 •

edited

Loading

Swatinem commented Jun 2, 2022

vaind commented Jun 2, 2022

vaind commented Jun 7, 2022

bitsandfoxes Jun 7, 2022

Swatinem commented Jun 7, 2022

bitsandfoxes Jun 7, 2022

bitsandfoxes Jun 7, 2022

bruno-garcia left a comment

vaind commented Jun 7, 2022

feat: Allow uploading sources for debug files #773

feat: Allow uploading sources for debug files #773

Conversation

Swatinem commented Jun 1, 2022

github-actions bot commented Jun 1, 2022 • edited Loading

vaind left a comment • edited Loading

Choose a reason for hiding this comment

Swatinem commented Jun 1, 2022

vaind commented Jun 1, 2022

vaind commented Jun 1, 2022

Swatinem commented Jun 2, 2022

Swatinem commented Jun 2, 2022

vaind commented Jun 2, 2022 • edited Loading

Swatinem commented Jun 2, 2022

vaind commented Jun 2, 2022

vaind commented Jun 7, 2022

bitsandfoxes Jun 7, 2022

Choose a reason for hiding this comment

Swatinem commented Jun 7, 2022

bitsandfoxes Jun 7, 2022

Choose a reason for hiding this comment

bitsandfoxes Jun 7, 2022

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

vaind commented Jun 7, 2022

github-actions bot commented Jun 1, 2022 •

edited

Loading

vaind left a comment •

edited

Loading

vaind commented Jun 2, 2022 •

edited

Loading