ILSpy

ILSpy copied to clipboard

2nd part for #2346

@sharwell we'd like to cherry-pick that PR as well - which changes should be included if we wanted to remove the workaround as pointed out by Tom in #2346? (note that today it seems to be a mix of #2346 and the "second part")

I think there's not much cherry picking left, it's more like take it or leave it - unless you want to ignore DRY and SPOM and resemble what the VSIX-SdkProjectAdapter does, implementing the same by your own.

Sorry, I know nothing about the specifics in the VS addin csproj file format. Therefore I have to ask.

I'm not a huge fan of hiding the VSIX build configuration behind a new package. For me, it make it harder to understand what the build is doing because it no longer follows the default behavior for VSIX builds.

Aside from that, I'm very much not a fan of the way PowerShell is used in the VSIX-SdkProjectAdapter build targets. If you want to inject a value for Version in the vsixmanifest, all you need to do is set Version="|%CurrentProject%;GetVsixVersion|" in your vsixmanifest and define the GetVsixVersion target in your project. See https://github.com/tunnelvisionlabs/InheritanceMargin/commit/0f9db24410967d488eab0bce9845c09407931672 for additional examples of how this built-in functionality can be used.

I'm not a huge fan of hiding the VSIX build configuration behind a new package. For me, it make it harder to understand what the build is doing because it no longer follows the default behavior for VSIX builds.

• what exactly do you mean by the "default behavior for VSIX builds"? Where is this defined? Officially there is no support for VSIX in SDK-style projects
• the paradigm of SDK-style projects is to stop duplicating all the defaults in every project (DRY), so you should not use SDK-style projects at all, they are hiding so many defaults...

all you need to do is set [Version="|%CurrentProject%;GetVsixVersion|"]

if that would have been documented somewhere, I'd used this from the beginning. I'll definitely gonna change this!

So, provocatively speaking, we'd "paper over" a few cracks in project format and exchange the devil we know (the workaround) for a NuGet package that is the "filler". Did I get that options comparison close to right?

@sharwell: updated the VSIX-SdkProjectAdapter, the PowerShell is now gone... @christophwille: the devil always will catch you earlier or later, and the NuGet package is the "exorzist" 😏; I'm using that e.g. here dotnet/ResXResourceManager, which uses quite a lot of runtime NuGet packages, so this is proven to not break anything

I'm not going to block this if someone wants to merge it, but I would caution the maintainers to unzip the resulting VSIX before and after this change and do a directory diff to make sure there aren't any unintended consequences.

I have already tested this, the only difference is that it now contains all binaries from the mono.cecil package, so that is now on the safe side

@tom-englert the concept seems cool. I'm not familiar with the specific way NuGet is being used to resolve dependencies. How do you envision the automatic assembly inclusion to play with [ProvideCodeBase], considering packages need a 1:1 mapping of included assemblies to this attribute?

@sharwell VSIX-SdkProjectAdapter only ensures that all the necessary targets are executed in the same order as when using the old style project format, so the VSIX container is generated and the content is the same.

It does not deal with the [ProvideCodeBase] attribute. Which use case do you have in mind?

For VSIX dependencies, it's not sufficient to just include them in the package. You also have to include information in the pkgdef file to inform Visual Studio about the available assembly. Normally this is added with ProvideCodeBaseAttribute. If you automate the inclusion of NuGet dependencies into the VSIX package but don't also automate the application of [ProvideCodeBase] for those assemblies, it becomes very easy to update a NuGet package and have a new dependency in the package that Visual Studio will not be able to find.

See here for an example of registering dependencies that are included in a VSIX package: https://github.com/laurentkempe/GitDiffMargin/blob/05cc232d8e6507302ea1d2958c50a68893a6436a/GitDiffMargin/Properties/AssemblyInfo.cs#L39-L53

@sharwell I know, I'm using this in my extensions, too: https://github.com/dotnet/ResXResourceManager/blob/master/src/ResXManager.VSIX/Properties/AssemblyInfo.cs

However VSIX-SdkProjectAdapter does not automate the inclusion of NuGet dependencies, it just restores the build behavior as it was with the original old-style project format, where including NuGet packages already worked the way it is now.

Automating the ProvideCodeBase behavior is a good point I was already thinking about, but not really related to this PR. Can we move this discussion somewhere else?

How about to https://github.com/tom-englert/VSIX-SdkProjectAdapter/issues with a link back to where it started?

Closed as stale