Skip to content

Conversation

@JTOne123
Copy link

CSProj files have been updated to enable SourceLink in your nuget

[This pull request was created with an automated workflow]

I noticed that your repository and Nuget package are important for our .NET community, but you still haven't enabled SourceLink.

We have to take 2 steps:

  1. Please approve this pull request and make .NET a better place for .NET developers and their debugging.
  2. Then just upload the .snupkg file to https://www.nuget.org/ (now you can find the snupkg file along with the .nuget file)

You can find more information about SourceLine at the following links
https://github.com/dotnet/sourcelink
https://www.hanselman.com/blog/ExploringNETCoresSourceLinkSteppingIntoTheSourceCodeOfNuGetPackagesYouDontOwn.aspx

If you are interesting about this automated workflow and how it works
https://github.com/JTOne123/GitHubMassUpdater

If you notice any flaws, please comment and I will try to make fixes manually

@stakx
Copy link
Member

stakx commented Jun 26, 2020

Hi again @JTOne123, I think it would be good to enable SourceLink. I'd like to leave the final decision to @jonorossi however.

If we decide to go ahead with SourceLink here, I think the following changes will be additionally needed:

  • We already build symbols packages (albeit not those of the .snupkg kind), see this <IncludeSymbols> in common.props. We should perhaps make sure that these MSBuild properties aren't spread across too many MSBuild project files. It appears to me that they belong exclusively in Castle.Core.csproj; we don't need debugging symbols for every project (such as unit test projects), after all. (Edited: Scratch that. The properties need to go in common.props because we generate some additional logging integration packages that should get the SourceLink treatment too.)

  • The README currently states (in the Releases section) where debugging symbols can be found. This paragraph will become obsolete and should be replaced with a hint about SourceLink (and ideally a short hint or doc link about how to enable it).

@stakx
Copy link
Member

stakx commented Jun 26, 2020

A small P.S. question @JTOne123: this page on docs.microsoft.com suggests enabling deterministic builds in conjunction with SourceLink. Do you know more about this? Should we do this here, too?

@jonorossi
Copy link
Member

@stakx I have no objection to adding SourceLink support, we did talk about it a while back but the situation with symbols was a bit all over the place.

I had a very quick look at this yesterday but didn't comment. In addition to your comments, I think the properties probably belong in the next PropertyGroup and the automated deployment to NuGet.org needs to be changed to publish these otherwise they won't get there.

@jonorossi jonorossi changed the title [PR] The proj files have been updated to enable SourceLink Enable SourceLink Jun 26, 2020
@JTOne123
Copy link
Author

Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

In addition to the review comments below, please also make the following changes:

  • *.snupkg must get published to NuGet, so you'll need to add a few additional commands to appveyor.yml here:

    Core/appveyor.yml

    Lines 67 to 70 in a7454e5

    nuget push ".\build\Castle.Core.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
    nuget push ".\build\Castle.Core-log4net.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
    nuget push ".\build\Castle.Core-NLog.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
    nuget push ".\build\Castle.Core-Serilog.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json

  • For completeness, you should perhaps add *.snupkg as artifacts at the end of appveyor.yml, something like this:

       artifacts:
       - path: build\*.nupkg
         name: core
    +  - path: build\*.snupkg
    +    name: symbols
  • Please update the paragraph in the Releases section of the README.md that explains where to get debugging symbols from / how to use them. For example:

    -Debugging symbols are available in symbol packages in the AppVeyor build artifacts since version 4.1.0. For example, [...]
    +SourceLink-enabled symbol packages (`.snupkg`) are published to NuGet. [This document](https://devblogs.microsoft.com/nuget/improved-package-debugging-experience-with-the-nuget-org-symbol-server/#:~:text=Consume+snupkg+from+NuGet) has instructions for configuring the Visual Studio debugger to use those. For versions 4.4.1 down to 4.1.0, symbol packages are available in the AppVeyor build artifacts. For example, [...]
    

Thanks!

Comment on lines +8 to +10
<PublishRepositoryUrl>true</PublishRepositoryUrl>
<IncludeSymbols>true</IncludeSymbols>
<SymbolPackageFormat>snupkg</SymbolPackageFormat>
Copy link
Member

Choose a reason for hiding this comment

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

Please move these properties to the following location (and make sure to use tabs for indentation, not spaces):

<IncludeSymbols>true</IncludeSymbols>

</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
Copy link
Member

Choose a reason for hiding this comment

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

Please move this <PackageReference> into this item group (and again, please use tabs for indentation):

<ItemGroup>
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" PrivateAssets="All" />
</ItemGroup>

@JTOne123 JTOne123 closed this Jul 25, 2020
@stakx stakx mentioned this pull request Dec 9, 2025
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants