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 'Microsoft.DiaSymReader.Native.*.dll' to the VisualStudioSetup VSIX #16273

Merged
merged 1 commit into from Jan 6, 2017
Merged

Add 'Microsoft.DiaSymReader.Native.*.dll' to the VisualStudioSetup VSIX #16273

merged 1 commit into from Jan 6, 2017

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Jan 6, 2017

FYI. @KevinH-MS, @tmat, @jaredpar, @Pilchie, @ManishJayaswal, @jasonmalinowski, @dotnet/testimpact

This is required for determinism to be supported in design-time builds. Live Unit Testing currently requires this (@KevinH-MS is probably the best person to delve into the finer details about the build system here, if required).

@KevinH-MS
Copy link
Contributor

More detailed explanation of dependency: If a project is compiled with /deterministic+ and /debug+ (legacy pdb format), then these dlls must be present in the same folder as Microsoft.CodeAnalysis.dll. Today, they only live in the MSBuild folder. In order to be able to call Emit on such projects using the IDE compiler, these files must exist in the IDE package folder.

@tmat
Copy link
Member

tmat commented Jan 6, 2017

👍

@tannergooding
Copy link
Member Author

Also, @srivatsn for ask mode approval.

  • Customer scenario - Live Unit Testing will not work for projects where determinism is enabled and the classic pdb format is targeted
  • Workarounds - The user can change their PDB format to portable-pdb
  • Risk - Low, this just adds the binaries to the package so they are available for use from Live Unit Testing
  • Performance impact - When Live Unit Testing is enabled, one additional binary will be loaded for solutions that meet the above criteria
  • Is this a regression from a previous update? - No, Emit is not called from the IDE compiler (design-time build) outside of Live Unit Testing
  • How was the bug found? - Repositories such as 'Roslyn' and 'Roslyn-Project-System' would not work with Live Unit Testing and failed compilation due to an error indicating these binaries were missing

@tannergooding tannergooding merged commit 2be7c0d into dotnet:master Jan 6, 2017
@jasonmalinowski
Copy link
Member

@tannergooding and @KevinH-MS: just curious: LUT does do Emit calls in-proc to build updated binaries? You don't always do the build through MSBuild?

@jasonmalinowski
Copy link
Member

@tannergooding: why wasn't this done by adding the Microsoft.DiaSymReader.Native to the "packages to include" list earlier in the project file?

@KevinH-MS
Copy link
Contributor

KevinH-MS commented Jan 6, 2017

@jasonmalinowski We actually do our Emit call in the Roslyn OOP process (piggy-backing on the Compilations used by Diagnostic analysis), but that process loads its dlls from the language services extension folder, and these dlls need to live SxS in order for them to get picked up by the loader.

@tannergooding
Copy link
Member Author

@jasonmalinowski, I had no clue if PackagesToInclude worked with native binaries/runtimes and knew that this worked, so I opted for it instead 😄

If PackagesToInclude works with runtimes\win\native, then I would be happy to switch it over.

@jasonmalinowski
Copy link
Member

@tannergooding: Did you try it? :-) It should work, since how that entire MSBuild magic works is to collect runtime outputs. Outside the purview of the build task it doesn't know the difference.
@KevinH-MS: 👍

@tannergooding
Copy link
Member Author

@jasonmalinowski, it is not working for DiaSymReader.Native

@jasonmalinowski
Copy link
Member

@tannergooding Grumble. Odd. Thanks for trying though.

@tannergooding
Copy link
Member Author

@jasonmalinowski, this probably needs further investigation, since the compilers extension project references the package the way you indicated.

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.

None yet

7 participants