Skip to content

Commit c55a1ea

Browse files
baronfelbaronfel
authored
Merge pull request #308 from dotnet/sdkanalysislevel
Add a design for SdkAnalysisLevel
2 parents 62d47b6 + 23111d8 commit c55a1ea

File tree

2 files changed

+190
-0
lines changed

2 files changed

+190
-0
lines changed

‎INDEX.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ Use update-index to regenerate it:
9999
| | [Rate limits](proposed/rate-limit.md) | [John Luo](https://github.com/juntaoluo), [Sourabh Shirhatti](https://github.com/shirhatti) |
100100
| | [Readonly references in C# and IL verification.](proposed/verifiable-ref-readonly.md) | |
101101
| | [Ref returns in C# and IL verification.](proposed/verifiable-ref-returns.md) | |
102+
| | [SDK Analysis Level Property and Usage](proposed/sdk-analysis-level.md) | (PM) [Chet Husk](https://github.com/baronfel), (Engineering) [Daniel Plaisted](https://github.com/dsplaisted) |
102103
| | [Swift Interop](proposed/swift-interop.md) | [Andy Gocke](https://github.com/agocke), [Jeremy Koritzinsky](https://github.com/jkoritzinsky) |
103104
| | [Target AVX2 in R2R images](proposed/vector-instruction-set-default.md) | [Richard Lander](https://github.com/richlander) |
104105

‎proposed/sdk-analysis-level.md

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
# SDK Analysis Level Property and Usage
2+
3+
4+
**Owner** (PM) [Chet Husk](https://github.com/baronfel) | (Engineering) [Daniel Plaisted](https://github.com/dsplaisted)
5+
6+
Today, users of the .NET SDK have a large degree of control over the way that the .NET SDK and the
7+
tools bundled in it emit diagnostics (warnings and errors). This control is provided in part by a
8+
series of MSBuild properties that control the severity levels of specific warnings, if certain messages
9+
should be treated as diagnostics, even a coarse-grained way to set the entire analysis baseline for a project.
10+
11+
The default values for these properties are often driven by the Target Framework(s) chosen for a project, and
12+
as a result users have developed an expectation of behaviors around changing the Target Framework of a project.
13+
It's generally understood that when a TFM is changed, new kinds of analysis may be enabled, and as a result what
14+
was a successful build may now have errors.
15+
16+
This cadence is predictable and repeatable for users - new TFMs are generally only introduced once a year - but
17+
for tooling developers this cadence means that important changes can generally only be tied to a new TFM release.
18+
New diagnostics can be introduced mid cycle, but they can only be enabled by default in a new TFM. Failure to adhere
19+
to this pattern results in pain for our users, as they are often not in control of the versions of the SDK used
20+
to build their code, often because their environment is managed by an external force like an IT department or
21+
a build environment.
22+
23+
Some changes are not able to be logically tied to the TFM, however, and we have no toolset-level parallels
24+
to the existing MSBuild Properties of `AnalysisLevel`, `WarningLevel`, `NoWarn`, etc. This means we have no way to introduce
25+
changes that are activated just by installing a particular version of the SDK, and we have no clear way
26+
to communicate intent to tools that naturally operate outside of the scope of a project - like NuGet at
27+
the repo/solution level.
28+
29+
To fill this gap, and to provide users a way to simply and easily control the behaviors of the SDK and tools,
30+
I propose that we:
31+
32+
* Add a new property called `SdkAnalysisLevel` to the base .NET SDK Targets, right at the beginning of target evaluation
33+
* Set this property default value to be the `MAJOR.Minor.FeatureBand` of the current SDK (e.g. 7.0.100, 7.0.400, 8.0.100)
34+
* Increment this value in line with the SDK’s actual version as it advances across releases
35+
* Use this property to determine the default values of existing properties like `AnalysisLevel` and `WarningLevel` in the absence of any user-provided defaults
36+
* Pass this value wholesale to tools like the compilers – where there is a complicated decision matrix for determining the effective verbosity of any given diagnostic
37+
38+
## Scenarios and User Experience
39+
40+
### Scenario 1: Jolene doesn't control her CI/CD environment
41+
42+
In this scenario Jolene is a developer on a team that uses a CI/CD environment that is
43+
managed by an external team. The infrastructure team has decided that the version of the .NET SDK that
44+
will be preinstalled on the environment will be 8.0.200, but this version introduced a new
45+
warning that is treated as an error by default. At this point, Jolene has a few choices to make about how to resolve this error.
46+
47+
* Resolve the new warning. This may take an indeterminate amount of time, and blocks the teams progress in the meantime.
48+
* Add a `NoWarn` for the new warning(s) and continue working. This unblocks the team but can be a bit of a whack-a-mole situation, as new warnings will require new `NoWarn`s.
49+
* Add a `SdkAnalysisLevel` with a value of the SDK she was previously using. This unblocks the team and informs tooling of the desired unified warnings experience that Jolene's team is coming from.
50+
51+
```mermaid
52+
graph TD
53+
classDef good fill:green
54+
classDef bad fill:red
55+
start([New SDK Update])
56+
opt1{Resolve the warning}
57+
opt2{Add NoWarn}
58+
opt3{Add `SdkAnalysisLevel`}
59+
result1[Unknown time, blocks team]
60+
class result1 bad
61+
result2[Unblocks team, not future-proof]
62+
class result2 bad
63+
result3[Unblocks team, future-proof]
64+
class result3 good
65+
start-->opt1
66+
start-->opt2
67+
start-->opt3
68+
opt1-->result1
69+
opt2-->result2
70+
opt3-->result3
71+
```
72+
73+
After assessing these choices Jolene's team doesn't have time to fix the
74+
diagnostic until next month, so for now she instructs the build to behave as if it were
75+
using the 8.0.100 SDK by setting the `SdkAnalysisLevel` property to `8.0.100` in a
76+
`Directory.Build.props` file in her repo root:
77+
78+
```xml
79+
<Project>
80+
<PropertyGroup>
81+
<SdkAnalysisLevel>8.0.100</SdkAnalysisLevel>
82+
</PropertyGroup>
83+
</Project>
84+
```
85+
86+
### Scenario 2: Marcus is on a GPO-managed device
87+
88+
Marcus is working on a small single-project .NET MAUI app, and his company manages his hardware via GPO.
89+
On release day, the company pushed out Visual Studio updates to his team, and as a result his prior
90+
feature band of `8.0.200` is no longer available - it's been replaced by `8.0.300`. He's not sure
91+
about these warnings new warnings so he unblocks himself by setting `<SdkAnalysisLevel>8.0.200</SdkAnalysisLevel>`
92+
in his project file to unblock builds until he has time to investigate.
93+
94+
---
95+
96+
In both of these scenarios, the user is *not in control* of their environment. They can control their code,
97+
but they do not have the time to address the full set of problems introduced by an update. They use the new
98+
property to request older behavior, for a _limited time_, until they can address the issues. In addition,
99+
this single property was able to control the behavior of the SDK, the compilers, and any other tools that
100+
have been onboarded to the new scheme, without having to look up, comment out, or add many `NoWarn` properties
101+
to their project files.
102+
103+
104+
## Requirements
105+
106+
### Goals
107+
108+
* Users have a unified way to manage diagnostics for all the tools in the SDK
109+
* Tooling has a way to understand what compatibility level a user explicitly wants
110+
* Users are able to upgrade across SDKs without being forced to immediately resolve warnings
111+
* Tools bundled in the SDK adopt `SdkAnalysisLevel` as a guideline
112+
113+
<!--
114+
Provide a bullet point list of aspects that your feature has to satisfy. This
115+
includes functional and non-functional requirements. The goal is to define what
116+
your feature has to deliver to be considered correct.
117+
118+
You should avoid splitting this into various product stages (like MVP, crawl,
119+
walk, run) because that usually indicates that your proposal tries to cover too
120+
much detail. Keep it high-level, but try to paint a picture of what done looks
121+
like. The design section can establish an execution order.
122+
-->
123+
124+
### Non-Goals
125+
126+
* Users keep using `SdkAnalysisLevel` indefinitely
127+
* Microsoft-authored tools delivered outside the SDK adhere to `SdkAnalysisLevel`
128+
* Non-Microsoft authored tools adhere to `SdkAnalysisLevel`
129+
130+
<!--
131+
Provide a bullet point list of aspects that your feature does not need to do.
132+
The goal of this section is to cover problems that people might think you're
133+
trying to solve but deliberately would like to scope out. You'll likely add
134+
bullets to this section based on early feedback and reviews where requirements
135+
are brought that you need to scope out.
136+
-->
137+
138+
## Stakeholders and Reviewers
139+
140+
| Team | Representatives |
141+
| ---- | --------------- |
142+
| SDK | @marcpopmsft @dsplaisted |
143+
| MSBuild | @rainersigwald @ladipro |
144+
| NuGet | @aortiz-msft @nkolev92 |
145+
| Roslyn | @jaredpar @CyrusNajmabadi |
146+
| F# | @vzarytovskii @KevinRansom |
147+
| PM | @KathleenDollard @MadsTorgersen |
148+
149+
## Design
150+
151+
### Valid values of `SdkAnalysisLevel`
152+
153+
The implementation of `SdkAnalysisLevel` is itself quite straightforward - the default value of `SdkAnalysisLevel` is always
154+
the stable 'SDK Feature Band' of the SDK that is currently being run, and the only valid values for the property are other SDK Feature Bands.
155+
An SDK Feature Band is a Semantic Version of the form `<Major>.<Minor>.<Patch>`, where the `<Patch>` version is a multiple of 100. Some
156+
valid feature bands for this discussion might be 6.0.100, 7.0.300, or 8.0.200.
157+
158+
### Where to define `SdkAnalysisLevel`
159+
160+
The more interesting question is where such a value might be set. Users typically set values for properties in one of three ways:
161+
162+
* as a MSBuild global property via an environment variable or `-p` option of MSBuild/.NET CLI
163+
* as a MSBuild local property in a Project file (or MSBuild logic Imported by a project file)
164+
* as a MSBuild local property in a Directory.Build.props file (this is broadly the same as option 2 but happens implicitly)
165+
166+
If we would like users to be able to set this new property in any of these, the SDK cannot determine the 'final' value of `SdkAnalysisLevel` until after the Project file has been evaluated.
167+
If we want all of the build logic in `.targets` files to be able to consume or derive from the value then we must set the value as early as possible.
168+
These two constraints point to setting the property as early as possible during the `.targets` evaluation of the SDK - for this reason
169+
we should calculate the new property [at the beginning of the base SDK targets](https://github.com/dotnet/sdk/blob/558ea28cd054702d01aac87e547d51be4656d3e5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L11).
170+
171+
While an earlier location could be chosen (e.g. Microsoft.NET.Sdk.props) this would open up a layering/timing issue where the value of `SdkAnalysisLevel` could be set to a default, then computed by some other MSBuild props before the users' input had been evaluated. This would be confusing and error-prone for users, and would require a lot of extra work to ensure that the value was always correct. For this reason we choose to set the value in the base SDK targets as described above.
172+
173+
### Relation to existing WarningLevel and AnalysisLevel properties
174+
175+
These properties cover a lot of the same ground, but are defined/imported [too late in project evalation](https://github.com/dotnet/sdk/blob/558ea28cd054702d01aac87e547d51be4656d3e5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L1315) to be usable by the rest of the SDK. In addition,
176+
we cannot safely move their evaluation earlier in the overall process due to compatibility concerns. Since `SdkAnalysisLevel` will be defined earlier,
177+
we should define how it impacts these two. Curently WarningLevel and AnalysisLevel are [hardcoded](https://github.com/dotnet/sdk/blob/558ea28cd054702d01aac87e547d51be4656d3e5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.Analyzers.targets#L25C6-L25C26) and must be updated with each new SDK. We will change this to infer the AnalysisLevel based on the value of `SdkAnalysisLevel` - the `<Major>` portion of the `SdkAnalysisLevel` will become the value of `AnalysisLevel`, which itself influences the default for `WarningLevel`.
178+
179+
## Q & A
180+
181+
<!--
182+
Features evolve and decisions are being made along the road. Add the question
183+
as a subheading and provide the explanation for the decision below. This way,
184+
you can easily link to specific questions.
185+
186+
When you find yourself having to explain something in a GitHub discussion or in
187+
email, consider to update your proposal and link to your answer instead. This
188+
way, you avoid having to explain the same thing over and over again.
189+
-->

0 commit comments

Comments
 (0)