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

Add support for C++ libs into sourcelink #605

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nickdalt
Copy link

@nickdalt nickdalt commented Apr 19, 2020

Modify Microsoft.SourceLink.Common as follows:-

  • Targets files generates sourcelink file only for C++ static library projects
  • Targets file has new target to merge the sourcelink files associated with each lib into the master sourcelink file. Only executed for dll/exe C++ projects
  • New task to perform the merge of sourcelink files

Makes the assumption that the for a C++ static lib the sourcelink file will be in the same directory with the extension sourcelink.json

Possible additional tasks for maintainer:-

  • I am not that familiar with MSBuild so I may be using the wrong properties, conditions etc
  • Localisation of log messages for task

See also issue #580

@dnfclas
Copy link

dnfclas commented Apr 19, 2020

CLA assistant check
All CLA requirements met.

@tmat
Copy link
Member

tmat commented Apr 25, 2020

Thanks for the PR.

I'm not sure there is much benefit in merging Source Link files. VC++ linker supports multiple /sourcelink entries: /link /sourcelink <sourcelinkfile1.json> /sourcelink <sourcelinkfile2.json>. If multiple are specified the linker creates named streams called sourcelink$1, sourcelink$2, etc. and the debugger recognizes these.

This was specifically designed to remove complexity of merging JSON from build systems such as CMake.

…, multiple uses of the link /sourcelink arg are used.
@nickdalt
Copy link
Author

OK, I hadn't realised that /sourcelink could be used multiple times. I have updated the code to used /sourcelink with each discovered .sourcelink.json file.

Base automatically changed from master to main March 17, 2021 22:33
<!--
Add compiler targets: C++ generates sourcelink file only for static libs.
-->
<PropertyGroup Condition="'$(Language)' == 'C++' and $(ConfigurationType) == 'StaticLibrary'">
Copy link
Member

@tmat tmat Aug 12, 2021

Choose a reason for hiding this comment

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

$(ConfigurationType)

Missing ' ' around $(ConfigurationType).

Copy link
Author

Choose a reason for hiding this comment

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

Done

<!--
Add compiler targets: C++ generates PDB with SourceLink in Link phase.
-->
<PropertyGroup Condition="'$(Language)' == 'C++'">
<PropertyGroup Condition="'$(Language)' == 'C++' and $(ConfigurationType) != 'StaticLibrary'">
Copy link
Member

@tmat tmat Aug 12, 2021

Choose a reason for hiding this comment

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

$(ConfigurationType)

Missing ' ' around $(ConfigurationType).

Copy link
Author

Choose a reason for hiding this comment

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

Done

<!-- C++ Link task currently doesn't recognize SourceLink property only add this for non-static libs since lib doesn't
understand /sourcelink
-->
<ItemGroup Condition="'$(Language)' == 'C++' and $(ConfigurationType) != 'StaticLibrary'">
Copy link
Member

Choose a reason for hiding this comment

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

$(ConfigurationType)

Missing ' ' around $(ConfigurationType).

Copy link
Author

Choose a reason for hiding this comment

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

Done


try
{
//// Throughout we expect that the sourcelink files for a lib is alongside
Copy link
Member

Choose a reason for hiding this comment

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

s

typo: file

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

<Link Update="@(Link)">
<AdditionalOptions>%(Link.AdditionalOptions) /sourcelink:"$(SourceLink)"</AdditionalOptions>
<AdditionalOptions>%(Link.AdditionalOptions) @(SourceLinks->'/sourcelink:&quot;%(Identity)&quot;', ' ')</AdditionalOptions>
Copy link
Member

Choose a reason for hiding this comment

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

Identity

The values need to be escaped before they can be used as command line arguments. Perhaps, for simplicity of msbuild, FindAdditionalSourceLinkFiles could instead produce the command line string instead of individual items?

@tmat
Copy link
Member

tmat commented Aug 12, 2021

@nickdalt Thanks for the PR - sorry for the delay in review. Was busy with other things.

Any ideas how we can test this reasonably?

I think we should at least have unit tests for FindAdditionalSourceLinkFiles task. End-to-end test might be too complicated since it would depend on VC++ compiler targets.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

Command line switches need escaping and we also need a test.

…, multiple uses of the link /sourcelink arg are used.
Added unit tests for FindAdditionalSourceLinkFiles
Tweaks to inheritance to handle Task type confusion in VS2022
@nickdalt
Copy link
Author

nickdalt commented Sep 6, 2021

Command line switches need escaping and we also need a test.

I am not clear what you want here

Any ideas how we can test this reasonably?

I think we should at least have unit tests for FindAdditionalSourceLinkFiles task. End-to-end test might be too complicated since it would depend on VC++ compiler targets.

I can't see any easy way of testing end to end since that would require publishing the NuGet, consuming it in multiple C++ projects, building them, and finally checking that the pdb contains the sourcelink details for the libs.

I have however added unit tests for FindAdditionalSourceLinkFiles

@sylveon
Copy link

sylveon commented May 24, 2022

I am interested in this - however my setup is a few static libraries packaged in a NuGet package and then acquired in another solution. I imagine in this setup, it would be required for the NuGet package to ship the relevant .sourcelink.json files, and then for the consumer of my package to also have the SourceLink NuGet package installed so that it can then be passed to link.exe during the final link to a .DLL file, correct?

Also, it seems SourceLink is disabled by default for IDE builds, but in a setup like the one mentioned here, we would need to at least get the SourceLink metadata from the static libraries into the final PDB even from IDE builds. Would we need to enable SourceLink as a whole for IDE builds or is this something this PR considers?

I guess, alternatively, we could tweak our NuGet package's props file to pass the relevant /sourcelink flags for the .lib files it ships, but would the linker be okay with potentially passing the same sourcelink.json file twice? For example if the consuming project also uses SourceLink.

@tmat
Copy link
Member

tmat commented Dec 31, 2022

@nickdalt Sorry for the long delay. Switched focus to some other work. If you're still interested in bringing this PR forward, please rebase to the latest main.

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.

4 participants