Skip to content
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

Merged
merged 2 commits into from
May 23, 2022

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented May 20, 2022

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

  • Modify sourcelink to allow to generate this always.
  • Modify this to run in empty.csproj instead of runtime-prereqs.proj and add a p2p ref. That be invoked separately if any other part of the repo wants native sourcelink.
  • Revert to the old NoTargets behavior, but I don't think this is right...

Opening this PR to discuss if there's a better fix and have one that at least solves the issue.

@dotnet-issue-labeler
Copy link

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.

@ghost ghost assigned hoyosjs May 20, 2022
@hoyosjs
Copy link
Member Author

hoyosjs commented May 20, 2022

cc @jkotas

@ViktorHofer
Copy link
Member

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.

Comment on lines +19 to +21
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...
Copy link
Member

@ViktorHofer ViktorHofer May 20, 2022

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?

Copy link
Member Author

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>
@hoyosjs
Copy link
Member Author

hoyosjs commented May 20, 2022

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.

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.

@ViktorHofer
Copy link
Member

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.

@ViktorHofer ViktorHofer merged commit ff5840b into dotnet:main May 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2022
@hoyosjs hoyosjs deleted the juhoyosa/fix-no-targets-sourcelink branch July 2, 2022 06:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants