Redesign dependencies tree to support multiple targets. #1880

abpiskunov · 2017-03-28T23:47:44Z

What changed:

Instead of subscribing to active configuration for DT build events to get dependencies we use aggregation similar to Language services and subscribe to all configured projects to get DT build events.
Difference with Language Services is that we need to subscribe to heterogeneous events and thus needed an extra layer of subscribers (ICrossTargetSubscriber) that can subscribe to a list of ConfiguredProjects and subscribers know what type of events they can subscribe (example DT Rules vs SharedProjects which are solution folders).
I abstracted out all aggregate subscribtions logic and kept dependencies objects separate, deriving on abstractions. This would simplify things if we would like to merge LanguageServices and Dependencies aggregation logic in the future - (I would consider that to have one implementation).
All cross targeting logic is in Dependencies\CrossTarget folder
All Dependencies specific implementations for cross-target abstractions is under Dependencies\Subscriptions folder.
Instead of "on-demand" model where we queried children for each dependency, we switch to snapshot model. Snapshot is immutable world of all dependencies for all targets for each projects. This simplifies synchronization of dependencies across projects. All snapshot related objects are under Dependencies\Snapshot folder
Logic where groups and top level nodes are IProjectTree nodes and all lower levels are provided by GraphProvider is still in effect.
Instead of hardcoded DependenciesProjectTreeProvider logic for Dependencies node creation we now have delegating it to IDependenciesTreeViewProvider (s) which know how to construct Dependencies node based on given Snapshot. There could be many IDependenciesTreeViewProviders and we could have commands that allow user switch between how we display Dependencies node, for example groupped by target or by dependency type. For now I implemented only GrouppedByTargetTreeViewProvider which adds target framework nodes under Dependencies node when project has several targets. If there is only one target it skips target framework node. There is also a way to display nodes that don't care about targets at the Dependencies root , for example wbe client packages - they would have special target "all" in the Snapshot.
To allow comparisons and display of friendly framework names I created a global component ITargetFrameworkProvider , that uses NuGet API to compare and get target info by short or full tfm.
External providers (IProjectDependenciesSubTreeProvider) still can be provided for custom dependencies groups - web client packages will be still doing that. Some public API changed and they also will provide snapshots instead of on-demand data.
There are several kind of models for dependencies: IDependencyModel is public and used for data provided by subscribers or IProjectDependenciesSubTreeProviders , which then is converted to internal IDependency which is stored in snapshots. View providers and graph provider use IDependencyViewModels to display actual nodes , which is obtained from IDependency (this is needed since view could have different properties depending on IDependency state)
Worked with designers to come up with complete icons set for all dependencies types and we have several states:
○ Normal
○ Broken - icon with yellow sign
○ Implicit icon with grey lock (implicit dependencies cannot be deleted and some other things might be restricted, thus we need visual difference)
Icons are shipped as resources and registered by ManagedImages.imagemanifest, which ships with vsix (in order to dogfood my change and see icons, you need to patch your dev machine build dlls and copy them along with images manifest under real VS managed extensions path . Then close VS , remove ImagesCache (under vs hive fodlder in AppData\Microsoft\VisualStudio and run devenv /setup. After that it would work in your dev hive too.) Then you could revert real assemblies in your real VS folder.
By the nature of the change it fixes some open issues also: Bubble up error sign to higher level nodes if some child is broken; avoid flicks in dependency nodes when they are updated etc. I will go through issues list after PR is done and update them.
Old unit tests are commented out at the moment and I am adding new ones, will add them to the PR along the way.

abpiskunov · 2017-03-28T23:51:23Z

@dotnet/project-system @BillHiebert @natidea

tmeschter · 2017-03-29T21:18:48Z

When you state:

Logic where groups and top level nodes are IProjectTree nodes and all lower levels are provided by GraphProvider is still in effect.

Does that mean we end up with this, for example:

- ConsoleApp1 (ProjectTree)
  - Dependencies (ProjectTree)
    - .NETFramework 4.5.2 (ProjectTree)
      - Analyzers (ProjectTree)
          StyleCop.Analyzers (1.0.1) (ProjectTree)
      - NuGet (ProjectTree)
        - StyleCop.Analyzers (1.0.1) (ProjectTree)
            Newtonsoft.Json.dll (Graph)
            StyleCop.Analyzers.CodeFixes.dll (Graph)
            StyleCop.Analyzers.dll (Graph)

I ask because the work I'm doing to add nodes for diagnostics under individual analyzers will be much easier if the items under "Analyzers" continue to be represented with IProjectTree items.

abpiskunov · 2017-03-29T21:49:32Z

@tmeschter yes , sample you posted is correct. If you really want to do that, we could do a change: have a special flag for your provider "SupportsCPSNodesHierarchy" and when our TreeViewProvider see this flag for root node, it would fill all levels of hierarchy for your provider right away as IProjectTree and later GraphProvider would ignore your nodes. However, notice that we switched to GraphNodes since large number of IProjectTRees behaved very slow - in your case you probably would have relatevily small number of analyzers in most cases, but if you would have 1000 of them it would slow down whole tree.

tmeschter · 2017-04-03T20:14:44Z

...jectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/GrouppedByTargetTreeViewProvider.cs

+    [Export(typeof(IDependenciesTreeViewProvider))]
+    [AppliesTo(ProjectCapability.DependenciesTree)]
+    [Order(Order)]
+    internal class GrouppedByTargetTreeViewProvider : TreeViewProviderBase


tmeschter · 2017-04-03T20:15:36Z

...rojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/GrouppedByTypeTreeViewProvider.cs

+    [Export(typeof(IDependenciesTreeViewProvider))]
+    [AppliesTo(ProjectCapability.DependenciesTree)]
+    [Order(Order)]
+    internal class GrouppedByTypeTreeViewProvider : TreeViewProviderBase


Was this IDependenciesTreeViewProvider included by mistake?

i was going to implement it too and had a right click command to swicth views, but i will cut it for now. Will remove this provider

tmeschter · 2017-04-03T21:07:31Z

Is the target framework information exposed externally? That is, if I have an IVsHierarchy and item ID for the ".NETFramework 4.6" node I can query for the __VSHPROPID7.VSHPROPID_ProjectTreeCapabilities property and find out that it has the "TargetNode" capability. Is there some way to then determine which target framework it represents?

I ask because I'm planning on refactoring the code that adds nodes for individual diagnostics under analyzers to support both the old and new project systems, and it is implemented via IAttachedCollectionSource. So it needs to operate over IVsHierarchy without taking a dependency on project-system-specific types.

abpiskunov · 2017-04-03T22:42:19Z

@tmeschter no there no way to get information about target framework at the moment, only it's Caption/name from the IVsHierarchy item. How do you want to pass that info if we would have to send it?

tmeschter · 2017-04-04T16:25:28Z

@abpiskunov I can see two options that may work:

Expose the target framework moniker (or whatever it is called nowadays; whatever we use to uniquely identify a target framework) through IProjectTree.BrowseObjectProperties. Externally, this will be exposed through the IVsHierarchy via the __VSHPROPID.VSHPROPID_BrowseObject property.
Expose the TFM as a specially-formatted string exposed through IProjectTree.Flags, like $TFM:netstandard1.4 or similar. Externally these are surfaced through the __VSHPROPID7.VSHPROPID_ProjectTreeCapabilities property. This depends on the TFM not having any spaces in it.

The latter is probably easier to do, and I've already got the list of capabilities in order to look for the "TargetNode" capability.

Note I'm not suggesting you make this change now. If you think one of these approaches is workable I'll make the change myself after you get this merged in.

abpiskunov · 2017-04-04T16:36:18Z

I was thinking about option 2. I will apply this flag in my PR, so you had it when i checkin.

tmeschter

I haven't gone through the change with a fine-toothed comb, but I think I understand the overall approach and that looks good to me. At this point I think we'll make more progress by getting this merged in so we can try it out than we will by picking apart the details via code review.

Obviously, I do want to see the tests up and running as well, but I think even that could come in a later change.

natidea

Reported a few issues from trying out the build, but this is good enough for dogfooding at this point

What changed: - Instead of subscribing to active configuration for DT build events to get dependencies we use aggregation similar to Language services and subscribe to all configured projects to get DT build events. Difference with Language Services is that we need to subscribe to heterogeneous events and thus needed am extra layer of subscribers (ICrossTargetSubscriber) that can subscribe to a list of ConfiguredProjects and subscribers know what type of events they can subscribe (example DT Rules vs SharedProjects which are solution folders). I abstracted out all aggregate subscribtions logic and kept dependencies objects separate, deriving on abstractions. This would simplify things if we would like to merge LanguageServices and Dependencies aggregation logic in the future - (I would consider that to have one implementation). All cross targeting logic is in Dependencies\CrossTarget folder - All Dependencies specific implementations for cross-target abstractions is under Dependencies\Subscriptions folder. - Instead of "on-demand" model where we queried children for each dependency, we switch to snapshot model. Snapshot is immutable world of all dependencies for all targets for each projects. This simplifies synchronization of dependencies across projects. All snapshot related objects are under Dependencies\Snapshot folder - Logic where groups and top level nodes are IProjectTree nodes and all lower levels are provided by GraphProvider is still in effect. - Instead of hardcoded DependenciesProjectTreeProvider logic for Dependencies node creation we now have delegating it to IDependenciesTreeViewProvider (s) which know how to construct Dependencies node based on given Snapshot. There could be many IDependenciesTreeViewProviders and we could have commands that allow user switch between how we display Dependencies node, for example groupped by target or by dependency type. For now I implemented only GrouppedByTargetTreeViewProvider which adds target framework nodes under Dependencies node when project has several targets. If there is only one target it skips target framework node. There is also a way to display nodes that don't care about targets at the Dependencies root , for example wbe client packages - they would have special target "all" in the Snapshot. - To allow comparisons and display of friendly framework names I created a global component ITargetFrameworkProvider , that uses NuGet API to compare and get target info by short or full tfm. - External providers (IProjectDependenciesSubTreeProvider) still can be provided for custom dependencies groups - web client packages will be still doing that. Some public API changed and they also will provide snapshots instead of on-demand data. - There are several kind of models for dependencies: IDependencyModel is public and used for data provided by subscribers or IProjectDependenciesSubTreeProviders , which then is converted to internal IDependency which is stored in snapshots. View providers and graph provider use IDependencyViewModels to display actual nodes , which is obtained from IDependency (this is needed since view could have different properties depending on IDependency state) - Worked with designers to come up with complete icons set for all dependencies types and we have several states: ○ Normal ○ Broken - icon with yellow sign ○ Implicit icon with grey lock (implicit dependencies cannot be deleted and some other things might be restricted, thus we need visual difference) Icons are shipped as resources and registered by ManagedImages.imagemanifest, which ships with vsix (in order to dogfood my change and see icons, you need to patch your dev machine build dlls and copy them along with images manifest under real VS managed extensions path . Then close VS , remove ImagesCache (under vs hive fodlder in AppData\Microsoft\VisualStudio and run devenv /setup. After that it would work in your dev hive too.) Then you could revert real assemblies in your real VS folder. - Old unit tests are commented out at the moment and I am adding new ones, will add them to the PR along the way.

- register IDependenciesSnapshotProviders in aggregate global provider upfront so dependent projects could receive notifications in time to update their project dependencies - instead of remove and add changes , just add them , to avoid incorrect removeals when evaluation change events are happening after restore - add a filter to prevent "unresolved" rules override "resolved" ones. "resolved" rules can be deleted only when explicit remove change comes for "resolved" rule.

…ree node

…ider updates project path when project is renamed. All existing snapshots should be updated when corresponding DT build event comes with new project name.

…e providers for web

…Model equality comparison

What changes is the way we discover Sdk references. Now we have support for cross-targeting DT builds for dependencies and can run sdk targetes for each target in DT build. This simplified things since we don't have to call ResolvePackageDependenciesDesignTime to produce flat list accross all targets, but rely on packages evaluated for given target. Project system would know how to map sdk to corresponding package for given target.

…me properties were missing including targetPath. Fix was to avoid unnecessary properties dependency.

srivatsn · 2017-04-06T21:08:47Z

Hey @abpiskunov adding commits to this PR only make it more unreviewable. Can you please send any additional changes as separate PRs after merging this in so that they can be reviewed separately?

… sdk nodes readded when package macth found, to ensure refresh in the tree.

srivatsn · 2017-04-06T23:53:46Z

I have no idea what the latest commit does and this PR is already ridiculously large. Please don't add more commits to it.

abpiskunov · 2017-04-06T23:59:04Z

@srivatsn last change is to avoid unnecessary tree update when project is unloading and second is to make sure sdk nodes are removed and then readded when we find a package which dependencies need to appear under sdk. Node will be readded if it changes it's state form resolved to unresolved or vise-versa , to allow Graph provider to apply hierarchy changes. If we just update node in place CPS does not notify GraphProvider.

I was just testing changes while adding unit tests - not really changing anything dramatically, just tuneup for small corner cases. Anyway, will check in tomorrow and then finish unit tests in new PRs

What changes is the way we discover Sdk references. Now we have support for cross-targeting DT builds for dependencies and can run sdk targetes for each target in DT build. This simplified things since we don't have to call ResolvePackageDependenciesDesignTime to produce flat list accross all targets, but rely on packages evaluated for given target. Project system would know how to map sdk to corresponding package for given target.

dnfclas added the cla-already-signed label Mar 28, 2017

abpiskunov mentioned this pull request Mar 28, 2017

Show friendly names for target framework in project properties #1595

Closed

tmeschter reviewed Apr 3, 2017

View reviewed changes

abpiskunov mentioned this pull request Apr 4, 2017

Add TFMs to top level package properties grid (partial fix for 587 for RTM) #1515

Closed

tmeschter approved these changes Apr 4, 2017

View reviewed changes

natidea approved these changes Apr 4, 2017

View reviewed changes

Anton Piskunov and others added 9 commits April 5, 2017 14:57

Add comments and first unit tests for dependencies snapshot logic

5e62079

Add unit tests and comments for snapshot filters

8e601ed

Add more unit tests for snapshot and dependencies tree view provider

a350da9

Add unit tests for models and target framework as a flag for Target t…

73cc48b

…ree node

Tune up project renaming scenario, make sure aggregated snapshot prov…

d9890ba

…ider updates project path when project is renamed. All existing snapshots should be updated when corresponding DT build event comes with new project name.

Add minor changes and unit tests updates to accomodate custom sub tre…

8334f78

…e providers for web

Minor changes for better diagnostics; add provider type to Dependency…

0bb709b

…Model equality comparison

abpiskunov mentioned this pull request Apr 6, 2017

Matching change for project system PR https://github.com/dotnet/proje… dotnet/sdk#1090

Merged

Fix issue 1899, since that project had some non-standard imports , so…

76adb1e

…me properties were missing including targetPath. Fix was to avoid unnecessary properties dependency.

Add cancellation logic in tree view when project unloading and ensure…

4aeba2f

… sdk nodes readded when package macth found, to ensure refresh in the tree.

abpiskunov merged commit 8d2fea3 into dotnet:master Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign dependencies tree to support multiple targets. #1880

Redesign dependencies tree to support multiple targets. #1880

abpiskunov commented Mar 28, 2017 •

edited by srivatsn

Loading

abpiskunov commented Mar 28, 2017

tmeschter commented Mar 29, 2017 •

edited

Loading

abpiskunov commented Mar 29, 2017

tmeschter Apr 3, 2017

abpiskunov Apr 3, 2017 •

edited

Loading

tmeschter Apr 3, 2017

abpiskunov Apr 3, 2017

tmeschter commented Apr 3, 2017 •

edited

Loading

abpiskunov commented Apr 3, 2017

tmeschter commented Apr 4, 2017 •

edited

Loading

abpiskunov commented Apr 4, 2017

tmeschter left a comment

natidea left a comment

srivatsn commented Apr 6, 2017

srivatsn commented Apr 6, 2017

abpiskunov commented Apr 6, 2017

Redesign dependencies tree to support multiple targets. #1880

Redesign dependencies tree to support multiple targets. #1880

Conversation

abpiskunov commented Mar 28, 2017 • edited by srivatsn Loading

abpiskunov commented Mar 28, 2017

tmeschter commented Mar 29, 2017 • edited Loading

abpiskunov commented Mar 29, 2017

tmeschter Apr 3, 2017

Choose a reason for hiding this comment

abpiskunov Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

tmeschter Apr 3, 2017

Choose a reason for hiding this comment

abpiskunov Apr 3, 2017

Choose a reason for hiding this comment

tmeschter commented Apr 3, 2017 • edited Loading

abpiskunov commented Apr 3, 2017

tmeschter commented Apr 4, 2017 • edited Loading

abpiskunov commented Apr 4, 2017

tmeschter left a comment

Choose a reason for hiding this comment

natidea left a comment

Choose a reason for hiding this comment

srivatsn commented Apr 6, 2017

srivatsn commented Apr 6, 2017

abpiskunov commented Apr 6, 2017

abpiskunov commented Mar 28, 2017 •

edited by srivatsn

Loading

tmeschter commented Mar 29, 2017 •

edited

Loading

abpiskunov Apr 3, 2017 •

edited

Loading

tmeschter commented Apr 3, 2017 •

edited

Loading

tmeschter commented Apr 4, 2017 •

edited

Loading