Add SourceLink to allow NuGet package debugging

[cremor]

Have you thought about adding SourceLink to NHiberante? That would make it easier to debug NHibernate in a project that references it.

As I see it, you'd only have to add the few things to the project file as explained in the readme of SourceLink. And you could then remove RepositoryUrl and RepositoryType from NHibernate.props since they are set automatically by SourceLink.

Related since the user will need to somehow get pdb files to allow SourceLink to work:

• The official Open-source library guidance about symbol packages
• NH-4067 - NuGet package doesn't include pdb file #1056

[fredericDelaporte]

SourceLink currently advise to use the new symbol package format as part of enabling SourceLink.
I have heard about that new format as build warnings in NHibernate prior to releasing 5.2, but at that point it seemed a bit early to do the move.

Now it may be time. But compared to the legacy symbol package that NHibernate currently publishes, it does not embed sources. (Well, this point should not be much an issue, since that is the point of SourceLink). And due to the way it is generated, this new symbol package cannot be generated along the legacy one in a single package generation: either it generates the legacy format, or the new format, but not both.

This is a bit troublesome since the new format requires at least Visual Studio 2017 15.9 for being consumed, while the legacy one can be consumed by much older VS.

I do not see why we should cease providing symbols for older VS, since NHibernate can be used with quite old ones.

Maybe can we publish both symbol formats, but that will require a "double" package generation: one for the legacy format, one for the new one.

[cremor]

But compared to the legacy symbol package that NHibernate currently publishes, it does not embed sources.

Until I read your message in #1056 I didn't know that NHibernate publishes a (now legacy) symbol package. With that knowledge, the current situation is far better already.
But as I said in #1056 I think this should be documented. This describes the current situation perfectly: Package Consuming Experience
Doesn't help, since the symbol packages since NHibernate 5.0 are not available. See my last comment in #1056

And due to the way it is generated, this new symbol package cannot be generated along the legacy one in a single package generation: either it generates the legacy format, or the new format, but not both.
This is a bit troublesome since the new format requires at least Visual Studio 2017 15.9 for being consumed, while the legacy one can be consumed by much older VS.

Are you sure that consumers need 15.9? I can only find documentation that says the package authors need 15.9 to create the new .snupkg files (here). For consumers it seems like there are no additional requirements in addition to the already existing requirements to consume portable PDBs (Visual Studio 2015 Update 2 and later) according to the NuGet wiki.
Ok, I've now found the official announcement and it indeed says that 15.9 is required to consume .snupkg files. That's not good.

And SourceLink itself (which is independent of the PDB format) works since Visual Studio 2017 15.3+ according to their readme.

[fredericDelaporte]

I was indeed not knowing about SourceLink and was confusing it with the new snupkg format recently introduced. I have edited my first comment for it to make more sense in regard to what you were actually writing about.

[cremor]

SourceLink does indeed now suggest to use .snupkg files. But that was changed just 22 days ago. As can be seen in the diff, they suggested to embed the pdb in the main NuGet package before.
Update: And now they updated their readme again to mention the limitations of .snupkg files and the alternative of directly embedding the pdb in the package.

Microsoft's open source library guidance still suggests to embed the pdb file directly in the main NuGet package (source). You can read the discussion about that here: dotnet/docs#9110

While that guidance only mentions the VS configuration part, the support for older VS versions that you mentioned is important too. edit: The guide now also mentiones the version requirement.

A pdb file directly embedded in the main NuGet package would allow everyone (at least everyone who can read portable pdbs, so VS 2015 Update 2 and later) to use the symbols without any additional VS configuration required. And with SourceLink anyone with VS 2017 15.3+ would be able to debug the source directly, again without additional configuration.

Of course that pdb file should then be as small as possible (without embedded source). But since SourceLink requires no configuration, the only consumers that would loose source debugging would be the ones in versions older than 2017 15.3 but newer than what is required to read source code from pdb files (couldn't find the required version for that now, but I assume it's nothing older than VS 2017). And of that group of consumers also only those that had https://nuget.smbsrc.net/ configured as a symbol source. I assume this is a very small target audience.
Update: As noted in #1056 it seems like symbol packages didn't even work since NHibernate 5.0, so no one will loose anything.

[fredericDelaporte]

I am fiddling a bit locally with this, and I think it cannot be done yet.

Sourcelink currently implies to depend on a pre-release package, which forbids to release a non-pre-release package.

[cremor]

I don't think that's true. Look at Json.NET, they reference Source Link but also released a non-pre-release package that contains the commit id, so has Source Link enabled. The Source Link dependency is not carried to the final package because of PrivateAssets="All". This is also mentioned in the Source Link readme:

Source Link package is a development dependency, which means it is only used during build. It is therefore recommended to set PrivateAssets to all on the package reference. This prevents consuming projects of your nuget package from attempting to install Source Link.

[fredericDelaporte]

Indeed, I was lacking PrivateAssets="All", having overlooked it.