-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Refactor/Unify Build and Graph OM and API #10172
Refactor/Unify Build and Graph OM and API #10172
Conversation
Co-authored-by: Mariana Dematte <magarces@microsoft.com>
3d41e7d
to
e6771c4
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
rekicking CLA bot |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
rekicking cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me, but because there are so many changes to just nullable variables it gets harder to review this. Maybe all the nullable changes could have been in another PR just so we can focus on the functionality of the refactor.
Co-authored-by: Mariana Dematte <magarces@microsoft.com>
Experimental VS insertion: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/562572 (build goes fine now!) |
Prerequisity for indicating Build Submissions in Build Events (which will be needed by BuildCheck to detect Restore)
Context
Build and Graph Build OM and API is largely duplicated. Building on top of this (e.g. to add 'BuildSubmissionStartedEventArgs') would require even further duplication of code.
So let's deduplicate the OM and API.
Changes Made
BuildManager
BuildManager
Important notes:
I sealed the existing OM - letting it open was a design mistake that doesn't have sane usage. Hopefully there is none - we can easily revert if there proves to be some usages.
For some reasons the Results Cache is applicable only for 'standard' build requests - not for graph build requests. I do not fully understand why. I have left that aspect untouched
msbuild/src/Build/BackEnd/Components/Caching/IResultsCache.cs
Line 33 in 9bea802
ExecuteSubmission
differs very significantly for BuildRequest and GraphBuildRequest - those are left in their separate functions (but at least there is a single entrypoint function to them)The entrypoint location of where we should introduce emiting of
BuildRequestStarted
is indicated in code by TODO with some hints (https://github.com/dotnet/msbuild/pull/10172/files#diff-2b0716a511d8f4ee690ebd5c3a59dec1e3f9a5eab4ab2a80a1018820a658accbR1370-R1372) - so once this PR is merged, there hopefully should be a no blocker for Analyzers prototyping - Detect and skip restore (evaluation and execution) #9747 (FYI @maridematte)FYI @dfederm - this is heavily touching graph requesting - so your PoV would be very helpful