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

Add a design for SdkAnalysisLevel #308

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Add a design for SdkAnalysisLevel #308

merged 4 commits into from
Mar 7, 2024

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Dec 7, 2023

This is the first pass at SdkAnalysisLevel, a unified flag to help users manage their SDK warning levels in situations where they may not be able to pin SDKs via global.json or other means. The property is intended to communicate to tooling like the compilers an explicit intent - "treat me as if I were SDK X.Y.Z" without requiring the user to install or manage other SDKs. The first well-defined use of this is the Roslyn field member breaking change, where members of the Roslyn team hope to use this flag in conjunction with other user signals to determine if a user should get warnings about the field breaking change.

I've spoken with most of the folks mentioned in the stakeholders/reviewers section about this before - now it's time to get broader input and push the design to done.

@baronfel
Copy link
Member Author

baronfel commented Dec 7, 2023

CC the stakeholders/reviewers from the doc:

Team Representatives
SDK @marcpopMSFT @dsplaisted
MSBuild @rainersigwald @ladipro
NuGet @aortiz-msft @nkolev92
Roslyn @jaredpar @CyrusNajmabadi
F# @vzarytovskii @KevinRansom
PM @KathleenDollard @MadsTorgersen

proposed/sdk-analysis-level.md Outdated Show resolved Hide resolved
Copy link

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

I'm guessing we're not including the warning strengthening policy here?

@baronfel
Copy link
Member Author

Note from @timheuer - cover why use this vs global.json? Changing the SDK chosen vs Changing the behavior of the chosen SDK.

@baronfel
Copy link
Member Author

Looks great.

I'm guessing we're not including the warning strengthening policy here?

Yes exactly - we can define the policy/support matrix for teams after this is implemented. IIRC the main question was "how long is long enough to establish a lower bound for tools to support older SDKs", and some answers were "previous 2 LTSs", "previous LTS", and "forever".

@Evangelink
Copy link
Member

@baronfel Feel free to ping us (test team) on such changes :) This is interesting as we are also feeling this "limitation".

@baronfel
Copy link
Member Author

baronfel commented Jan 4, 2024

I've created dotnet/sdk#37778 with a potential implementation of the new field to help illustrate this doc.

@tboby
Copy link

tboby commented Feb 24, 2024

Both the scenarios described in this design are ones I have experienced in the past 6 months, and the changes proposed seem like the right way to start to reduce their impact.

I note this design doesn't actually detail what should be done with this new level, so it's hard to say if it would solve the issue, but I hope giving a real world example might be useful.

I think another important scenario is that of mixed target frameworks within a single build environment.

My context is a highly regulated industry where changes cannot be made lightly. For example, a single repository may have:

  • a maintenance branch targeting 6 LTS
  • main containing an rc targeting 7
  • a feature branch targeting 8

We might simultaneously need pipelines to support:

  • safely building the main branch if we need to release, which must be stable
  • building and running new integration tests or security analysis tools on the maintenance branch - without changing the product
  • upgrading the feature branch to target the latest runtime/SDK to evaluate whether we can upgrade and get the longest support for the next release

In order to simplify managing builds and enforce consistency our pipeline definitions are shared across repositories.

We could theoretically migrate repositories to the next major SDK versions individually but:

  • that involves making many small changes to the pipelines where errors are hard to detect
  • branch specific pipelines are fragile at best or not supported at all

In addition windows cicd runners are complicated, and it's not practical to have multiple runners with different sdks. It may not be acceptable nor practical to add global.json files to many frozen projects.

This version introduced a new warning which is treated as an error

This doesn't have to be a new warning; a bug fix to an existing warning is sufficient. This happened to us with the release of 8. A bug fix to CA2017 suddenly started failing builds due to a relatively minor logging issue.

This time round the number of repos, and the timing in relation to our releases meant we could just fix code. However, in two years I imagine we'll be too big for that to be practical, and a similar change in a minor SDK release would be equally disruptive.

How this would help:
I would set sdk analysis level in every solution alongside the target framework so that builds do not break on SDK upgrade. I would potentially set up a parallel build pipeline which unset the level but did not fail builds to help assess impact.

Questions:

  1. Would any changes to analysers be considered a change in level?

If yes, how practical is it for dotnet to manage backwards compatibility?

If no, how is this explained, and does it it undermine the value if setting the analysis level only sometimes works.

  1. What previous levels would be supported? Presumably all in support SDK versions at the time of release?

  2. When would support be dropped? This might not be as complicated as I'm thinking, but would a patch SDK release of net 6 today maintain the analyser level for the previous LTS (which would be years out of support)

  3. Re the non-goal "Users keep using SdkAnalysisLevel indefinitely": I can't see a practical way of using the property without making it a permanent feature in my solution build props alongside tfm. If it isn't already set then the SDK upgrade will break previously stable builds. It may be appropriate to keep using it until that runtime is out of support if the product lifetime is similar.

@nkolev92
Copy link

nkolev92 commented Mar 7, 2024

@baronfel

As far as I can tell, this design is implemented in https://github.com/dotnet/installer/pull/18687/files, should this PR be merged?

@baronfel
Copy link
Member Author

baronfel commented Mar 7, 2024

It definitely could - was mainly leaving it open as a way to collect more feedback, but we can 'snap' at this point-in-time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants