Skip to content

Conversation

@vaind
Copy link
Collaborator

@vaind vaind commented Oct 18, 2023

closes #2692

This is mostly done in Sentry.targets - I've merged the two jobs (one uploading debug files, the second one uploading sources) because it was getting unwieldy when trying to handle both native AOT and managed symbols (and referenced sources) in both jobs.

I've also had to rework the PrepareCLI workflow a bit so that we get consistent integration test runs. Ppreviously, the integration test build would also upload symbols for the Sentry project itself, which is not what would happen when building actual user projects, because that project would have come from a nuget package.

TODO:

  • force some source bundles to be uploaded for managed apps? it seems all sources are embedded with .net 8 SDK... How can we do it though? I'm open to suggestions
  • test on Linux
  • test on macOS (although the actual native integration will only come as a part of macOS native SDK integration #2625)

#skip-changelog (This is part of a bigger task to support Native AOT)

@vaind vaind changed the base branch from main to feat/4.0.0 October 18, 2023 20:47
@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2023

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

Generated by 🚫 dangerJS against c9e7edf

@vaind vaind force-pushed the feat/native-image-upload branch from 3be6fb9 to 90d4ff9 Compare October 19, 2023 18:39
Comment on lines -53 to -56
<!-- If running in CI and auth token is not set, disable SentryCLI completely (to avoid logging warnings). -->
<PropertyGroup Condition="'$(GITHUB_ACTIONS)' == 'true' And '$(SENTRY_AUTH_TOKEN)' == ''">
<UseSentryCLI>false</UseSentryCLI>
</PropertyGroup>
Copy link
Collaborator Author

@vaind vaind Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This (and other similar cases below) is now handled by a condition in Sentry.targets property UseSentryCLI

@vaind vaind marked this pull request as ready for review October 20, 2023 19:27
<!-- Native AOT publishing
While '_IsPublishing' looks a bit "private", it's also OK if it suddenly stops working and this step runs anyway.
See https://github.com/dotnet/sdk/issues/26324#issuecomment-1169236993 -->
<PropertyGroup Condition="'$(PublishDir)' != '' and '$(PublishAot)' == 'true' and '$(_IsPublishing)' == 'true'">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we already have a CI_PUBLISHING_BUILD property... I take it this doesn't work because it's only ever set by by our github actions (and we want this to work even in the context of builds on local development machines)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a transitive build target, so one that gets triggered during lib-consumer builds.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha... cunning. I had no idea we were doing that (or that it was even possible - never had a need before). Is that enabled simply by this?

<!--
Include Sentry's custom targets file in the nuget package.
This file contains targets that are invoked during the end-user's build.
The same file is included twice, so it ends up being used for both direct and transitive package references to Sentry.
-->
<ItemGroup>
<None Include="buildTransitive\Sentry.targets" Pack="true" PackagePath="buildTransitive\Sentry.targets" />
<None Include="buildTransitive\Sentry.targets" Pack="true" PackagePath="build\Sentry.targets" />
</ItemGroup>

I think that's all we need to do right (reading the docs).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I've learned about this a couple of weeks ago myself, for the same reason :)

@bitsandfoxes bitsandfoxes self-assigned this Oct 23, 2023
<Target Name="UploadDebugInfoToSentry" AfterTargets="$(SentryCLIUploadAfterTargets)" DependsOnTargets="PrepareSentryCLI"
Condition="'$(SentryCLI)' != '' and ('$(SentryUploadSymbols)' == 'true' or '$(SentryUploadSources)' == 'true')">

<!-- if (UploadSymbols && UploadSources) { -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah they're nice although at one point I've found myself just copying a piece of a code from one if branch to the other, forgetting to actually update the relevant condition 😅

Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@bitsandfoxes bitsandfoxes merged commit 91b22a9 into feat/4.0.0 Oct 23, 2023
@bitsandfoxes bitsandfoxes deleted the feat/native-image-upload branch October 23, 2023 12:26
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.

Native AOT - upload debug files and sources

4 participants