Skip to content

add SourceLink support #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 15, 2018
Merged

Conversation

shupoval
Copy link

@shupoval shupoval commented Nov 3, 2018

added support of SourceLink in order to enable better debugging experience. For more info regarding SourceLink please refer to the official repo here

@nblumhardt
Copy link
Member

Thanks for the PR! Will the beta package reference prevent a stable version of this package being published, or is PrivateAssets="All" enough to get this through?

@danjagnow
Copy link

SourceLink is marked with <DevelopmentDependency>true</DevelopmentDependency>, so I believe it won't show up as a transitive dependency for Serilog.

@shupoval
Copy link
Author

I'm sorry for the delayed answer.

According to controlling dependency assets and using sourcelink PrivateAssets="All" determines that all SourceLink's assets will be consumed by this package only ("Serilog.Sinks.File" itself) and won't flow to the parent project (project that will reference the "Serilog.Sinks.File").

Hence referencing of a beta package shouldn't prevent a stable version of this package being published.

@nblumhardt
Copy link
Member

Great! Thanks for the replies. Since we'd ideally do this consistently throughout the Serilog org, it seems getting it spot-on here is worthwhile :-)

One last question, in single-published-package repos like the Serilog ones, is there any reason not to include the SourceLink task/references in the CSPROJ files themselves, rather than in a PROPS file? Thanks!

@shupoval
Copy link
Author

@nblumhardt, you are probably referring to the following PR.

In that PR SourceLink v2 was added. For now, it is being considered as obsolete and SourceLink v3 (which is being added in this PR) would evolve further. For more details please take a look at SourceLink and .NET Foundation.

Probably I'd also need to create a PR with this updated version of SourceLink into that repo as well (and other kinds of serilog sinks repos :) heh, a lot of work to do but I hope I'll be able to do it).

Regarding your last question about props vs *PROJ it is a really good question. I'd agree that in single-published-package repos, it probably doesn't give any benefit.
Please let me know if you'd prefer these changes were in PROJ file instead of Directory.Build.props.

Thank you!

@nblumhardt
Copy link
Member

Thanks for the reply and details! Yes, I'd forgotten about the original serilog/serilog one; seems like v3 is the way to go, anyway :-)

For consistency, since we don't split e.g. NuGet packaging etc. out into PROPS files, rolling it into the CSPROJ file seems like the way to go 👍

@nblumhardt
Copy link
Member

Awesome, thanks! Let's give this a shot 👍

@nblumhardt nblumhardt merged commit d2856bf into serilog:dev Nov 15, 2018
@nblumhardt nblumhardt mentioned this pull request Oct 17, 2019
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.

3 participants