Skip to content

Graph cross targeting take two#4218

Merged
cdmihai merged 16 commits intodotnet:masterfrom
cdmihai:graph2
Mar 22, 2019
Merged

Graph cross targeting take two#4218
cdmihai merged 16 commits intodotnet:masterfrom
cdmihai:graph2

Conversation

@cdmihai
Copy link
Copy Markdown
Contributor

@cdmihai cdmihai commented Mar 7, 2019

Reimplementation of #4143 to address its issues by adding explicit nodes for inner builds instead of trying to hide their complexity away. It does so by imposing a contract on what crosstargeting means, and requiring the sdk to be self-descriptive about its crosstargeting structure (the property name of the inner build (e.g. TargetFramework), and the property name of the inner build values (e.g. TargetFrameworks)).

  • no more races on inner builds. They get deduped in the graph.
  • inner builds can have non overlapping references
  • inner builds can have arbitrary incoming edges
  • clear path forward on how to filter the graph per TargetFramework. Right now if project A depends on crosstargeting project B, all of Bs innerbuilds will be added as speculative edges of A. This is no different from building a solution today, which builds all TFs. If one wants to filter, then they have to flow the desired TF through the graph (using Nuget's GetNearestTF) and clip out the unused speculative edges.

Future PRs:

  • update documentation
  • tag nodes and edges of interest with extra metadata: outer builds, inner builds, speculative edges
  • refactorings, better inner build detection to avoid overbuilding some targets

@cdmihai
Copy link
Copy Markdown
Contributor Author

cdmihai commented Mar 7, 2019

cc @Erikma @dannyvv @yash256

Comment thread ref/Microsoft.Build/net/Microsoft.Build.cs Outdated
@AndyGerlicher
Copy link
Copy Markdown
Contributor

Looks good! Only think I see is the ToDot which the more I think about it the more I think it should go.

Comment thread src/Shared/UnitTests/TestEnvironment.cs Outdated
Comment thread src/Tasks/Microsoft.ManagedLanguages.targets Outdated
Comment thread src/Tasks/Microsoft.ManagedLanguages.targets Outdated
@cdmihai cdmihai force-pushed the graph2 branch 2 times, most recently from f426692 to d28acc0 Compare March 12, 2019 17:56
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts. Haven't looked at the hard parts (the C# code changes) yet.

Comment thread src/Shared/UnitTests/TestEnvironment.cs
Comment thread src/Tasks/Microsoft.CSharp.targets Outdated
Comment thread src/Tasks/Microsoft.ManagedLanguages.targets
Comment thread src/Tasks/Microsoft.ManagedLanguages.targets
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed tests. Several questions about graphs that did not match my expectations. Perhaps the reasons will emerge as I look at the implementation (next up).

Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs
Comment thread src/Build.UnitTests/Graph/ProjectGraph_Tests.cs Outdated
Comment thread src/Shared/Constants.cs Outdated
Comment thread src/Tasks.UnitTests/Copy_Tests.cs Outdated
Comment thread src/Build/Graph/ProjectInterpretation.cs
Comment thread src/Build/Graph/ProjectInterpretation.cs
Comment thread src/Build/Graph/ProjectInterpretation.cs
@rainersigwald
Copy link
Copy Markdown
Member

Can you add some negative tests? Like "doesn't provide the correct metadata to understand the inner build" -- what's the failure look like? InvalidProjectFileException?

Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, my review is complete. Many comments, but the big conceptual thing I have is about the edge elision/promotion around non-graph-root outer builds.

Comment thread src/Build/Graph/ProjectInterpretation.cs Outdated
Comment thread src/Build/Graph/ProjectInterpretation.cs Outdated
Comment thread src/Build/Graph/ProjectGraph.cs
Comment thread src/Tasks/Microsoft.ManagedLanguages.targets
Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending filing the overbuild bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants