Project-system: Netstandard 2.0 projects show an assemblies node with all the facades

[nguerrera]

Environment

VS 15.3 d15rel 26504.1
.NET Core SDK 2.0.0-preview1-005957

Repro Steps

• File -> New Project -> Class Library (.NET Standard)
• Right click project -> Properties -> Change target framework from .NETStandard 2.0 to .NET Standard 1.1
• Save

Expected behavior

• There's nothing netstandard2.0 left to see.

Actual behavior

• Assemblies node still shows netstandard2.0 assemblies
• Go to definition on object brings me to netstandard,2.0.0 definition from metadata
• etc.

NOTE: Closing and reopening the solution gets me back to the correct state, unloading and reloading does not.

[srivatsn]

Related to races you've been investigating @davkean?

[davkean]

The language service is mixing data from multiple sources; it's reading the data flow - only to end up reading the real project file. Debugging this case, they don't agree - this is only going to cause a bunch of issues. Not sure it's related.

[davkean]

This code is nearly impossible to reason about - I have keep stop myself from rewriting it...

[davkean]

It looks like when TFM changes we throw away the entire Roslyn "project" and create a new one, but I can't see where we ever add the existing source files, references, etc to the new project.

[davkean]

Okay, even though my comment above is correct, this is a different bug.

The real cause of the underlying issues is due build giving us "impossible" results - I have zero idea who's bug this, CPS, MSBuild or the SDK:

Here's why I see when I change from .NET Standard 2.0 -> 1.1:

• TFM properties are correct (.NET Standard 1.1)
• project.assets.json is correct (only containing .NET Standard 1.1 data)
• <Reference/> items coming out of ResolveLockFileReferences are correct (only producing 1.1 references)

However, once the <Reference/> are passed to ResolvePackageFileConflicts, they are _incorrect_ and include .NET .Standard 2.0 references. I can't see where they are coming from - the first time we see these references is when they are passed to ResolvePackageFileConflicts - it's smells like some sort of stale cache of items.

32>  Using "ResolvePackageFileConflicts" task from assembly "C:\Program Files (x86)\Microsoft Visual Studio\Preview\Enterprise\MSBuild\Sdks\Microsoft.NET.Sdk\build\..\tools\net46/Microsoft.NET.Build.Tasks.dll".
32>  Task "ResolvePackageFileConflicts"
32>    Task Parameter:
        References=
            C:\Users\davkean\.nuget\packages\netstandard.library\2.0.0-preview1-25221-01\build\netstandard2.0\\ref\Microsoft.Win32.Primitives.dll
                    NuGetIsFrameworkReference=false
                    NuGetPackageId=NETStandard.Library
                    NuGetPackageVersion=2.0.0-preview1-25221-01
                    NuGetSourceType=Package
                    Private=false

[davkean]

Ah, the plot thickens, NETStandard.Library.targets is importing the references instead of using NuGet's support for adding references. This is imported from [project].csproj.nuget.g.targets targets

We don't currently reload targets , so when retargeting the [project].csproj.nuget.g.targets remain in memory, and so do the references. Not sure why these static items exist and we're just not using .NuGet's support for adding references @ericstj?

The other thing this is causing is that they are showing up under the "Assemblies" dependency node instead of showing them under the .NETStandard.Library SDK node:

[nguerrera]

We don't currently reload targets.

Scary. So adding/removing/upgrading any package with targets will have similar issues? I think this may be relevant to dotnet/sdk#1142.

I'm also astonished that unload/reload project doesn't force all of this to the same state as reloading the solution.

[davkean]

We're working on reloading targets for 15.3 as we speak, but I'd still like to know why we're using static items instead of just using the NuGet support. Clearly it's producing items differently to NuGet and that's confusing the Dependency node.

Basically for all eternity, once loaded, targets have been fixed for the life of a ProjectCollection. All CPS projects share a single project collection and all legacy project system share a single project collection, hence why unloading the project doesn't "fix" the problem.

[davkean]

I also filed https://github.com/dotnet/project-system/issues/2148 for the bug that I referred to here: https://github.com/dotnet/project-system/issues/2132#issuecomment-300057992.

[srivatsn]

@ericstj thoughts on the comment above? Can the package include the assemblies through NuGet's infrastructure?

[ericstj]

not sure why these static items exist and we're just not using .NuGet's support for adding references @ericstj?

The behavior we want is different than what NuGet provides. We only want NETStandard.Library to provide references on a NETStandard-targeting project. We don't want it to provide references when installed in say, a NETCoreApp or Xamarin project. We also don't want to have to update the package every time a new TFM comes around to add a placeholder to the package. /cc @terrajobst

[nguerrera]

Is any still a thing in nuget? Can you put placeholder there?

[davkean]

Never saw a reply to this? Something needs to be fixed here - this package isn't playing nicely with the Dependency node.

[ericstj]

You'll have to describe more about what you're suggesting. There is nothing wrong with this package functionally so if you ask us to change it you're ignoring a bug in the project system.

[davkean]

The project system thinks that this package contains normal references and so it's references are intermixed with the user's references instead of listed with the package itself.

The other thing I'm concerned about is if we change the generation of the <Reference/> this package will be out of sync of that generation. Is there a reason we can't do what @nguerrera is suggesting?

[davkean]

Not sure I understand the bug part, are you referring to lack of reload targets? That's been the behavior for 13 years since MSBuild 1.0. Does this package go downlevel if so, you're going to run into it there as well.

[davkean]

I suspect the data we're producing from the assets file isn't lining up with these "fake" NuGet references which is confusing the node.

[ericstj]

How should we change our references to make them more friendly with the project system? Add them in a target instead? I honestly don't know which is why I'm asking.

I don't understand how @nguerrera's suggestion of adding a placeholder under any fixes anything. We'd still have targets that you need to revaluate. That'd also cause side effects depending on where we put it.

With respect to the bug: If you change the TFM you are changing the entire project evaluation and you should revaluate all targets files including the includes for targets files. Otherwise you'll end up with folks who have TFM-specific targets files applying where they shouldn't. This isn't something specific to our package, but generic requirement of supporting nuget packages which have TFM-specific targets.
EG: build/netstandard1.2/foo.targets (do something) build/netstandard2.0/foo.targets (do something else).

[davkean]

You should use NuGet constructs for adding the references. That works and is well understood, and plays nicely with the dependency node. Ignoring targets. will @nguerrera's suggestion support what you are trying to do?

The targets issue is well understood, and we understand the ramifications of it - we are working on supporting it for 15.3. It's not as if this was just some simply bug we overlooked. It's literally been the behavior since we first shipped MSBuild. It's also not about evaluation - it's that we don't _reload_ targets once they have been loaded. The support MSBuild added for us to enable you to edit the live project in the Editor has unblocked these scenarios for us.

[ericstj]

Again I'll say, placeholders don't accomplish what we are trying to do. If you have a more specific idea than just "use a placeholder" please explain. Let me be more specific about how this package works and what its requirements are.

The NETStandard package wants to provide assets which are only applicable to NETStandard2.0 projects. If installed in a NETStandard2.0 compatible project that is not TargetFramework=NETStandard (eg: NETFramework, NETCoreApp, etc) we want to install successfully but not provide any references. Each framework provides its own NETStandard references. We can do this today by putting assets in ref and then placeholders in every TFM folder that supports netstandard2.0 today but then when a new netstandard2.0 framework comes along tomorrow we must update the package to support it by adding another placeholder. There is no TFM that sits between netstandard2.0 and the TFMs supporting netstandard2.0 so we cannot futureproof the package by putting a placeholder somewhere else. To get an idea of the precedence for a given TFM you can look at this (though its a little out of date).

Instead of using placeholders we could have a target subtract the references if the project is not TargetFramework=.NETStandard, but we chose the additive targets because we felt it was more in line with something that resembles and SDK. If it helps we can change the references to have less metadata so that they don't look like they came from NuGet, or move them to be added by a target, or something else.

Hopefully this helps.

[nguerrera]

This is what I was asking about:

ref/
    any/
        _._
    nestandard2.0/
       netstandard.dll

It sounds like it wouldn't work as I'd hoped, though.

[ericstj]

No, any is the very last thing that NuGet looks at, after netstandard and others. All you do by putting a placeholder in any is make your package install everywhere as a noop.

[nguerrera]

I think subtracting would be easier so that when the refs are kept they have exactly the representation as any other nuget ref and show up nicely in IDE visualization. Otherwise, you have to duplicate how things look when they're raised from the assets file in the usual way.

I think it might be similar to conflict resolution where conflicting items are discarded, but I admit not having studied that closely yet.

[ericstj]

Downside of subtraction is you end up paying a lot for flowing the unneeded items through the project. Conflict resolution is only OK because at the limit we won't have conflicts. I really don't want to land in a place where we're always subtracting.

[srivatsn]

CPS has implemented reloading of targets so the repro above works. However the assemblies node showing in the solution explorer is still ugly. We should find a way to fix that by atleast making those assemblies show up under the Netstandard.library package. @natidea @tmeschter - is there any metadata that we can add on these ResolvedReference items added by the package to tell the dependency node that it's coming from the netstandard.library package?

[ericstj]

Let me know what you recommend we do to hide these. I agree they should be hidden.

I can move them to a target if that's preferred. I've tried adding both <InProject>False</InProject> and <Visible>False</Visible> to the items but they still appear in solution explorer.

[terrajobst]

I agree with @ericstj. I'd rather we don' show the assembly list at all. If we want to allow the customer browsing them, I'd rather we only show non-facade assemblies (i.e. only netstandard.dll).

Is there a way to make them invisible in the UI by metadata?

[davkean]

Before we figure out how to hide these items, will this package or something like it ever be _included_ in desktop/legacy projects? If so, then this design needs to change. We will never reload targets in those projects while they still run on the old project system.

[ericstj]

This package doesn't provide any references when installed in legacy projects. It only provides references when installed in a netstandard project. So the behavior in legacy projects is static and doesn't depend on retargeting. The only case this changes on retargeting is when installed in a netstandard project.

We have one other package (NETStandard.Library.NETFramework) that adds references through targets files but does so inside a target that runs as part of ResolveReferences (similar to how nuget itself adds the targets). In that case the set of targets imported would change when the target framework changes. We do that because we're trying to bootstrap off of NuGet's TFM selection algo rather than to approximate it within a single static target.

Does this even matter, isn't legacy synonymous with packages.config and packages.config didn't even support retargeting?

Let me know the specific scenario you're worried about and I can test for it.

[davkean]

@ericstj Packages.config isn't a concern because these targets are pushed into the project file itself. You need to worry about PackageRef and legacy, any usage of the new nuget.g.prop/nuget.g.targets in legacy will always stick around until you restart VS, unfortunately.

[davkean]

Basically when I uninstall this package - the references should disappear during design-time builds. "Real" builds will always use the correct results, design-time builds however, run in-proc and will always see the targets files.

[ericstj]

Can you please be more specific about legacy: How do I get a legacy project using PackageRef? I think I'm confused on your terminology.

In my mind I associate PackageRef with NETCore.SDK since that's the thing that implements raising the items and in my mind all NETCore.SDK projects are new projects, but I get the feeling I'm missing something.

[davkean]

<PackageReference/> is "supported" in legacy projects since VS 2017.1, UWP is opt'ing into it, and users can turn it on via the NuGet options in VS.

[ericstj]

I see I'll test that scenario for our other package and will follow up with bugs if we have them.

In the meantime, this specific package still has a scenario for adding references via targets that we don't want to show in the solution explorer. I can accomplish that by getting them out of static evaluation and into a target. Is that best practice, or do you have metadata that can hide them?

[davkean]

These references shouldn't be hidden, they should be associated with the package so they are grouped under that. @natidea can you please figure out the correct metadata to apply to these to show them under the right package?

[ericstj]

I think we are ok with either solution. I agree grouping under the package is what folks would expect. Whatever pattern we determine should be the same thing we recommend to anyone else that needs to do this.
When I discussed it with @nguerrera and @dsplaisted the other day they thought that might be difficult and if that's the case we'd probably be ok with just hiding.

[nguerrera]

My concern isn't difficulty, it's fragility. I see perf issues with the way these assets are raised to items today and I want there to be fewer things that have to change if we slim it down.

[ericstj]

Yep, ^^ this ^^. Sorry to misrepresent.

[ericstj]

Need an answer for this TODAY as we're locking down on the source for this package.

[srivatsn]

@natidea is on vacation. @tmeschter can you please look into this today. @abpiskunov if you know off the top of you head please let us know.

[tmeschter]

@srivatsn As yet I haven't identified the metadata that would cause the assembly to show up under a package.

Also, the tree doesn't have a _package_ representing NETStandard.Library; rather, it has an _SDK_. It's not clear to me if we can surface assemblies directly under an SDK.

[tmeschter]

@srivatsn @ericstj Here's where we are with this. The short answer is that there doesn't appear to be a simple way to do this.

There are basically two parts to making this work as desired.

1. Don't show these assemblies under the "Assemblies" node. I have yet to figure out how we control which assemblies do show up under here, and which don't. For example, if I add the Newtonsoft.Json package to the project it's assemblies don't show up here, but I can't figure out why.

2. Showing the assemblies under some node representing the NETStandard.Library. There's no metadata we can add to the Reference items for these assemblies to make this possible, as the relevant tasks/targets in MSBuild don't even look at Reference items. It may be possible to create a parallel set of items with the proper metadata, but I don't understand the system well enough to describe what would need to be created. Further, we have an SDK node for NETStandard.Library, not a package node, and as yet I don't know if there is any way to make assemblies show up under an SDK.

[tmeschter]

@ericstj About the only thing I can suggest right now is to add some metadata like <Visible>False</Visible> to the Reference items and we'll find a way to update the project system to hide those from the user.

[terrajobst]

From an experience point, I'd be OK with that.

[nguerrera]

If hiding is OK, I think we can get the same effect by moving them from static evaluation to a target, right?

[tmeschter]

@nguerrera I don't think that will do it. We use both evaluation and design-time builds to populate the Dependencies.

[ericstj]

Acutally, I found that if I move it to a target that runs after ResolveLockFileReferences it works. I don't see the assembly node, but I do still get intellisense and build works fine.

[ericstj]

For example

<Target Name="ImplicitlyExpandNETStandardReferences"
          AfterTargets="ResolveLockFileReferences">
  <ItemGroup>
    <Reference Condition="'$(_NetStandardLibraryRefPath)' != ''" Include="$(_NetStandardLibraryRefPath)*.dll">
      <Private>false</Private>
      <NuGetPackageId>NETStandard.Library</NuGetPackageId>
      <NuGetPackageVersion>$(NETStandardLibraryPackageVersion)</NuGetPackageVersion>
    </Reference>
    <ReferenceCopyLocalPaths Condition="'$(_NetStandardLibraryLibPath)' != ''" Include="$(_NetStandardLibraryLibPath)*.dll">
      <Private>false</Private>
      <NuGetPackageId>NETStandard.Library</NuGetPackageId>
      <NuGetPackageVersion>$(NETStandardLibraryPackageVersion)</NuGetPackageVersion>
    </ReferenceCopyLocalPaths>
  </ItemGroup>
</Target>

[tmeschter]

@ericstj Having looked at this for most of the day, I can't explain why that would work. That's very... scary.

[ericstj]

Let me double check. Doing this with the preview build if that makes a difference.

[nguerrera]

I think a deliberate visible=false + project system follow up is better then.

[ericstj]

Ok, deleting the .vs folder and reloading the solution brings the Assemblies node back. Something about moving them from static -> target confused the project system, but they do come back after clearing things out.

[ericstj]

Ok, do we want Visible or InProject? I think InProject is what worked before but I think Visible makes more sense.

[davkean]

We don't want to hide them, makes it harder for user to understand where references are coming from, can't Search, etc

Sent from my Windows 10 phone

From: Nick [e-mail]
Sent: Saturday, May 27, 2017 8:38 AM
To: dotnet/[e-mail]
Cc: David [e-mail]; [e-mail]
Subject: Re: [dotnet/project-system] Netstandard 2.0 projects show an assemblies node with all the facades (#2132)

I think a deliberate visible=false + project system follow up is better then.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fproject-system%2Fissues%2F2132%23issuecomment-304402124&data=02%7C01%7CDavid.Kean%40microsoft.com%7C6bd3c4f62e774869239b08d4a487f1ec%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636314351166135112&sdata=RFOoF78o%2BY7fWkVnN%2BNhXrHm2KNuiOEv7Q3bxMv6BCM%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABDYItIKSk_RnYrcmFXs7bI7qyG4OOT7ks5r91RpgaJpZM4NSaqY&data=02%7C01%7CDavid.Kean%40microsoft.com%7C6bd3c4f62e774869239b08d4a487f1ec%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636314351166135112&sdata=X9icAWVu2FhMYCbGtcvolQ8PseI57QSZ%2F7TK67Xz%2Bzk%3D&reserved=0.

[nguerrera]

Visible since afaik cps does away with hiding items from imports.

[nguerrera]

I think hiding is the lesser evil than replicating the asset raising targets. I'm still worried about the fragility of that.

[nguerrera]

To not hide project system should support a simple metadata contract saying show this Reference under that NuGet package. What I don't want to happen is for Eric to generate the huge set of FileDefinition items that asset raising does.

[terrajobst]

We don't want to hide them, makes it harder for user to understand where references are coming from, can't Search, etc

Actually I would. Most of the assemblies are facades and don't provide any types. I've found that developers are very confused why they see both mscorlib and netstandard. I'd rather they are collapsed to a single node, similar to how UWP/Portable used to work. Arguably that node is the SDK.

[davkean]

We want users to trust results in dependency node, if we hide majority of their framework assemblies, how can the do that?

Sent from my Windows 10 phone

From: Nick [e-mail]
Sent: Saturday, May 27, 2017 8:43 AM
To: dotnet/[e-mail]
Cc: David [e-mail]; [e-mail]
Subject: Re: [dotnet/project-system] Netstandard 2.0 projects show an assemblies node with all the facades (#2132)

I think hiding is the lesser evil than replicating the asset raising targets. I'm still worried about the fragility of that.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fproject-system%2Fissues%2F2132%23issuecomment-304402761&data=02%7C01%7CDavid.Kean%40microsoft.com%7C98839dda16584470840708d4a488a7e6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636314354219085888&sdata=%2FNfn7blBoxNmkAbXvNr%2FMegkRAeqSP5HppX%2F5FnDWvE%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABDYIlmIdFB_q_RQogcQMjRAytD2sEY3ks5r91WagaJpZM4NSaqY&data=02%7C01%7CDavid.Kean%40microsoft.com%7C98839dda16584470840708d4a488a7e6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636314354219085888&sdata=cKs2ouEYBpyAc02PdxkGGFc3JDkOz9OndJnwfORUw38%3D&reserved=0.

[nguerrera]

Instead of Visible=false change, can project system follow up by honoring this current code?

      <NuGetPackageId>NETStandard.Library</NuGetPackageId>
      <NuGetPackageVersion>$(NETStandardLibraryPackageVersion)</NuGetPackageVersion>

I want the way for custom targets to do this to be as minimal as possible.

[terrajobst]

We want users to trust results in dependency node, if we hide majority of their framework assemblies, how can the do that?

I think it depends on the mental model you have. If you think of the platform as something you reference pieces of (.NET Framework) or when you compose your target entirely (.NET Core 1.x, node.js) seeing the assemblies makes a ton of sense. But if you think of the platform as an atomic unit you implicitly reference (UWP/Portable) then being confronted with a ton of items you have no control over doesn't make sense. I'd argue the story we tell customers is that .NET Standard is a set of APIs that are always there and are expressed in a single assembly. Seeing 60+ assemblies isn't telling that story very well.

[terrajobst]

I should add that I'd love to see netstandard.dll as that's the thing that actually contains the APIs and thus is a likely candidate for right click | View In Object Browser.

[terrajobst]

@nguerrera I'm not sure what you're proposing. Are you saying the project system should show the package instead of / in addition to the SDK?

[nguerrera]

No, I'm assuming the project system knows which package it's rendering as an SDK. I'm proposing that if a Reference item claims to belong to a package with just some simple metadata that the project system should support that. And if the claimed package is the SDK, then show it there.

[ericstj]

I was poking around to see how this works and see
https://github.com/dotnet/sdk/blob/67fd3a133eb052fe396377b09b291cf32cd88212/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets#L171-L243

I suspect regular references are a seperate thing and any code to correlate would need to be in the project-system. Alternatively I could probably muck with the _DependenciesDesignTime item but I don't think anyone wants me to do that.

[srivatsn]

I think you should just add metadata like <Facade>true</Facade> for the assemblies that are facades. The NuGetPackageId is set already anyway. We need to make the dependencies node parent these assemblies under the SDK package and then for the ones that are facades we can either not show them at all or show with a different icon or group them separately or something else but the package simply declares that these are different from netstandard.dll

[ericstj]

There have been many suggestions here, let me try to summarize.

Today we add references and set NuGetPackageId and NuGetPackageVersion. Either the SDK or project system will make a change so that these get nested in the right place in the solution explorer without any further metadata/items from our packages.

Further, there has been a suggestion that facades be de-prioritized in the UI. To facilitate that, anything that is a pure facade should have <Facade>true</Facade> metadata added to it.

@srivatsn / @terrajobst is that correct?

Sounds like we need to file a few issues for these, @srivatsn would you like me to do that?

[srivatsn]

Yes that sounds correct to me. We can use this issue to track the nesting under the SDK node based on the NuGetPackageId since doing that solves the issue mentioned in the title. Please file an issue to visually de-prioritize facades.

[ericstj]

@srivatsn I assume that should go in the project-system repo?

[srivatsn]

Yes