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

Enable warnings as errors in CI pipeline #968

Closed
MattKotsenas opened this issue Sep 4, 2024 · 10 comments · Fixed by #976
Closed

Enable warnings as errors in CI pipeline #968

MattKotsenas opened this issue Sep 4, 2024 · 10 comments · Fixed by #976

Comments

@MattKotsenas
Copy link
Contributor

Would you be interested in turning on warnings as errors in the CI pipeline? When I build locally I see ~200 warnings, which makes ensuring I don't introduce new warnings difficult.

Some maintainers dislike the churn, so I wanted to ask before starting. If you agree, I'd follow an approach like this:

  1. Enable warnings as errors in the AppVeyor pipeline, but not locally to avoid slowing down local dev when you're just trying to experiment
  2. Fix all the warnings that can be easily / trivially fixed. I can break the fixes up by ID to make review easier
  3. For issues that are difficult to fix, use inline suppressions to "baseline" and prevent future issues, and follow tracking bug(s) to address
@EdwardCooke
Copy link
Collaborator

That's interesting. When I build locally there's no errors or warnings. Same when the CI server builds, there's no warnings.

@MattKotsenas
Copy link
Contributor Author

Interesting... I'll see if I can figure out what's causing the difference in behavior on my machine.

One thing that I notice is there's no global.json, so we're likely using different SDK versions. What SDK version do you / CI use?

@EdwardCooke
Copy link
Collaborator

I would assume the CI is using the latest. On mine I have 8.0.304. I ran the docker build, which is currently broken, just about to push up a PR for fixing that, and it shows a few.

@EdwardCooke
Copy link
Collaborator

K, the only warnings I could get to appear, which were some formatting and a file header, should be solved now. They also only showed up in Linux which I was able to get to them through the Docker image build.

@MattKotsenas
Copy link
Contributor Author

OK, I'm running a 9.0-preview build. So I bet that coupled with opt-ing into "recommended" here:

<AnalysisLevel>latest-Recommended</AnalysisLevel>

means that the recommendations get stricter with the .NET 9 SDK.

I can just close this since this is just my problem. I also can make a quick PR with a global.json like this:

{
  "sdk": {
    "version": "8.0.300",
    "rollForward": "patch"
  }
}

(or similar). That would then signal to users what the acceptable SDK range is, pick the correct one if it's available, and give a (mostly) helpful error message if one isn't available.

Let me know what you prefer. Thanks!

@MattKotsenas
Copy link
Contributor Author

OK, yeah verified with SquiggleCop that it's due to new warnings being recommended as part of .NET 9.

My personal preference is to use the global.json to set a version to help prevent people from getting into unsupported configurations, but I'm happy to do whatever you prefer :)

@EdwardCooke
Copy link
Collaborator

I think it would be beneficial to fix any warnings you’re getting with .net 9. We’ll eventually want to support it natively like we do with net6/net8. I think that would be my personal preference.

@lahma
Copy link
Contributor

lahma commented Sep 5, 2024

Created #971 to remove warnings on NET 9 SDK.

@EdwardCooke
Copy link
Collaborator

Thanks. I’ll hopefully get to the pr today or tomorrow.

@EdwardCooke
Copy link
Collaborator

Done. And errors are fixed. I enforced the code style checking on release builds (it added ~40 seconds) to the build and that's annoying so it'll do it during CI. Had to ignore a couple of IDE0055 Linux bugs and CRLF line endings but it's out now.

dotnet/roslyn#55526
dotnet/roslyn#73122

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 a pull request may close this issue.

3 participants