Provide guidance around an idiomatic way to configure "official" / CI builds

[MattKotsenas]

Summary

There doesn't seem to be a standard practice or idiomatic way to specify an "official" build that optional / expensive operations should key off of. Some common types of operations that would benefit from this type of practice:

• Setting version numbers
• Enabling / disabling NuGet lock mode (see https://devblogs.microsoft.com/nuget/enable-repeatable-package-restores-using-a-lock-file/)
• Setting PDB info (e.g. deterministic source paths, embedding, etc.)
• Configuring how pedantic tools should be in regard to warning levels, warnings as errors, etc.

Different codebases create drastically different methods for configuring similar sets of properties, which makes new developer onboarding more difficult. My goal with this issue is to reach consensus and provide guidance for the ecosystem to increase discoverability.

Example scenario

To ground the conversation, I'll use the common example of enabling warnings as errors when doing an official build. I'm picking this example because it is both one that teams often set, and it's a common source of contention for developers. Some developers want it off while hacking around code; some developers want it on to prevent surprises in CI. @jaredpar has a good summary of the tradeoffs here: dotnet/runtime#78414 (comment).

Note

Specifically for code analysis there's https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#optimizeimplicitlytriggeredbuild, which would let Visual Studio handle the most common case here, however:

1. it is VS-centric
2. it doesn't appear to be open to extension
3. It can be confusing for users, as a build may pass and then fail with no user changes

Based on my experience and a quick review of public GitHub code, here's a sampling of the options folks use today.

Configuration Options

Configuration == Release

<PropertyGroup Condition=" '$(Configuration)' == 'Release' ">
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

This one is probably the oldest and most straightforward to understand. However, most devs operate in Debug most of the time, and often CI pipelines run CI tests in Debug in addition to Release.

Pros

• Simple to understand

Cons

• Only applies to release
• Can lead to bit rot in Debug
• Ties together unrelated concerns

BuildingInVisualStudio

<PropertyGroup Condition=" '$(BuildingInVisualStudio)' != 'true' ">
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

Another one I've seen occasionally is using the BuildingInVisualStudio property as a proxy for "is in dev inner loop". However, it only works for developers using Visual Studio. Additionally, setting this property on the command line to reproduce CI-only issues is likely to break other things.

Pros

• Automatic proxy for the dev's inner loop

Cons

• VS-centric
• Unpopular (only 4 hits in GitHub)

ContinuousIntegrationBuild

<PropertyGroup Condition=" '$(ContinuousIntegrationBuild)' == 'true' ">
  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

ContinuousIntegrationBuild is provided by the dotnet/reproducible-builds: Contains the DotNet.ReproducibleBuilds package package, and is consumed by some tools like Source Link, and also mentioned in a few blog posts. The RPB implementation specifically has the issue of being a computed property, requiring consumption in a .targets file, which is confusing for users.

Pros

• Relatively popular - 3k hits -- Code search results
• Used by some dotnet projects like SourceLink and official blog posts

Cons

• Slightly awkward name
• Requires manual plumbing or adopting RPB

Next steps

Where do we go from here? Ideally we could answer these questions:

• Are there other options we should consider?
• Are there any other important pros / cons to call out?
• What guidance do we want to give?

I think I'm leaning towards having ContinuousIntegrationBuild automatically set by the SDK (ideally this part: https://github.com/dotnet/reproducible-builds/blob/7cf65de99f502a5238065a420b772ba737ab96f5/src/DotNet.ReproducibleBuilds/DotNet.ReproducibleBuilds.props#L15-L38) and then publishing guidance that it's the preferred way to do this sort of customization.

As always, I'm sure there's a lot that I'm missing, so thoughts, suggestions, and alternatives all welcome. Tagging @baronfel as someone who may be an owner here? Also tagging @jaredpar who I assume has opinions in this space, and @rainersigwald for FYI as well.

[jaredpar]

To get the best guidance for CI / official builds I think you have to start with outlining the goals of each system.

For CI I believe the goal should be to get the maximum amount of information on the quality of a change in the least amount of time. Given that goal I believe that there should be more than one kind of build in CI. When there is a single build then you have to make a decision about whether to enable code correctness checks like /warnaserror and that creates an information trade off for the CI system:

• /warnaserror+: this gives you more information about code correctness but does so at the expense of tests. A single extra space on a line could cause a style warning that elevates to an error and that effectively ends the CI run. Tests never execute so the developer has gained no information on the functionality of their change, only the style
• /warnaserror-: this helps maximize the chance that the build will succeed and thus tests will execute. This means the developer gets information about the functionality of their code but does not get information about code correctness (given analyzers won't block build).

Neither trade off is inherently right and dev teams can spend a lot of time debating which path to take. Instead of having this debate the ideal setup for CI is to simply do both:

• Build correctness leg: this leg builds with /warnaserror+, all analyzers enabled, code style enforced, etc ... Essentially every requirement that at team places on the code correctness is validated on this leg.
• Test leg: this leg builds with /warnaserror-, disables all analyzers, disables code style checking, etc ... essentially anything that could interfere with a build succeeding. It then runs tests on that change. This maximizes the chances that developers get information about the functionality of their code.

These legs can easily run in parallel and thus don't impact the time goal.

For official builds I believe the goal should be to build the code for production. I think there is little value gained from running anything other than basic smoke tests on official builds. The CI pipeline is where the functionality of the code is asserted, the official build is for producing that validated code for production purposes. I do believe in /warnaserror+ in official builds because, in my experience at least, there are some types of code correctness that can only be validated on a production build machine. For example: tools or analyzers that validate binaries or in a particular signed state, have PDBs set correctly for the production environment, etc ...