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

Experiment with enabling msbuild static graph build #41975

Open
ViktorHofer opened this issue Sep 8, 2020 · 17 comments
Open

Experiment with enabling msbuild static graph build #41975

ViktorHofer opened this issue Sep 8, 2020 · 17 comments

Comments

@ViktorHofer
Copy link
Member

The --graph / --graphBuild msbuild CLI option enables the static graph build feature. From discussion with @rainersigwald:

no, Arcade doesn't use it by default
specs at https://github.com/dotnet/msbuild/blob/master/documentation/specs/static-graph.md but they're not good for high-level/motivation

in brief:
MSBuild normally discovers dependencies just in time: a project is building and invokes the MSBuild task to build another project. That works fine, but means we can't do some things like advanced scheduling (because we don't know what's going to be built in advance) and caching at the project level. MSBuild static graph restricts the build to define a graph of ProjectReferences in advance and then builds based on that graph, rather than just-in-time. That can help some builds

it's mostly helpful when combined with a build system that can cache at the project level, like CloudBuild does internally
but it can have some benefits even without that

We should see if enabling this feature reduces our build times and even if not, a predictable static graph is preferred over a just-in-time hard to reason about graph so it might be a good option anyway.

@jaredpar this would be a pre-cursor for using CloudBuild in CI.

cc @dotnet/runtime-infrastructure

@ViktorHofer ViktorHofer added this to the Future milestone Sep 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 8, 2020
@ghost
Copy link

ghost commented Sep 8, 2020

Tagging subscribers to this area: @ViktorHofer
See info in area-owners.md if you want to be subscribed.

@ViktorHofer ViktorHofer changed the title Experiment enabling msbuild static graph build Experiment with enabling msbuild static graph build Sep 8, 2020
@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Sep 8, 2020
@jkoritzinsky
Copy link
Member

The installer layer is likely not compatible with static graph builds today. We'll have to do some work to provide the right information to MSBuild to support static graph.

@ViktorHofer
Copy link
Member Author

Agreed. We already disable static graph for some of the installer projects during restore:

runtime/Build.proj

Lines 40 to 45 in 171ef84

<Target Name="RestoreWithoutStaticGraph"
BeforeTargets="Restore">
<MSBuild Projects="@(DepprojProjectToBuild);@(PkgprojProjectToBuild);@(BundleProjectToBuild)"
Properties="MSBuildRestoreSessionId=$([System.Guid]::NewGuid());RestoreUseStaticGraphEvaluation=false"
Targets="Restore" />
</Target>
.

@jkoritzinsky you mentioned that this will change with your new SDK?

@jkoritzinsky
Copy link
Member

So static graph restore will work with the new SDK, but I don't know if a full static graph build will. I'll need to figure out based on the static graph protocol how to represent the various <MSBuild /> calls in the new sdk.

@ViktorHofer
Copy link
Member Author

That makes sense. We will likely hit similar problems during the libraries builds as we call into msbuild in couple places as well (in example in the TargetFramework.SDK that lives in Arcade).

@rainersigwald
Copy link
Member

There are two levels of static graph:

  1. -graph, which requires that projects have ProjectReferences to build the graph but can handle calls outside it
  2. -isolate, which is required for CloudBuild, and which forbids project-to-project references not known to the graph.

So hopefully the installer stuff will work in -graph mode even if it would fail in -isolate.

@ViktorHofer
Copy link
Member Author

Thanks for the clarification @rainersigwald but doesn't that mean that the SDK itself won't work with -isolate as we invoke ie GetTargetFrameworks via an msbuild task?

@jaredpar
Copy link
Member

jaredpar commented Sep 8, 2020

@rainersigwald

which requires that projects have ProjectReferences to build the graph but can handle calls outside it

Can u elaborate on what this means? Or maybe provide some examples of what would not meet this requirement?

Feel like I've asked this before, wonder if we should start a doc on static graph (cookbook style) that discusses what is / isn't legal here.

@ViktorHofer
Copy link
Member Author

I agree with Jared that further details on what is allowed and what is prohibited in the two modes would help us. Sounds like we should be able to enable -graph?

@rainersigwald
Copy link
Member

@ViktorHofer @jaredpar can I impose on you to check out https://github.com/dotnet/msbuild/pull/5741/files?short_path=570cf46#diff-570cf46aec4396d3da8763436f48defb? Feedback welcome!

doesn't that mean that the SDK itself won't work with -isolate as we invoke ie GetTargetFrameworks via an msbuild task?

We should account for all of the standard SDK and common.targets referencing "for free". Things should only fail to work for customizations on top of that.

📝 There are a lot of "should"s in that sentence. We have some known problems, for example with NuGet and especially pack operations. That's why we're not evangelizing this hard. Please let us know how it works for you!

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 29, 2020

I just tried a graph build for libraries in dotnet/runtime and I see a few issues:

  1. How do I turn on graph build globally? I had to pass it down when invoking dotnet msbuild which I never use at it doesn't do an incremental restore. Is there a setting to make it global similar to the static graph restore with nuget? @rainersigwald
  2. During a traversal build, errors that are raised during an individual project's build aren't listed at the end of the build. I'm unsure if the full traversal build's ExitCode is negative.
  3. Our TargetFramework.Sdk filtering on the P2P level doesn't work, probably because P2P are determined during evaluation. Ie when I do dotnet msbuild -graphBuild:true Build.proj /p:Subset=libs.ref, all target frameworks are built. @Anipik can you please take a look? Opened TargetFramework.Sdk filtering doesn't work with graphBuild msbuild option enabled arcade#6265 to track this.

Aside from that I'm now a fan of graph build as I don't see any incremental build issues (refpack vs p2ps) with it enabled anymore as all dependencies are determined and built up-front.

EDIT: WOW, incremental builds are now super fast.

@ViktorHofer
Copy link
Member Author

gentle ping @rainersigwald regarding my questions 1 and 2 above.

@rainersigwald
Copy link
Member

  1. How do I turn on graph build globally? I had to pass it down when invoking dotnet msbuild which I never use at it doesn't do an incremental restore. Is there a setting to make it global similar to the static graph restore with nuget?

There's no switch like that at the moment. Can you add a parameter to the scripts you use to invoke build? dotnet/msbuild#5324 may also be related.

  1. During a traversal build, errors that are raised during an individual project's build aren't listed at the end of the build. I'm unsure if the full traversal build's ExitCode is negative.

That's funky. Can you share some repro steps? I don't see this with a small repro attempt.

  1. Our TargetFramework.Sdk filtering on the P2P level doesn't work, probably because P2P are determined during evaluation. Ie when I do dotnet msbuild -graphBuild:true Build.proj /p:Subset=libs.ref, all target frameworks are built.

That's probably the problem. Is the filtering expressible at evaluation time? What specifically is being filtered here--TFs of references, or actual projects?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Nov 16, 2020

There's no switch like that at the moment. Can you add a parameter to the scripts you use to invoke build? dotnet/msbuild#5324 may also be related.

We and our customers invoke individual libraries with dotnet build and it would be great if we could enable static graph in the repository so that it doesn't need to be passed-in every time (otherwise I doubt that people would use the feature at all). I was thinking of a global.json switch that msbuild parses early enough.

That's funky. Can you share some repro steps? I don't see this with a small repro attempt.

Will do.

That's probably the problem. Is the filtering expressible at evaluation time? What specifically is being filtered here--TFs of references, or actual projects?

It's actually filtering projects... How do transitive project references work with static graph? As far as I know, those aren't discovered before the IncludeTransitiveProjectReference target runs? We hook onto the AssignProjectConfigurations target and modify the ProjectReference items then: https://github.com/dotnet/arcade/blob/7a2f81b46ec807fe5df62f6df9c26bd3daee4fcf/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets#L62-L90.

@rainersigwald
Copy link
Member

We and our customers invoke individual libraries with dotnet build and it would be great if we could enable static graph in the repository so that it doesn't need to be passed-in every time (otherwise I doubt that people would use the feature at all). I was thinking of a global.json switch that msbuild parses early enough.

Can you use a Directory.Build.rsp with -graph in it?

@ViktorHofer
Copy link
Member Author

didn't know of that. will give it a try.

@ViktorHofer
Copy link
Member Author

That's probably the problem. Is the filtering expressible at evaluation time? What specifically is being filtered here--TFs of references, or actual projects?

I misspoke, TFs are filtered, not projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

5 participants