Skip to content

📦 Include symbols (snupkg's) when creating nuget packages.#552

Merged
bruno-garcia merged 8 commits into
getsentry:mainfrom
PureKrome:530/snupkg
Oct 18, 2020
Merged

📦 Include symbols (snupkg's) when creating nuget packages.#552
bruno-garcia merged 8 commits into
getsentry:mainfrom
PureKrome:530/snupkg

Conversation

@PureKrome
Copy link
Copy Markdown
Contributor

Fixes #530

While reviewing the code, I've noticed that the CI system is using something called Zeus for release management. So i'm not familiar with that project (sponsored by Sentry, btw 👏🏻 ). As such, I'm guessing certain steps and unable to do a full end-to-end test, so I'm not to sure how successful this PR will be/get.

Basic idea I've done:

  • dotnet pack includes symbols
  • push nupkg (currently the status quo) and now also the snupkg up to zeus 'artifacts'
  • fix up the build.props to have the default suggested source-link settings.

I can't see if the snupkg's do end up getting copied to Zeus. Also, I can't see how to 'release' both if they are up in Zeus.

@PureKrome PureKrome changed the title 📦 Include symbols (snupkg's) when creating nuget packages. 📦 Include symbols (snupkg's) when creating nuget packages. Oct 16, 2020
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 16, 2020

Codecov Report

Merging #552 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #552   +/-   ##
=======================================
  Coverage   85.18%   85.18%           
=======================================
  Files         137      137           
  Lines        3322     3322           
  Branches      749      749           
=======================================
  Hits         2830     2830           
  Misses        294      294           
  Partials      198      198           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fce1c1f...4d94a50. Read the comment docs.

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this one too @PureKrome
Left a couple notes

Comment thread CHANGELOG.md Outdated
Comment thread src/Directory.Build.props
<!-- Optional: Publish the repository URL in the built .nupkg (in the NuSpec <Repository> element) -->
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<!-- Optional: Embed source files that are not tracked by the source control manager in the PDB -->
<EmbedUntrackedSources>true</EmbedUntrackedSources>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What would such files be? This was left out I believe because I didn't understand the value (or couldn't verify it would add value) when I originally added it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a new comment in the original issue #530 with some screen shots about this.

If I set that to true or false, it still says 'untracked'. Hmmm.

When i add the ContinuousIntegrationBuild value in .. i can get better results.

ContinuousIntegrationBuildadded + false

image

ContinuousIntegrationBuild added + true

image

but if i don't add the ContinuousIntegrationBuild value...

image


TL;DR;

To get NuGet package explorer to be 'happy', I need:

  • ✔️ <EmbedUntrackedSources>true</EmbedUntrackedSources>
  • ✔️ <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>

I'm honestly not too sure what these are doing and how they make things better/worse. Or in other words, what effect does it have when i DO NOT include them? Sure i know NuGet Package Explorer cries .. but what is happening behind the scenes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got some info on the CIB setting.

This repo shows how to get a fully deterministic build using Source Link. Deterministic builds are important as they enable verification that the resulting binary was built from the specified source and provides traceability.

Deterministic builds require a property to be set to true during CI: ContinuousIntegrationBuild. These should not be enabled during local dev or the debugger won't be able to find the local source files.

So i'll try and make sure the CIB setting is only set via the CLI (because we're using various CI environments right now)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, -FINALLY- stumbled across the docs about the settings.

Set EmbedUntrackedSources to true to instruct the build system to embed project source files that are not tracked by the source control or imported from a source package to the generated PDB.

If the project generates additional source files that are added to Compile item group in a custom target, this target must run before BeforeCompile target (specify BeforeTargets="BeforeCompile"). Otherwise, these additional source files will not be automatically embedded into the PDB.

Again, I don't really understand what this means with respect to Sentry / Sentry's project system :(

I'm going to leave it IN .. as Nuget Package Manager was having a cry, if it was false.

Comment thread src/Directory.Build.props
Comment thread src/Directory.Build.props

<!-- Prop required by Microsoft.SourceLink.GitHub -->
<!-- Optional: Publish the repository URL in the built .nupkg (in the NuSpec <Repository> element) -->
<PublishRepositoryUrl>true</PublishRepositoryUrl>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this true by default? I wonder because sourcelink already worked on this repo

Copy link
Copy Markdown
Contributor Author

@PureKrome PureKrome Oct 18, 2020

Choose a reason for hiding this comment

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

Another great question. I found the docs on this: (Emphasis, mine)

The URL of the repository supplied by the CI server or retrieved from source control manager is stored in PrivateRepositoryUrl variable.

This value is not directly embedded in build outputs to avoid inadvertently publishing links to private repositories. Instead, PublishRepositoryUrl needs to be set by the project in order to publish the URL into RepositoryUrl property, which is used e.g. in the nuspec file generated for NuGet package produced by the project.

To me, this suggests that the default is FALSE .. but if that's the case ... how does this work?

I've removed it (locally) and generated a new package .. and .. it lists the url in there!

I .. don't get it :(

I think this value is different to <RepositoryUrl>https://github.com/getsentry/sentry-dotnet</RepositoryUrl> which already exists.
~~
So maybe:
- <RepositoryUrl> can be anything, which is meta data for someone to 'know' where the repo is if they want to manually look at code
- <PublishRepositoryUrl> maybe related to how the symbol packages are known where to exist?

I'm thinking, based on the docs, I should put it in there .. mainly because this is a PUBLIC repo.

EDIT: Thinking more on this, I think the above is maybe wrong.

It's an order of importance issue. (I just tested this locally)

  • if <RepositoryUrl> exists, use this.
  • else if <PublishRepositoryUrl> == true, then auto set <RepositoryUrl>
  • else no value for <RepositoryUrl>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, asked a question in the sourcelink repo.

@PureKrome
Copy link
Copy Markdown
Contributor Author

Ta for the feedback, I'll review it later on after i wake up. (nearly 2am here).

here's some results from the draft PR CI/CD..

Okay - so CI passes with some good results:

snupkg's are getting built:
image

nupkg's and snupkg's are also getting collected as artifacts:
image

... and getting uploaded into the artifact storage:
image

and finally, uploaded to Zeus (if the zeus hook variable thingy was set ... which I have no idea why it was not .. i'm guessing because the branch was not main or release/* ?
image

So I hope this helps?

@bruno-garcia
Copy link
Copy Markdown
Member

All looking great @PureKrome
My notes are just questions, could lift this from draft already eh?
Thanks again for your help and sleep tight 🛌 🇦🇺

@PureKrome PureKrome marked this pull request as ready for review October 18, 2020 12:05
Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough analysis. Let's add it in!

@bruno-garcia bruno-garcia merged commit dc203d9 into getsentry:main Oct 18, 2020
@PureKrome
Copy link
Copy Markdown
Contributor Author

Yay! Thanks @bruno-garcia.

One last request: could this get the hacktoberfest-accepted label added, please sir?

@PureKrome PureKrome deleted the 530/snupkg branch October 18, 2020 22:26
@bruno-garcia
Copy link
Copy Markdown
Member

Sorry I forgot @PureKrome

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.

Publish snupkg to nuget.org

3 participants