GitTools / GitLink

Making .NET open source accessible!
MIT License
554 stars 86 forks source link

Add IndexAllDepotFiles option (-a) #167

Closed jairbubbles closed 7 years ago

jairbubbles commented 7 years ago

It allows to properly index sources for native Pdbs

A few remarks:

On a simple cpp project, source information looks like this:

VERSION=2
SRCSRV: variables ------------------------------------------
RAWURL=https://raw.github.com/jairbubbles/BitmapFont/bfaf267c467dc18be3b037d80d45bcec7eb51cff/%var2%
SRCSRVVERCTRL=https
SRCSRVTRG=%RAWURL%
SRCSRV: source files ---------------------------------------
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\BitmapFont.cpp*BitmapFont/BitmapFont.cpp
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\BitmapFontCache.cpp*BitmapFont/BitmapFontCache.cpp
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\BitmapFontCache.h*BitmapFont/BitmapFontCache.h
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\BitmapFontCache_Test.cpp*BitmapFont/BitmapFontCache_Test.cpp
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\Rect.h*BitmapFont/Rect.h
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\Rect_Test.cpp*BitmapFont/Rect_Test.cpp
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\stdafx.cpp*BitmapFont/stdafx.cpp
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\stdafx.h*BitmapFont/stdafx.h
C:\Users\jairb\Source\Repos\BitmapFont\BitmapFont\targetver.h*BitmapFont/targetver.h
SRCSRV: end ------------------------------------------------

Which seems ok. (I didn't test in Visual Studio if symbols are correctly retrieved yet).

Fixes #166

GeertvanHorrik commented 7 years ago

Thanks, that was fast!

The only thing I am missing is the option that can be added to the msbuild task as well. Maybe @AArnott can give some hints about that one.

jairbubbles commented 7 years ago

Should we add a new option or reserve this mode only for native pdbs? If it's not useful for managed pdbs maybe we should not expose it and keep it internal. GitLink is simple and that's what I like about it.

GeertvanHorrik commented 7 years ago

I think it can be added here:

https://github.com/GitTools/GitLink/blob/develop/src/GitLinkTask/GitLink.targets#L9

Can we auto-detect native pdb? If so, we can auto-pass in the right value there (but keep it overridable if users want to). That way we still keep it simple for starters but allows for maximum flexibility.

GeertvanHorrik commented 7 years ago

Looks great!

GeertvanHorrik commented 7 years ago

I can remove the WIP from the title now I think :)

GeertvanHorrik commented 7 years ago

1 last thing that needs updating is the readme (where we explain the feature). Then it's all ready to be merged.

jairbubbles commented 7 years ago

Ok do you want me to check for native pdb detection in another merge request?

GeertvanHorrik commented 7 years ago

Ok do you want me to check for native pdb detection in another merge request?

If you know how to do that now, then it can be included in this one. If you need to investigate, we can do it in a new PR.

jairbubbles commented 7 years ago

Yes I do have to investigate as I'm not a PDB expert :wink:

GeertvanHorrik commented 7 years ago

Looks great, thanks for all the efforts. I can merge this one if you are ready for it.

jairbubbles commented 7 years ago

Just let me check that symbols are correctly retrieved from GitHub when debugging in Visual Studio.

jairbubbles commented 7 years ago

It works! (At least with that trivial example)

If I rename my file locally the source file gets downloaded from GitHub and put into cache: image

GeertvanHorrik commented 7 years ago

Nice, well done :) So I can start merging?

jairbubbles commented 7 years ago

Yep 😄

(It seems that if you call multiple times GitLink on a same pdb the source information gets duplicated and Visual Studio does not seem to like it)

GeertvanHorrik commented 7 years ago

It seems that if you call multiple times GitLink on a same pdb the source information gets duplicated and Visual Studio does not seem to like it

That's something we might need to look into for a future version as well.