-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Initial version of Analyzers specs #9853
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
Merged
Merged
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
968c003
Initial draft of specs
JanKrivanek b523fe9
Clarify
JanKrivanek aeb92c6
Clarify configuration section
JanKrivanek b2376cf
Improve wording
JanKrivanek c38e806
Apply suggestions from code review
JanKrivanek fbccee4
Rework based on PR feedback
JanKrivanek ff567bd
PR review meeting feedback
JanKrivanek d041ff8
Apply suggestions from code review
JanKrivanek 08b3680
Update based on PR feedback
JanKrivanek 25ae871
Apply suggestions from code review
JanKrivanek 7465f2d
Reflecting PR comments
JanKrivanek e4154d1
Add planned inbox checks
JanKrivanek d5b6357
Sample code formatting
JanKrivanek f7e0216
Apply suggestions from code review
JanKrivanek af29bda
Reflecting PR comments
JanKrivanek 41922db
Reflecting PR comments
JanKrivanek 815eae8
Fix typos
JanKrivanek 10746e1
Reflect PR comments
JanKrivanek 23bc707
Update BuildCheck-Architecture.md
JanKrivanek 60c5844
Reflect on PR comments
JanKrivanek d4f76ba
Add explicit mention of possibility to fail the build
JanKrivanek cd26a98
Apply suggestions from code review
JanKrivanek 529d3d8
Reflected PR comments
JanKrivanek 90d89aa
Merge branch 'doc/analyzers' of https://github.com/JanKrivanek/msbuil…
JanKrivanek 42e2698
Update documentation/specs/proposed/BuildCheck-Architecture.md
JanKrivanek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
|
|
||
| # BuildCheck - Architecture and Implementation Spec | ||
|
|
||
| This is an internal engineering document. For general overview and user point of view - please check the [BuildCheck - Design Spec](BuildCheck.md). | ||
|
|
||
| # Areas of Ownership | ||
|
|
||
| | Area | Owner | | ||
| |----------|:-------------| | ||
| | PM | @baronfel | | ||
| | Advisory/Leadership | @rainersigwald | | ||
| | Infrastructure | @jankrivanek | | ||
| | Configuration | @f-alizada | | ||
| | Custom Analyzers | @YuliiaKovalova | | ||
| | Inbox Analyzers | @ladipro | | ||
| | Replay Mode | @surayya-MS | | ||
| | Tracing | @maridematte | | ||
| | Perf Advisory | @AR-May | | ||
|
|
||
| # Table of Contents | ||
|
|
||
| - [Infrastructure and Execution](#infrastructure-and-execution) | ||
| * [Data Source](#data-source) | ||
| * [Execution Modes](#execution-modes) | ||
| * [Live Mode Hosting](#live-mode-hosting) | ||
| * [Handling the Distributed Model](#handling-the-distributed-model) | ||
| * [Analyzers Lifecycle](#analyzers-lifecycle) | ||
| - [Configuration](#configuration) | ||
| - [Acquisition](#acquisition) | ||
| - [Build OM for Analyzers Authoring](#build-om-for-analyzers-authoring) | ||
|
|
||
| # Infrastructure and Execution | ||
|
|
||
| ## Data Source | ||
JanKrivanek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| The major source of data for BuildCheck will be the `BuildEventArgs` data - as it is already well established diagnostic source for MSBuild builds. | ||
|
|
||
| BuildCheck can source this data either offline from the binlog, or as a plugged logger during the live build execution. Choice was made to support both modes. | ||
|
|
||
| ## Execution Modes | ||
|
|
||
| **Replay Mode** - so that users can choose to perform analyses post build, without impacting the performance of the build. And so that some level of analysis can be run on artifacts from builds produced by older versions of MSBuild. | ||
|
|
||
| **Live mode** - this is what users are used to from compilation analyses. Integrating into build execution will as well help driving adoption by opting-in users by default to some level of checking and hence exposing them to the feature. | ||
|
|
||
| ## Live Mode Hosting | ||
|
|
||
| Prerequisity: [MSBuild Nodes Orchestration](../../wiki/Nodes-Orchestration.md#orchestration) | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| The BuildCheck infrastructure will be prepared to be available concurrently within the `entrypoint node` as well as in the additional `worker nodes`. There are 2 reasons for this: | ||
| * BuildCheck will need to recognize custom analyzers packages during the evaluation time - so some basic code related to BuildCheck will need to be present in the worker node. | ||
| * Presence in worker node (as part of the `RequestBuilder`), will allow inbox analyzers to agile leverage data not available within `BuildEventArgs` (while data prooved to be useful should over time be exposed to `BuildEventArgs`) | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ## Handling the Distributed Model | ||
|
|
||
| We want to get some bnefits (mostly inbox analyzers agility) from hosting BuildCheck infrastructure in worker nodes, but foremost we should prevent leaking the details of this model into public API and OM, until we are sure we cannot achive all goals from just entrypoint node from `BuildEventArgs` (which will likely never happen - as the build should be fully reconstructable from the `BuildEventArgs`). | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| How we'll internally handle the distributed model: | ||
| * Each node will have just a single instance of infrastructure (`IBuildCheckManager`) available (registered via the MSBuild DI - `IBuildComponentHost`). This applies to a entrypoint node with inproc worker node as well. | ||
| * Entrypoint node will have an MSBuild `ILogger` registered that will enable funneling data from worker nodes BuildChecks to the entrypoint node BuildCheck - namely: | ||
| * Acquisition module will be able to communicated to the entrypoint node that particular analyzer should be loaded and instantiated | ||
| * Tracing module will be able to send partitioned stats and aggregate them together | ||
| * Theoretical execution-data-only sourcing inbox analyzer will be able to aggregate data from the whole build context (again - we should use this only for agility purposes, but shoot for analyzer that needs presence only in entrypoint node). | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * Appart from the scenarios above - the BuildCheck infrastructure modules in individual nodes should be able to function independently (namely - load the inbox analyzers that should live in nodes; send the analyzers reports via logging infrastructure; load user configuration from `.editorconfig` and decide on need to enable/disable/configure particular analyzers). | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * Communication from main to worker node between BuildCheck infra modules is not planned. | ||
|
|
||
| ## Analyzers Lifecycle | ||
|
|
||
| Planned model: | ||
| * Analyzers factories get registered with the BuildCheck infrastructure (`BuildCheckManager`) | ||
JanKrivanek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * For inbox analyzers - this happens on startup. | ||
| * For custom analyzers - this happens on connecting `ILogger` instance in entrypoint node receives acquistion event (`BuildCheckAcquisitionEventArgs`). | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * `BuildCheckManager` receives info about new project starting to be build | ||
JanKrivanek marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * On entrypoint node the information is sourced from `ProjectEvaluationStartedEventArgs` | ||
| * On worker node this is received from `RequestBuilder.BuildProject` | ||
| * `BuildCheckManager` calls Configuration module and gets information for all analyzers in it's registry | ||
| * Analyzers with issues in configuration (communicated via `BuildCheckConfigurationException`) will be deregistered for the rest of the build. | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * Global configuration issue (communicated via `BuildCheckConfigurationException`) will lead to defuncting whole BuildCheck. | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * `BuildCheckManager` instantiates all newly enabled analyzers and updates configuration for all allready instantiated analyzers. | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * At that point of time analyzers are prepared for receiving data and performing their work. MSBuild will start calling `BuildCheckManager` callbacks (mostly pumping `BuildEventArgs`), passed data will be transalted into BuildCheck OM and passed to analyzers. | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * Analyzers may decide to report results of their findings (via `BuildCopDataContext.ReportResult`), the infrastructure will then perform post-processing (filter out reports for `Rule`s that are disabled, set the severity based on configuration) and send the result via the standard MSBuild logging infrastructure. | ||
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
JanKrivanek marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| # Configuration | ||
|
|
||
| **TBD** - implementation details to be amended by @f-alizada | ||
|
|
||
| # Acquisition | ||
|
|
||
| **TBD** - implementation details to be amended by @YuliiaKovalova | ||
|
|
||
|
|
||
| # Build OM for Analyzers Authoring | ||
|
|
||
| **TBD** - details for the initial inbox analyzers set to be amended by @ladipro | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.