Skip to content

Replace GeneratePkgDef with impl that does not load assemblies#43302

Merged
tmat merged 10 commits intodotnet:masterfrom
tmat:RemoveCreatePkgDef
Apr 23, 2020
Merged

Replace GeneratePkgDef with impl that does not load assemblies#43302
tmat merged 10 commits intodotnet:masterfrom
tmat:RemoveCreatePkgDef

Conversation

@tmat
Copy link
Member

@tmat tmat commented Apr 12, 2020 •
edited
Loading

Removes all RegistrationAttributes applied to our VS package type definitions.
These attributes were used by CreatePkgDef tool to generate pkgdef entries. This tool loaded our assemblies during build and executed code on each RegistrationAttribute that produces pkgdef entries.

The replacement avoids loading assemblies during build, which is problematic, with an msbuild target that generates the pkgdef entries. Instead of adding attributes to VS package type definitions we now specify the raw pkgdef entries manually in PackageRegistration.pkgdef file. This file gets merged into the final pkgdef that will be included in the VSIX.

pkgdef entries that contain version information are auto-generated based on msbuild items (since the content is not static text). See 7bd173d#diff-1d351de3898f9b1efcf9aa14605f5766R4-R36 for detailed description.

TODO:

Changes to pkgdef

Roslyn.VisualStudio.Setup

added

	[$RootKey$\Editors\{08467b34-b90f-4d91-bdca-eb8c8cf3033a}\Extensions]
	"csx"=dword:00000027
	[$RootKey$\Editors\{A6C744A8-0E4A-4FC6-886A-064283054674}\Extensions]
	"csx"=dword:00000028

removed

	[$RootKey$\RuntimeConfiguration\dependentAssembly\bindingRedirection\{E92D0A46-3BAC-0BD9-DE02-24BD0A972AF8}]
	"name"="Microsoft.VisualStudio.CallHierarchy.Package.Definitions"
	"publicKeyToken"="31BF3856AD364E35"
	"culture"="neutral"
	"oldVersion"="14.0.0.0-14.9.9.9"
	"newVersion"="15.0.0.0"

tmat
tmat commented Apr 12, 2020
View reviewed changes
tmat
tmat commented Apr 12, 2020
View reviewed changes
jasonmalinowski
jasonmalinowski reviewed Apr 14, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Looking fine so far; the better integration and handling where you can just mark a ProjectReference to have a binding redirect or codebase is really nifty.

There's some parts I can't review as the .pkgdefs are being considered by GitHub to be binary files. I'm guessing they're being encoded as UTF-16 and confusing the binary/text classifier? Can we reencode the files?

I absolutely do want us to reach out to the VS SDK owners though to get this integrated back so we're not maintaining this long term.

src/VisualStudio/Setup/Roslyn.VisualStudio.Setup.csproj Outdated
Comment on lines 295 to 296
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 14, 2020

Choose a reason for hiding this comment

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

@dibarbet Why do we have to Codebase these? I guess if we're shipping our own copy then we have to be the ones to declare the codebase for this particular version?

@tmat tmat mentioned this pull request Apr 15, 2020
Merged
@tmat tmat force-pushed the RemoveCreatePkgDef branch from 7bd173d to 83790d5 Compare April 17, 2020 22:32
@tmat tmat marked this pull request as ready for review April 17, 2020 22:37
@tmat tmat requested review from a team as code owners April 17, 2020 22:37
@tmat tmat force-pushed the RemoveCreatePkgDef branch from b21c9ef to 4011180 Compare April 19, 2020 23:29
@tmat tmat force-pushed the RemoveCreatePkgDef branch from 4011180 to 907c489 Compare April 20, 2020 23:46
jasonmalinowski
jasonmalinowski requested changes Apr 21, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Only big issue (and reason for block) seems to be that the LiveShare .pkgdef includes stuff that would have came from attributes, but I don't see the attributes being deleted anywhere; I'm guessing that got forgotten and either there's a conflict somehow or the source version is dead.

Otherwise stuff looks good.

src/VisualStudio/IntegrationTest/TestSetup/PackageRegistration.pkgdef
"{d02dac01-ddd0-4ecc-8687-79a554852b14}"=dword:00000002

[$RootKey$\Menus]
"{d02dac01-ddd0-4ecc-8687-79a554852b14}"=", Menus.ctmenu, 1"
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 21, 2020

Choose a reason for hiding this comment

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

We've confirmed there's no extra magic around menus this is also breaking?

Copy link
Member Author

@tmat tmat Apr 22, 2020

Choose a reason for hiding this comment

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

I don't understand the question. This is the exact value there used to be before.

@tmat tmat merged commit 0daa32b into dotnet:master Apr 23, 2020
@ghost ghost added this to the Next milestone Apr 23, 2020
@tmat tmat deleted the RemoveCreatePkgDef branch April 23, 2020 01:21
@tmat tmat modified the milestones: Next, 16.7 Apr 23, 2020
333fred added a commit to 333fred/roslyn that referenced this pull request Apr 23, 2020
@333fred
Merge remote-tracking branch 'upstream/master' into merge-master
86c102a 
* upstream/master: (1099 commits)
  Specially handle tuple methods in CodeGenerator.EmitMethodInfoExpression for VB. (dotnet#43553)
  Remove unnecessary usings
  AssetStorage cleanup (dotnet#43511)
  Remove unused code (dotnet#43556)
  Revert anonymous type DebuggerDisplay change (dotnet#43575)
  Replace GeneratePkgDef with impl that does not load assemblies (dotnet#43302)
  Fix
  Update src/Analyzers/Core/CodeFixes/MakeFieldReadonly/AbstractMakeFieldReadonlyCodeFixProvider.cs
  PR feedback
  use capacity when creating builders.
  Push options down.
  Rename methods
  Fix GetSymbolInfo on ValueTuple declaration (dotnet#43467)
  Add support for cref-type-parameters.
  Support OOP with dynamic types.
  Support error locals in symbolkey
  Update tests to run OOP
  Update docs/Language Feature Status.md
  Update for partial methods
  Fix typos (dotnet#43494)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

Assignees

No one assigned

Labels

Area-Infrastructure

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

3 participants

@tmat @jasonmalinowski @jinujoseph