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

Enable EmbedInteropTypes for PackageReference (matching Packages.Config capability) #2365

Closed
AArnott opened this issue Mar 20, 2016 · 58 comments
Assignees
Labels
Milestone

Comments

@AArnott
Copy link
Contributor

AArnott commented Mar 20, 2016

In the packages.config world, when references are actually added to the user's project file, the project system will add <EmbedInteropTypes>true</EmbedInteropTypes> metadata to the reference when its sniff-test tells it that the assembly should be linked rather than referenced. This is an important distinction for several reasons.

But in the project.json world, this is lost -- the project system has no say in the matter, and the build system seems to reference everything even when it should be linked. This will break scenarios when Visual Studio SDK extensions use NuGet packages for such references as Microsoft.VisualStudio.Imaging.Interop.14.0.DesignTime as the types won't be embedded, and although the extension might work on that developer's machine (who has the VS SDK installed), the extension will fail at runtime on the end user's machine because that assembly isn't present. It was supposed to be embedded into the extension author's assembly!

I discussed this in a design meeting with @davidfowl and @jasonmalinowski in mid-September, 2015. But I haven't heard that anything came of it afterward. The design IIRC was that the NuPkg should support a new link folder next to lib and ref. The assemblies in the link folder get passed to the compiler with /link: instead of /ref:.

So a package with this file layout:

MyPackage.nuspec
lib\
   net45\
      MyRegularAssembly.dll
link\
   net45\
      MyContracts.EmbedME.dll

When consumed by a C# project would produce a command line like this:

csc.exe /ref:MyPackage\1.0.0\lib\net45\MyRegularAssembly.dll /link:MyPackage\1.0.0\link\net45\MyContracts.EmbedME.dll

Design Spec - https://github.com/NuGet/Home/wiki/Embed-Interop-Types-with-PackageReference

@yishaigalatzer
Copy link

I think as a workaround, the vs packages can use msbuild targets for now. I'm moving this out.

@yishaigalatzer yishaigalatzer added this to the Client-VNext milestone Mar 23, 2016
@AArnott
Copy link
Contributor Author

AArnott commented Mar 23, 2016

Yes, as a workaround I've published http://nuget.org/packages/microsoft.visualstudio.sdk.embedinteroptypes

@jaredpar
Copy link

@AArnott how would a consumer of a interop package differentiate if they wanted to link or not? Today I can /reference:EnvDTE or /link:EnvDTE. I would assume EnvDTE comes in a single package (maybe that's wrong). As a consumer though how would I control the behavior?

@AArnott
Copy link
Contributor Author

AArnott commented Apr 13, 2016

@jaredpar You shouldn't. You should only reference EnvDTE. It isn't an embeddable assembly because (IIRC) it defines constants. Yes, the compiler lets you embed it, till you happen to reference some of those non-embeddable types and then the compiler screams at you and you're forced to reference it instead of link it. And as the CLR and compiler work best when everyone either links or references in unison, everyone should either link an assembly or everyone should reference it. There should not be a mix.

I think EnvDTE10 was the first truly embeddable assembly in that family.

@jaredpar
Copy link

@AArnott I think you're missing my point. Ignore the policy and compat issues around EnvDTE.

The core issue is that there are interop assemblies in existence today that some developers link and others reference. How can I distinguish between those two cases today in project.json?

And as the CLR and compiler work best when everyone either links or references in unison, everyone should either link an assembly or everyone should reference it. There should not be a mix.

That's a nice policy. Reality though is developers do bad things and we have to support that scenario. Not supporting mixing embed and not embed won't fix developers patterns, it will just be an adoption blocker.

@AArnott
Copy link
Contributor Author

AArnott commented Apr 13, 2016

I understood your point that today users choose whether to embed or not. But I don't see how them losing the choice is an adoption blocker. If folks referenced an assembly and when they update their nuget package it starts linking instead, I can't imagine how that would break them or cause them to avoid the upgrade.
If folks were linking an assembly that should be referenced instead, sure, that would cause at least a hiccup for them as they may need to deploy an assembly that they didn't before. But are there scenarios that you know of where that actually would happen? The compiler usually doesn't let you link an assembly that shouldn't be. The only exception I can think of is EnvDTE, and that doesn't matter if we start referencing instead of linking because VS ships it anyway.

So what is the blocking scenario that you're thinking of if consumers can't pick?

@jaredpar
Copy link

@AArnott

So what is the blocking scenario that you're thinking of if consumers can't pick?

Exactly what I said. Today I have a library that sometimes I /link and sometimes I /reference. How in project.json can I specify the setting I want in the project I want?

I feel like this is a question that should have a very straight forward answer. Either:

  • There is no way to do this. That to me is an adoption blocker. Customers don't care about best practices. They care about converting their code, as it exists today. Asking them to change deployment semantics is going to get push back.
  • There is a way to do this and the syntax is X.

In the case there is no way to do this then project.json is not functionally equivalent to what I have today. It cannot fully express the flexibility of the compiler.

@AArnott
Copy link
Contributor Author

AArnott commented Apr 14, 2016

It cannot fully express the flexibility of the compiler.

It already cannot. We're talking about giving project.json the ability to express more of what the compiler can do.

In the case there is no way to do this then project.json is not functionally equivalent to what I have today.

Sure it is: if you upgrade to the latest NuGet that adds support, nothing changes. You'd also need to update one of your NuGet package dependencies to a newer version that leverages the feature.

Exactly what I said

But you haven't answered my question: can you name a specific scenario where it's important for a customer that the behavior not change? This won't cause linked assemblies to start being referenced (since they're all referenced today, unless special msbuild authoring has been written and this feature wouldn't break that). This feature may cause assemblies you were referencing to start linking (when you update the package), and that won't screw up your deployment semantics, which is what you mentioned being concerned about. You can always deploy an assembly you've linked without side effects. But now you can stop deploying it.

So I repeat: I'd be interested in seeing a concrete scenario that breaks with this feature.

@jaredpar
Copy link

We're talking about giving project.json the ability to express more of what the compiler can do.

If we're fixing it then lets fix it. Instead of starting from first princples about how builds should work we should start with how they actually work. Any gap in the flexibility is a potential adoption blocker.

So I repeat: I'd be interested in seeing a concrete scenario that breaks with this feature.

I've provided a concrete scenario. There is a lib that the customer sometimes /link: and sometimes /reference:. How can they replicate their build, and hence their deployment dependencies, with this proposal.

This feature may cause assemblies you were referencing to start linking (when you update the package), and that won't screw up your deployment semantics,

So you're guaranteeing that every customer out there will be 100% happy with us suddenly starting to link assemblies they were previously referencing? When they push back on this what will way say? Sorry this is how you should have been building?

@AArnott
Copy link
Contributor Author

AArnott commented Apr 15, 2016

So you're guaranteeing that every customer out there will be 100% happy with us suddenly starting to link assemblies they were previously referencing?

No. You keep leaving out the point that we're not changing behavior for any existing solutions. You have to upgrade to see the new behavior.

I've provided a concrete scenario. There is a lib that the customer sometimes /link: and sometimes /reference:. How can they replicate their build, and hence their deployment dependencies, with this proposal.

That sounds more abstract than concrete. It's a straw man. Can you give me a single actual assembly that customers reference today that would break the customer's scenario after upgrading the package when it starts to link instead?

Otherwise, I don't see why you're so passionate about it.

@harikmenon harikmenon modified the milestones: Client-VNext, Future Apr 19, 2016
@int19h
Copy link

int19h commented Jan 24, 2017

A similar workaround NuGet package is needed for VS 2017, since it has some new assemblies that require embedding, e.g. Microsoft.VisualStudio.Imaging.Interop.15.0.DesignTime.

@AArnott
Copy link
Contributor Author

AArnott commented Jan 24, 2017

@int19h: I've pushed this package that includes the assembly you mention: Microsoft.VisualStudio.SDK.EmbedInteropTypes 14.1.7

@int19h
Copy link

int19h commented Jan 24, 2017

Another one is Microsoft.VisualStudio.Shell.Interop.15.0.DesignTime.

I'm not sure if there's some reliable place to get a complete list of these...

@AArnott
Copy link
Contributor Author

AArnott commented Jan 24, 2017

There isn't an exhaustive list, sadly. So as they are reported, we add them.
I've pushed a new version that adds that assembly as well.

Microsoft.VisualStudio.SDK.EmbedInteropTypes 15.0.4

@jaredpar
Copy link

Still feel like this is the wrong approach. The question of

How can I embed the interop types from NuGet package X?

Shouldn't be

Andrew creates a new NuGet package

That's not scalable. I should be able to declare that I want an existing package embedded as is. The existing solution is introducing an artificial barrier into NuGet package consumption that shouldn't exist.

@AArnott
Copy link
Contributor Author

AArnott commented Jan 25, 2017

I totally agree that I don't scale.

But I still think the package author should be able to indicate that an assembly is meant to be linked rather than referenced so consumers don't have to be super smart and take extra steps to do it right.

@tmat
Copy link

tmat commented Aug 23, 2017

Any updates on this? This is PITA to deal with.

@IlyaBiryukov
Copy link

It would be great to have it in metadata. E.g.

<PackageReference Include="MyEmbeddableNuget" Version="1.0.0">
    <EmbedInteropTypes>true</EmbedInteropTypes>
</PackageReference>

@AArnott
Copy link
Contributor Author

AArnott commented Sep 15, 2017

@IlyaBiryukov since packages can contain several assemblies, and those assemblies may be a mix of those that should be linked vs. those that should be referenced, how would you resolve that in your proposal?

Jared and I discuss this option above of letting the consumer influence the linking of assemblies, which you can review. My argument is an assembly can and should be linked if and only if its author went to special steps to enable and maintain it. There is no circumstance (that I'm aware of) that the consumer could make a better decision about link vs. ref than the library author. So I'm arguing that the decision be made and expressed in the package itself.

@BobbyCannon
Copy link

I just realized that the new PackageReference won't let me "EmbedInteropTypes" == false for certain assembly references... so I have to roll back a really long conversion from packages.config to PackageReference, 14 projects, wish I would have known this earlier today.

Anyway, what does this mean for projects like my open source project that requires "EmbeddedInteroptTypes" reference option? I guess I should just post a notice to not migrate to PackageReference until there is a way to handle this. Kinda stinks because people are going to pull my nuget package and think TestR doesn't work. Super sadface... I think this is important for the minority (aka me) :)

@AArnott
Copy link
Contributor Author

AArnott commented Nov 21, 2018

Interops are still a compile time decision though, defining it on the package is too broad.

I disagree. The package author knows whether they designed the assembly for linking or referencing, since linking requires very special considerations. The user should not reference an assembly that is meant to be linked. That causes problems like compiler warnings, failures at runtime when the assembly isn't even present, etc.

BTW, on that subject, nuget should not deploy a embed/link assembly to the application's bin folder (i.e. it should be Private=false, CopyLocal=false).

@jainaashish
Copy link

With that proposal, you'd need to explicitly specify dependency and embed for it work.

No you don't. With new proposal, PackageType:

  • Default continue to be dependency (no change)
  • embed which means its dependency package but interop type.
  • dotnettool continue to be same
  • dotnetclitool continue to be same
    ......................

BTW, on that subject, nuget should not deploy a embed/link assembly to the application's bin folder (i.e. it should be Private=false, CopyLocal=false).

I'd imagine Project System will take care of this since NuGet doesn't directly add any reference.

@AArnott
Copy link
Contributor Author

AArnott commented Nov 21, 2018

BTW, on that subject, nuget should not deploy a embed/link assembly to the application's bin folder (i.e. it should be Private=false, CopyLocal=false).

I'd imagine Project System will take care of this since NuGet doesn't directly add any reference.

Can you verify this as part of this feature work, please? I don't know how the lines divide nuget vs. csproj build authoring goes, but it's the responsibility of the thing that creates the Reference/ReferencePath items to set metadata on those items to prevent copying to the bin folder. I expected NuGet owned the MSBuild targets that converted PackageReference to ReferencePath, which if that's the case, suggests that my request is a nuget responsibility, IMO.

@nkolev92
Copy link
Member

nkolev92 commented Nov 21, 2018

I disagree. The package author knows whether they designed the assembly for linking or referencing, since linking requires very special considerations. The user should not reference an assembly that is meant to be linked. That causes problems like compiler warnings, failures at runtime when the assembly isn't even present, etc.

@AArnott
I worded that badly. I am not suggesting it's a user decision.
It's an author decision.
I meant solely as "link" vs "ref" during compilation.

Even if it's not a scenario where people have packages that don't do both, I don't see the need to change a well established pattern that doesn't have any obvious issues.

@jainaashish
If you want to do both is what I mean.

@jainaashish
Copy link

Even if it's not a scenario where people have packages that don't do both, I don't see the need to change a well established pattern that doesn't have any obvious issues.

I'm sorry but loosing the point here, when did it become a well established pattern? which nobody uses...? interop types are already a advance concept which only handful of people use so they know what they are doing.

@nkolev92
Copy link
Member

The well established pattern is the folders for different asset groups.
ContentFiles has a similar approach to this where the build action is specified by the nuspec, but even that's for a specific asset group rather than on a whole package.

@jainaashish
Copy link

in that sense, these are still compile assets which are being linked instead of reference which is a implementation details. So essentially these are still not entirely a different asset group rather a different package type, IMO.

@jainaashish
Copy link

@brinko99
Copy link

brinko99 commented Jul 30, 2020

@nkolev92

More details and an example here: https://github.com/NuGet/Samples/tree/master/NuGet.Samples.Interop

You're sample seems to fail for me when the package's target framework doesn't match that of the importing project. For example, your sample targets net46. When I add the package to Core 3.1 project, the file is neither linked or referenced. VS doesn't even show the containing assemblies.

image

If I modify the package to include the same assembly in netcoreapp3.1 folder, it does work. Is this expected behavior? Is there any further workaround at this time while we wait for 'embed'?

Edit: This is under VS 16.6.3, dotnet 3.1.301

Edit: I've got it working...

My assemblies were located under lib\net35; I had the .targets under build\ but find that it must be under build\net35.

Thanks,

@jblockx
Copy link

jblockx commented Oct 10, 2024

@AArnott, @nkolev92

From the sample:

Additionally, by default the build assets do not flow transitively. Packages authored as described here work differently when they are pulled as a transitive dependency from a project to project reference. The package consumer can allow them to flow by modifying the PrivateAssets default value to not include build.

This is the part that hurts the worst. It's not entirely in the package author's hands then -- middle-tier consumers have to be aware and do something special. That's unfortunate.

With respect to the sample I was wondering whether it would be an option to add the (NuGet.Samples.Interop.)targets not only to the build folder but also to the buildTransitive folder?
As explained on the following pages:

As far as I can tell, this would avoid that the package consumer needs to adapt the PrivateAssets default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests