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

Mark RichCodeNav.EnvVarDump as private asset #43238

Merged
1 commit merged into from
Apr 10, 2020

Conversation

jasonmalinowski
Copy link
Member

Otherwise this package becomes a dependency of all our NuGet packages.

Otherwise this package becomes a dependency of all our NuGet packages.
@jasonmalinowski jasonmalinowski requested a review from tmat April 9, 2020 20:22
@jasonmalinowski jasonmalinowski requested a review from a team as a code owner April 9, 2020 20:22
@jasonmalinowski jasonmalinowski self-assigned this Apr 9, 2020
@tmat
Copy link
Member

tmat commented Apr 9, 2020

I wonder if the package needs to be excluded in source build...

@BertanAygun
Copy link
Contributor

BertanAygun commented Apr 9, 2020

I wonder if the package needs to be excluded in source build...

The package can be excluded if EnableRichCodeNavigation != true but should be a no-op in that case anyway. It is needed to be part of source build though as the target generates a log file to help with LSIF generation afterwards.

@tmat
Copy link
Member

tmat commented Apr 9, 2020

Yes, it's a no-op, but the problem is that source build does not have access to this package at all.

Another thing that we might want to consider is to set EnableRichCodeNavigation = true in one leg of CI build, so that we have some validation done during PR. This would avoid breaking our official build if something went wrong. How much time does it take to run the target?

@BertanAygun
Copy link
Contributor

Another thing that we might want to consider is to set EnableRichCodeNavigation = true in one leg of CI build, so that we have some validation done during PR. This would avoid breaking our official build if something went wrong. How much time does it take to run the target?

This build target is very quick since it only produces an extra file near the output file with some build info (not index itself). There would be another build step (which I was thinking to do in a parallel CI for now to not cause any issues with existing builds) which then runs the actual LSIF tool based on those files.

@tmat
Copy link
Member

tmat commented Apr 9, 2020

In that case we should always run the target in CI (when ContinuousIntegrationBuild is true).

@jasonmalinowski
Copy link
Member Author

I wonder if the package needs to be excluded in source build...

I wasn't sure; I saw the other analyzers being excluded. How do I know? (It seems like the source build CI did fail, but because of some TLS handshake issue; I don't think it actually validated the change.)

@tmat
Copy link
Member

tmat commented Apr 10, 2020

@BertanAygun BTW, once this is implemented: #41395 you might be able to extract the information you need from DLL+PDB (build drop).

That said, the most efficient approach would be to integrate generation of LSIF data in csc task itself. This is because the compiler already has all symbols loaded and bound during compilation. Perhaps LSIF generation could be an analyzer that outputs data to a file?

@BertanAygun
Copy link
Contributor

@tmat, those are the long term plans I believe. I agree it would make everything a lot easier if we provided a msbuild variable that let the compiler output lsif side by side with the assembly.

Regarding the compiler switches in pdb, would that info also include csproj path, target framework etc? I assume latter can be derived from assembly itself but it would be nice to have that available easier.

@tmat
Copy link
Member

tmat commented Apr 10, 2020

Regarding the compiler switches in pdb, would that info also include csproj path, target framework etc? I assume latter can be derived from assembly itself but it would be nice to have that available easier.

No, it wouldn't contain this information since the compiler doesn't have it. It would contain all information necessary to recreate the compilation, assuming that all metadata references are available on a (symbol) server and all sources are available in the source control (or embedded in the PDB).

@tmat
Copy link
Member

tmat commented Apr 10, 2020

@tmat, those are the long term plans I believe. I agree it would make everything a lot easier if we provided a msbuild variable that let the compiler output lsif side by side with the assembly.

OK, sounds like a good plan.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit e12badf into dotnet:master Apr 10, 2020
@jasonmalinowski jasonmalinowski deleted the fix-richnav-privateassets branch May 12, 2020 05:04
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants