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

Update BuildCheck docs with enablement design #10033

Merged
merged 4 commits into from May 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions documentation/specs/proposed/BuildCheck-Architecture.md
Expand Up @@ -84,6 +84,16 @@ Planned model:

**TBD** - implementation details to be amended by @f-alizada

## High-level logic

MSBuild engine always finds and parses relevant `.editorconfig` files to see which analyzers should be enabled, if any. In typical builds this operation will not be expensive compared to everything else happening as part of building a project. It's a new cost to all builds though, and in the unlikely case that it starts showing as a perf bottleneck, we can cache the data relevant to MSBuild in a separate intermediate file, in a process-wide in-memory cache invalidated by timestamp checks, and so on.
JanKrivanek marked this conversation as resolved.
Show resolved Hide resolved

The rest of the configuration comes directly or indirectly from project files in the form of properties, which creates an interesting ordering issue. For the engine to know the final values of properties such as `TargetFramework`, it needs to evaluate the project. However, if it turns out that an analyzer should be enabled that is interested in _tracing_ the evaluation, it is already too late. It's important to note that this issue exists only for a subset of analyzers. Analyzers interested in the _result_ of evaluation, for example, are fine. The best way of handling this would be to simply evaluate again. Technically, we only need to finish Pass 1 of evaluation to know the value of properties and have the relevant property functions called, so the extra work can be limited to pass 0 and 1. Measurements show that 75% of evaluation is spent in passes 0 and 1. In the very worst case when an extra pass 0/1 runs for each project and lacks any kind of further optimization, single-process incremental build of OrchardCore has been measured to take about 5% longer. There are opportunities for optimizing this, for example by adding a marker to SDK targets files notifying MSBuild of the point after which `TargetFramework` is expected to be fixed so the engine can bail early.
ladipro marked this conversation as resolved.
Show resolved Hide resolved

Once `TargetFramework` is known, we can combine the default analyzer config with what came from `.editorconfig`, restart evaluation if an evaluation-tracing analyzer just got enabled, and continue with the rest of the build. Unlike `TargetFramework` which has an additive effect on the enabled analyzers, a master switch like `RunMSBuildChecks` may instruct the engine to not run anything. It would be unfortunate if the user had to pay any extra cost when `RunMSBuildChecks` evaluates to false. So preferrably we don't do anything perf sensitive until we hit the point in evaluation when `TargetFramework`, `RunMSBuildChecks`, `SdkAnalysisLevel`, ... are all known and the set of analyzers to use is finalized.

Since we are unlikely to enable any analyzers by default in .NET 9, the focus in this release should be on optimizing the `.editorconfig` handling.

# Acquisition

**TBD** - implementation details to be amended by @YuliiaKovalova
Expand Down
21 changes: 17 additions & 4 deletions documentation/specs/proposed/BuildCheck.md
Expand Up @@ -22,9 +22,11 @@ Users are able to tune the behavior of the checks via `.editorconfig` which brin

Powerusers are able to develop, test and publish their custom analyzers easily and contribute them back to community. The local development scenario doesn’t require roundtrip through packaging.

A solid set of in-the-box analyzers is provided by MSBuild and the .NET SDK, extended each release, with high quality reports (pointing exact locations of issue, offering clear and actionable explanations, not repetitive for builds with multi-execution or/and multi-importing of a same script in single build context). The existing in-the-box analyzers are gradually enabled by default and their severity increased - in waves (likely tied to sdk releases) - aiming to constantly increase quality of our customers build scripts. MSBuild.exe (and hence Visual Studio) builds will take more conservative approach with requiring an explicit opt-in into the analyzers - in order to not introduce upgrade blockers.
A solid set of in-the-box analyzers is provided by MSBuild and the .NET SDK, extended each release, with high quality reports (pointing exact locations of issue, offering clear and actionable explanations, not repetitive for builds with multi-execution or/and multi-importing of a same script in single build context). The existing in-the-box analyzers are gradually enabled by default and their severity increased - in waves (likely tied to sdk releases) - aiming to constantly increase the quality of our customers build scripts. To avoid breaking customers builds, there will still be an explicit user gesture required to opt into running the analysis. This will be done either by configuring the analyzers with `.editorconfig` or auto-enabling the analysis based on the TFM of the project. There will be no difference between building with `dotnet build` and with `MSBuild.exe`, they will follow the same enablement rules with the set of enabled built-in analyzers derived from `.editorconfig` and TFM/props. Building in Visual Studio will eventually reach parity with command-line build as well.

The analysis has small impact on build duration with ability to disable analysis altogether which will remove all the performance costs associated with the analysis. The perf impact on representative projects is continuously monitored and documented by the MsBuild team.
Projects that don't use the .NET SDK and those that are not SDK-style at all are TBD. There is a possibility of using a property like `MSBuildAnalysisLevel` to enable some base analyzers we believe will add value everywhere.
ladipro marked this conversation as resolved.
Show resolved Hide resolved

The analysis has small impact on build duration with ability to disable analysis altogether which will remove all the performance costs associated with the analysis. The perf impact on representative projects is continuously monitored and documented by the MSBuild team.


# Scope of initial iteration
Expand Down Expand Up @@ -99,7 +101,7 @@ The proposed initial configuration for those is TBD (as well based on initial te

### Live Build

BuildCheck will run as part of the build and execute [inbox analyzers](#inbox-analyzers) and [custom analyzers](#acquisition-of-custom-analyzers) based on the [configuration](#configuration). Users will have an option to completely opt-out from BuildCheck to run via commandline switch.
BuildCheck will run as part of the build and execute [inbox analyzers](#inbox-analyzers) and [custom analyzers](#acquisition-of-custom-analyzers) based on the [configuration](#configuration). Users will have an option to completely opt-out from BuildCheck to run via an MSBuild property (could be set in a project file or passed on the commandline).
JanKrivanek marked this conversation as resolved.
Show resolved Hide resolved

Findings - reports - of analyzers will be output as build messages/warnings/errors, and the message/warnings/error code should help distinguish BuildCheck produced reports from regular build errors/warnings.

Expand Down Expand Up @@ -128,9 +130,20 @@ We might as well consider specifying custom analyzers on a command line (as a no

There will be 3 mechanisms of configuring the analyzers and rules:
* The default configuration declared by the analyzers themselves ([more details on implementation](#rules-declaration))
* [Sdk Analysis Level property](https://github.com/dotnet/designs/blob/main/proposed/sdk-analysis-level.md) – mostly for the inbox analyzers
* The TFM of the project and the [Sdk Analysis Level property](https://github.com/dotnet/designs/blob/main/proposed/sdk-analysis-level.md) – mostly for the inbox analyzers
* `.editorconfig` file

We will also consider respecting `SdkAnalysisLevel` to override the per-TFM defaults. Additionally, we may introduce a new "master switch" property, tentatively called `RunMSBuildChecks`, to make it possible to disable everything whole-sale. This would be used in scenarios like F5 in VS.
```
Skipping analyzers to speed up the build. You can execute 'Build' or 'Rebuild' command to run analyzers.
```

Here's the proposed release schedule:
- **.NET 9** - the feature is introduced and enabled in `dotnet build` and `MSBuild.exe` command-line builds. It is not enabled in VS just yet. No analyzers are enabled by default. It is not technically required to read the TFM or any other props during evaluation, though it would be nice to respect `RunMSBuildChecks` already in this release. `.editorconfig` can be the sole source of configuration.
- **.NET 10** - based on feedback and testing, we choose a set of analyzers to enable by default for projects targeting `net10.0`, and enable the feature as a whole in Visual Studio. Depending on how we feel about the perf characteristics of evaluation, especially the "double evaluation" mandated by discovering evaluation-tracking analyzers just-in-time, we may want to omit such analyzers from the default set.
- **.NET 11** and beyond - some more analyzers are enabled for projects targeting `net11.0`, the `net10.0` does not change. Everything is mature and performant enough that we are able to auto-enable any analyzer. The gradual tightening of rules enabled by default gets us closer to the long envisioned "strict mode", which will ultimately allow us to evolve MSBuild to be simpler and more performant.


For the `.editorconfig` file configuration, following will apply:
* Only `.editorconfig` files collocated with the project file or up the folder hierarchy will be considered.
* `.editorconfig` files placed along with explicitly or implicitly imported msbuild files won’t be considered.
Expand Down