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 SourceLink support to the library #39

Merged
merged 3 commits into from
Apr 17, 2018
Merged

Conversation

marfenij
Copy link

Hi again )
This is other awesome stuff which must present in each library IMHO.

I'm not familiar with codecov.io, but think this change should not break anything

@marfenij
Copy link
Author

marfenij commented Apr 2, 2018

Hi @dotarj
any notes for that PR ?

@dotarj
Copy link
Owner

dotarj commented Apr 6, 2018

Hi @marfenij, I'm not sure what the consequences of this change would be. I see that the assembly is larger, which is logical, but could there be any negative impact on performance? And what is effect on debugging in other software than Visual Studio 2017 (Visual Studio Code or Rider for example)?

@marfenij
Copy link
Author

marfenij commented Apr 6, 2018

I embedded PDB not used in runtime, it used for debug purpose.

That author of SourceLink write on Github

about VS Code:

about Rider:

  • I can't find any proof that this feature supported by it, but R# and .Peek allow to used that - mb Rider also support... I don't use Rider so can't check that

@dotarj
Copy link
Owner

dotarj commented Apr 13, 2018

Hi @marfenij, I'll try to read some more about SourceLink and run some tests this weekend. If everything is ok, I'll merge the PR after that.

@dotarj
Copy link
Owner

dotarj commented Apr 17, 2018

Hi @marfenij, I've had some trouble building protobuf-net-data when source link was enabled. Apparently, source link uses the command 'git rev-parse --show-toplevel' which failed because my machine was badly configured. That's resolved and everything looks good. One question though: shouldn't the argument -p:IncludeSymbols=true be removed for the msbuild -t:pack command?

@marfenij
Copy link
Author

@dotarj hi. I think need remove msbuild parameter, but I need also resolve conficts

@dotarj
Copy link
Owner

dotarj commented Apr 17, 2018

Ok, if you remove the argument, I'll resolve the conflicts. 😃

@marfenij
Copy link
Author

@dotarj these conflicts is not deadly, let's try check if you fix appveyor script

@dotarj
Copy link
Owner

dotarj commented Apr 17, 2018

All systems go! 😄 Only the -p:IncludeSymbols=true argument still has to be removed, but I'll do that in another commit. Thanks for the contribution!

@dotarj dotarj merged commit 759cd1c into dotarj:master Apr 17, 2018
@marfenij
Copy link
Author

@dotarj I stuck with internet and can't build solution - nuget can't restored

But you do it ))))

@marfenij marfenij deleted the source_link branch April 17, 2018 20:12
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