-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix native sourcelink support #69598
Fix native sourcelink support #69598
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
cc @jkotas |
For such cases, the NoTargets SDK has an extra hook point to import logic after the Sdk.targets file comes into play: https://github.com/microsoft/MSBuildSdks/blob/9af7ea2c0d302af50ec2cc2566c7f4e32325ec3f/src/NoTargets/Sdk/Sdk.targets#L87. You can set a property that points to a targets file that then sets the required properties. |
This is relatively ugly. The NoTargets SDK sets DebugType=None, but that makes it such that the sourcelink targets | ||
don't run, and we wouldn't generate the sourcelink file for native compilation. It would be better if we could call | ||
the target directly and have it generate the file, but it's guarded by this property anyway... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider submitting a change to https://github.com/dotnet/sourcelink to allow overriding this setting via a custom property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Creating a file for that one property seems like overkill, unless there's a strong preference, I am going to leave as it - It's localized next to the target invocation so it makes discoverability a bit easier IMO. |
Sure that works as well. Just wanted to let you know that such a hook exists. |
microsoft/MSBuildSdks#360 sets DebugType=none in a non-overrideable manner. The problem is
that property gates the sourcelink targets from running. While this is not very clean, the other
alternatives are not all that much better
Opening this PR to discuss if there's a better fix and have one that at least solves the issue.