Provide ResolvedFrameworkReferences for VS to populate Solution Explorer #3355

dsplaisted · 2019-06-25T22:37:30Z

No description provided.

dsplaisted · 2019-06-25T23:16:20Z

/azp run

azure-pipelines · 2019-06-25T23:16:24Z

Pull request contains merge conflicts.

…n.targets

Add ResolveFrameworkReferences target, rename ResolveTargetingPacks to ResolveTargetingPackAssets

Test case where targeting packs need to be downloaded

dsplaisted · 2019-06-26T03:57:33Z

@drewnoakes @davkean Take a look at this PR and let me know if this matches what you were expecting based on our discussion.

davkean · 2019-06-26T04:30:29Z

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets

+      ResolvedTargetingPacks="@(ResolvedTargetingPack)"
+      ResolvedRuntimePacks="@(ResolvedRuntimePack)">
+
+      <Output TaskParameter="ResolvedFrameworkReferences" ItemName="ResolvedFrameworkReference" />


What metadata does this item have?

Ah, its here: https://github.com/dotnet/sdk/pull/3355/files#diff-6cfbacaa410e7ee1787b4771b158ded0. It was collapsed.

Yes, this appears to be what we want. We'll also want a ResolveFrameworkReferences in MSBuild proper so that we can call regardless of we're an SDK project or not.

Can you show me an example of one of these and we can decide what properties we want to show up?

davkean · 2019-06-26T04:32:55Z

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets

+    to the assets file, and the ResolvePackageAssets target adds them to the TransitiveFrameworkReference
+    item.  Here, we add them to FrameworkReference if they aren't already referenced.
+    ============================================================
+    -->
  <Target Name="AddTransitiveFrameworkReferences" AfterTargets="ResolvePackageAssets"


Will these appear as "ResolvedFrameworkReference"?

Would it be a problem if you end up with ResolvedFrameworkReference items that didn't have a corresponding FrameworkReference when you evaluated? Would you just ignore them or would it be a warning or error?

We will ignore them.

drewnoakes · 2019-06-26T05:14:55Z

This looks great to me.

Seems the metadata on ResolvedFrameworkReference items will be:

OriginalItemSpec
TargetingPackPath
TargetingPackName
TargetingPackVersion
Profile
RuntimePackPath
RuntimePackName
RuntimePackVersion

I'm guessing we show all of these (maybe not OriginalItemSpec). Is that not the case?

The Dependencies node will also need IsImplicitlyDefined so we can control whether the reference can be removed in the UI.

dsplaisted · 2019-06-27T03:30:36Z

Note that the runtime pack metadata won't be set unless the SelfContained property is true and the RuntimeIdentifier is set. So I'm not sure whether it's a good idea to show those properties in some cases but not others, or to just never show them at all.

davkean · 2019-06-27T03:40:43Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs

-                        packagesToDownload.Add(packageToDownload);
-                    }
-                }
+                TaskItem resolvedFrameworkReference = new TaskItem(frameworkReference.ItemSpec);


We're expecting metadata (including IsImplicitlyDefined/PrivateAssets) to be copied from the original item -> resolved item, I think (?) this is what ResolveAssemblyReferences does.

I would expect IsImplicitlyDefined to determine whether you can delete the reference from the solution tree, but what would you do with PrivateAssets?

It shows up in the Properties page so that you can flip it.

I'm not sure I consider PrivateAssets on FrameworkReference items a user-facing feature. We use it to communicate that an implicit FrameworkReference is represented by the target framework, and thus doesn't need to flow transitively. I don't think there's a scenario for setting it on other FrameworkReferences, and if it shows up in the UI it's more likely to cause confusion and incorrect builds if people set it.

You might have a reference to a project with a direct FrameworkReference and not want to inherit its FrameworkReference.

drewnoakes · 2019-06-27T03:42:39Z

Currently assuming the properties are:

FrameworkReference
- PrivateAssets
- IsImplicitlyDefined (not visible)
ResolvedFrameworkReference
- OriginalItemSpec
- TargetingPackPath
- TargetingPackName
- TargetingPackVersion
- Profile
- PrivateAssets
- IsImplicitlyDefined (not visible)

davkean · 2019-06-27T03:44:11Z

Can we have an example of these values? What exactly will TargetingPackName/Version give us that the path won't?

dsplaisted · 2019-06-27T04:43:20Z

I will get examples later, but the name and version will be elements of the path. So they are to some degree redundant.

Also, just because we're setting a given piece of metadata on the ResolvedFrameworkReference items doesn't mean I think the value should necessarily be displayed. It just gives VS the flexibility to decide to display it if we think that's the right thing.

drewnoakes · 2019-06-27T07:46:13Z

Agree. It'd be useful to see examples of these values so we can decide whether to remove any of the above properties.

…e Solution Explorer

…m in solution explorer tree

dsplaisted · 2019-06-27T23:18:25Z

Here's an example of what the ResolvedFrameworkReference items look like now:

ResolvedFrameworkReference
    Microsoft.NETCore.App
        IsImplicitlyDefined = true
        OriginalItemSpec = Microsoft.NETCore.App
        Profile = 
        RuntimePackName = Microsoft.NETCore.App.Runtime.win-x86
        RuntimePackPath = C:\Users\daplaist\.nuget\packages\microsoft.netcore.app.runtime.win-x86\3.0.0-preview7-27814-01
        RuntimePackVersion = 3.0.0-preview7-27814-01
        TargetingPackName = Microsoft.NETCore.App.Ref
        TargetingPackPath = C:\git\dotnet-sdk\.dotnet\packs\Microsoft.NETCore.App.Ref\3.0.0-preview7-27814-01
        TargetingPackVersion = 3.0.0-preview7-27814-01
    Microsoft.AspNetCore.App
        IsImplicitlyDefined = 
        OriginalItemSpec = Microsoft.AspNetCore.App
        Profile = 
        RuntimePackName = Microsoft.AspNetCore.App.Runtime.win-x86
        RuntimePackPath = C:\Users\daplaist\.nuget\packages\microsoft.aspnetcore.app.runtime.win-x86\3.0.0-preview7.19311.5
        RuntimePackVersion = 3.0.0-preview7.19311.5
        TargetingPackName = Microsoft.AspNetCore.App.Ref
        TargetingPackPath = C:\git\dotnet-sdk\.dotnet\packs\Microsoft.AspNetCore.App.Ref\3.0.0-preview7.19311.5
        TargetingPackVersion = 3.0.0-preview7.19311.5
    Microsoft.WindowsDesktop.App
        IsImplicitlyDefined = 
        OriginalItemSpec = Microsoft.WindowsDesktop.App
        Profile = 
        RuntimePackName = Microsoft.WindowsDesktop.App.Runtime.win-x86
        RuntimePackPath = C:\Users\daplaist\.nuget\packages\microsoft.windowsdesktop.app.runtime.win-x86\3.0.0-preview7-27814-01
        RuntimePackVersion = 3.0.0-preview7-27814-01
        TargetingPackName = Microsoft.WindowsDesktop.App.Ref
        TargetingPackPath = C:\git\dotnet-sdk\.dotnet\packs\Microsoft.WindowsDesktop.App.Ref\3.0.0-preview7-27814-01
        TargetingPackVersion = 3.0.0-preview7-27814-01

I'd suggest that the project system have a list of the metadata values to show, instead of a list that shouldn't be shown and showing everything else.

drewnoakes · 2019-06-27T23:30:48Z

I'd suggest that the project system have a list of the metadata values to show, instead of a list that shouldn't be shown and showing everything else.

That's exactly what happens: https://github.com/dotnet/project-system/pull/4928/files#diff-5060c4224c575eb7de55c6c2353a0ac8

Are we sure that RuntimePackName or other runtime properties wouldn't be useful to see in the properties grid?

davkean · 2019-06-27T23:59:29Z

They only show up in certain situations, so it would be confused as to when they are populated vs. not.

nguerrera · 2019-06-28T16:53:16Z

...sks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets

+    Matches FrameworkReference items with KnownFrameworkReference items to determine the corresponding
+    targeting pack and if necessary the runtime pack.  If the packs aren't available in the NetCoreTargetingPackRoot
+    folder, then generate PackageDownload items in order to download the packs during restore.
+


Thank you for adding these. :)

nguerrera · 2019-06-28T16:55:46Z

src/Tests/Microsoft.NET.Build.Tests/DesignTimeBuildTests.cs

@@ -50,6 +50,7 @@ public void The_design_time_build_succeeds_before_nuget_restore(string relativeP
            var projectDirectory = Path.Combine(testAsset.TestRoot, relativeProjectPath);

            var command = new MSBuildCommand(Log, "ResolveAssemblyReferencesDesignTime", projectDirectory);
+            command.WorkingDirectory = projectDirectory;


What happened here?

One of the args creates a binlog, so this ensures it goes to the right folder.

That's not obvious. We should either make it so all tests can write binlogs or revert writing of binlog before committing. I don't want to see this spread.

Also binlog writing is significantly slower and our tests are already slow.

nguerrera · 2019-06-28T16:58:16Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolveFrameworkReferences.cs

+                resolvedFrameworkReference.SetMetadata("TargetingPackPath", targetingPack.GetMetadata(MetadataKeys.Path));
+                resolvedFrameworkReference.SetMetadata("TargetingPackName", targetingPack.GetMetadata(MetadataKeys.PackageName));
+                resolvedFrameworkReference.SetMetadata("TargetingPackVersion", targetingPack.GetMetadata(MetadataKeys.PackageVersion));
+                resolvedFrameworkReference.SetMetadata("Profile", targetingPack.GetMetadata("Profile"));


Naming nit: Should this be TargetingPackProfile?

Thinking about it, I think it makes slightly more sense as Profile than as TargetingPackProfile.

I don't have a strong opinion, but why? The Profile has no meaning at runtime.

…0191029.1 (dotnet#3355) - Microsoft.Build.Localization - 16.4.0-preview-19529-01 - Microsoft.Build - 16.4.0-preview-19529-01

dsplaisted added 4 commits June 25, 2019 16:20

Rename ResolveFrameworkReferences to ProcessFrameworkReferences

a55022e

Rename TargetingPackResolution.targets to FrameworkReferenceResolutio…

b77a8e2

…n.targets

Refactoring FrameworkReferenceResolution.targets

696496a

Add ResolveFrameworkReferences target, rename ResolveTargetingPacks to ResolveTargetingPackAssets

Update designtime tests

cb34a97

Test case where targeting packs need to be downloaded

dsplaisted force-pushed the process-frameworkreferences branch 2 times, most recently from 5062acd to eebffb7 Compare June 26, 2019 01:06

dsplaisted marked this pull request as ready for review June 26, 2019 03:56

davkean reviewed Jun 26, 2019

View reviewed changes

dsplaisted mentioned this pull request Jun 26, 2019

Add stub ResolveFrameworkReferences target dotnet/msbuild#4471

Merged

drewnoakes mentioned this pull request Jun 27, 2019

FrameworkReference support in Dependencies node dotnet/project-system#4762

Closed

4 tasks

drewnoakes mentioned this pull request Jun 27, 2019

FrameworkReference support dotnet/project-system#4928

Merged

7 tasks

davkean reviewed Jun 27, 2019

View reviewed changes

dsplaisted added 3 commits June 27, 2019 16:13

Create ResolvedFrameworkReference items for project system to populat…

90b7faf

…e Solution Explorer

Set metadata on references from targeting packs to enable showing the…

d9e3244

…m in solution explorer tree

Add test for ResolvedFrameworkReferences

c1a7da3

dsplaisted force-pushed the process-frameworkreferences branch from 6e46e4d to c1a7da3 Compare June 27, 2019 23:14

dsplaisted requested a review from nguerrera June 28, 2019 00:55

nguerrera approved these changes Jun 28, 2019

View reviewed changes

dsplaisted merged commit ef7932d into dotnet:master Jun 28, 2019

drewnoakes mentioned this pull request Jul 2, 2019

Show FrameworkReference version in Solution Explorer dotnet/project-system#2248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide ResolvedFrameworkReferences for VS to populate Solution Explorer #3355

Provide ResolvedFrameworkReferences for VS to populate Solution Explorer #3355

dsplaisted commented Jun 25, 2019

dsplaisted commented Jun 25, 2019

azure-pipelines bot commented Jun 25, 2019

dsplaisted commented Jun 26, 2019

davkean Jun 26, 2019

davkean Jun 26, 2019 •

edited

Loading

davkean Jun 26, 2019

davkean Jun 26, 2019

dsplaisted Jun 27, 2019

davkean Jun 27, 2019

drewnoakes commented Jun 26, 2019

dsplaisted commented Jun 27, 2019

davkean Jun 27, 2019

dsplaisted Jun 27, 2019

davkean Jun 27, 2019

dsplaisted Jun 27, 2019

davkean Jun 27, 2019 •

edited

Loading

drewnoakes commented Jun 27, 2019

davkean commented Jun 27, 2019

dsplaisted commented Jun 27, 2019 •

edited

Loading

drewnoakes commented Jun 27, 2019

dsplaisted commented Jun 27, 2019

drewnoakes commented Jun 27, 2019

davkean commented Jun 27, 2019

nguerrera Jun 28, 2019

nguerrera Jun 28, 2019

dsplaisted Jun 28, 2019

nguerrera Jun 28, 2019

nguerrera Jun 28, 2019

nguerrera Jun 28, 2019

dsplaisted Jun 28, 2019

nguerrera Jun 28, 2019

Provide ResolvedFrameworkReferences for VS to populate Solution Explorer #3355

Provide ResolvedFrameworkReferences for VS to populate Solution Explorer #3355

Conversation

dsplaisted commented Jun 25, 2019

dsplaisted commented Jun 25, 2019

azure-pipelines bot commented Jun 25, 2019

dsplaisted commented Jun 26, 2019

Choose a reason for hiding this comment

davkean Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewnoakes commented Jun 26, 2019

dsplaisted commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

drewnoakes commented Jun 27, 2019

davkean commented Jun 27, 2019

dsplaisted commented Jun 27, 2019 • edited Loading

drewnoakes commented Jun 27, 2019

dsplaisted commented Jun 27, 2019

drewnoakes commented Jun 27, 2019

davkean commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davkean Jun 26, 2019 •

edited

Loading

davkean Jun 27, 2019 •

edited

Loading

dsplaisted commented Jun 27, 2019 •

edited

Loading