dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.67k stars 1.06k forks source link

Consume NuGet /embed assets group for interop type assemblies from NuGet package #2732

Open jainaashish opened 5 years ago

jainaashish commented 5 years ago

This is the tracking issue to update dotnet build tasks to consume new /embed assets group from NuGet packages for interop type assemblies so that Project System passes it as link instead of ref to compiler.

Corresponding NuGet issue# https://github.com/NuGet/Home/issues/2365

@livarcocc @nguerrera @dsplaisted @rrelyea

jainaashish commented 5 years ago

NuGet PR# https://github.com/NuGet/NuGet.Client/pull/2532 to implement this feature is completed and will be inserted into VS/SDK/CLI with next dev16 insertion.

nkolev92 commented 4 years ago

@marcpopMSFT @dsplaisted @wli3

We had a question around functionality. Can we have this re triaged and get an estimate on it? Thanks.

marcpopMSFT commented 4 years ago

Moving it in to the .net 5 backlog which falls below our committed optional workload work but lifts it above the normal backlog. @nkolev92 do you have details on what the specific set of work we're asked to do to support this to help us cost it?

nkolev92 commented 4 years ago

@marcpopMSFT

Yes, here are some details.

The design can be found here: https://github.com/NuGet/Home/wiki/Embed-Interop-Types-with-PackageReference.

NuGet will write out a new section in the assets file equilvane to the compile items section: https://github.com/NuGet/Home/wiki/Embed-Interop-Types-with-PackageReference#sample-relevant-portion-of-projectassetsjson-file.

The files listed in that section should be passed to a compiler with a -link option, https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/link-compiler-option.

In MSBuild item terms that likely means just adding some extra metadata, like EmbedInteropTypes.

So change is in here, https://github.com/dotnet/sdk/blob/73c5b4e1dd36c3a8a041774e86eb3418622682d9/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs, read the new item types and pass them to the compile with extra the EmbedInteropTypes metadata.

I don't foresee this to be a super complex change.

nkolev92 commented 4 years ago

fyi @aortiz-msft

The SDK side of https://github.com/dotnet/NuGet.BuildTasks/issues/69.

marcpopMSFT commented 1 year ago

@JonDouglas @aortiz-msft looks like this never got done and was bumped forward release to release. What's the priority from the nuget side on this as we should either do it or move to the backlog so it doesn't keep getting strung along? Do you know the customer importance? Who needed the feature and do they still need it?

aortiz-msft commented 1 year ago

@nkolev92, would you mind summarizing the current status?

nkolev92 commented 1 year ago

I'd argue the feature is needed but not as much as before. I'd expect the change overall to be super simple though as the NuGet part is fully done and only the build part is left.

aortiz-msft commented 1 year ago

@marcpopMSFT See above.

dsplaisted commented 1 year ago

Hi folks,

We're working on adding this to the .NET SDK and I have some questions:

Should packages using this feature put a DLL in both the lib and embed folders? The spec seemed to suggest this, but since the result of the embed folder is that EmbedInteropTypes metadata should be set to true on the compile reference, it seems like maybe the DLL should only go in the embed folder. If it's in both, we will probably need logic in the ResolvePackageAssets task to deduplicate the items, or to match up the embed items with the lib ones.

Is there a simple way to test the end-to-end functionality? How can we test that the scenario is working correctly, preferably without having to involve COM interop.

@jainaashish @nkolev92 @AArnott

nkolev92 commented 1 year ago

Should packages using this feature put a DLL in both the lib and embed folders? The spec seemed to suggest this, but since the result of the embed folder is that EmbedInteropTypes metadata should be set to true on the compile reference, it seems like maybe the DLL should only go in the embed folder

It seems like the idea with duplicating was for the package to remain compatible with packages.config.

I think with interop the expectation would be that the assembly gets embed into the final assembly and that's it. It should not be deployed in the runtime folder.

If it's in both, we will probably need logic in the ResolvePackageAssets task to deduplicate the items, or to match up the embed items with the lib ones.

I feel like this should've been dedupped on NuGet side, but it's ok if it's done on the RPA side too.

AArnott commented 1 year ago

Yes to everything @nkolev92 said. Deduping by assembly (file)name makes sense, since a package could conceivably include both lib/a.dll and embed/b.dll with the intent to reference a.dll and link b.dll. But if a.dll appears in both lib/ and embed/, assuming that embed/ is the actual intent makes a lot of sense.

dsplaisted commented 1 year ago

Thanks folks.

We still need to figure out how to have a simple test case for this. When we put a simple library in the embed folder, we get the following error from the compiler:

Cannot embed interop types from assembly 'EmbedPackage, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' because it is missing either the 'System.Runtime.InteropServices.ImportedFromTypeLibAttribute' attribute or the 'System.Runtime.InteropServices.PrimaryInteropAssemblyAttribute' attribute.

What do we need to do to make a simple library that can embed interop types for a test? I vaguely remember something about NoPIA being possible to use for interfaces that didn't map to COM, maybe we could do something like that for testing?

nkolev92 commented 1 year ago

You need to build it with ImportedFromTypeLib specified. Here's what we have in one of our projects: https://github.com/NuGet/NuGet.Client/blob/release-5.11.x/src/NuGet.Clients/NuGet.VisualStudio/AssemblyInfo.cs