Set $(TreatWarningsAsErrors) = true for Release builds.

[jpobst]

Modern best practices for software development recommend treating warnings as errors. Although the majority of warnings are benign, warnings for any potential actual issues will be lost in the sea of benign warnings and will not be caught.

However, having to fix warnings during the development process can be annoying as code is often in a temporary state. As a balance, we only error on warnings for Release builds. This way local development isn't hindered, but CI will protect us from committing new warnings.

There are several projects that have warnings today. For now, we will simply grandfather those projects in, allowing us to move forward with protecting the rest of the projects from new warnings. With time, we should endeavor to fix these projects' warnings and remove them from this list. (Our build currently has ~170 warnings spread across these projects.)

[jpobst]

Tagging the whole team on this one, as this is something we should decide as a team.

[grendello]

I agree that we should have a "zero warnings" policy. Treating warnings as errors for release builds only is a good idea.

[moljac]

I like to listen and understand other side of the argument and usually they state that when warnings are treated as errors, then semantically we don't need warnings. Everything is an error.

From quality standpoint this is excellent, but from feasibility (in some cases) it could cause problems. Not sure if I would be able in my human lifetime to clean up all warnings in Android Libraries.

But agreed it is easier to move abstraction "WarningAsError" to build (Release) than to design diagnostic messages without Warning category/class.

Approved

[pjcollins]

When I build locally, I'm not adding -p:Configuration=Release and I'd want to be able to catch these issues before pushing to CI and waiting for results. I wonder if we should enable this for both Debug and Release? I suppose it may be a bit annoying to work through these early in process, but I think it would be preferable to know if you're adding a breaking change before you draft a commit/PR rather than later.

[grendello]

I often use warnings to "debug" stuff, it would break workloads like that. For Debug, perhaps we should add a private property to enable warnings-as-errors?

[dellis1972]

I use Release builds locally also since allot of our unit tests only work in a Release build.

[jpobst]

The thought was that when you are writing early code you're just first trying to get something working, and having to fix NRT warnings or such takes you out of the flow. So only enforcing warnings on Release was what we went with in JI:

https://github.com/dotnet/java-interop/blob/d5dfa0aac009ac4a8525f35a21dd469b8ccbc155/Directory.Build.props#L6

However, not seeing errors until CI can also be annoying. Especially since android CI takes much longer than JI CI.

There are other options we could consider:

• Keying it off of $(RunningOnCI) instead of Release.
• Additionally adding a local opt-in property that could be enabled. This could be set as an environment variable on ones local machine if someone prefers to always have warnings as errors enabled.

[pjcollins]

Happy to be overruled here, it sounds like keeping this as currently implemented is preferable by most

[jonathanpeppers]

I'm in favor of doing something that would help us slowly solve the warnings over time.

@jpobst maybe you can use $(RunningOnCI)=true for these checks for now?

I often use warnings to "debug" stuff, it would break workloads like that. For Debug, perhaps we should add a private property to enable warnings-as-errors?

@grendello you can set $(TreatWarningsAsErrors) for this. We still have Configuration.Override.props if you want to set it that way, or an env var should also work.

[jpobst]

@grendello you can set $(TreatWarningsAsErrors) for this. We still have Configuration.Override.props if you want to set it that way, or an env var should also work.

I don't think this will work today, as it would also enable it for all the projects with existing warnings. We would need a custom one for now to hit the grandfathering logic in this PR. But ideally we would eventually be able to just use the global one.