Skip to content

Conversation

@mthalman
Copy link
Member

Recently, Microsoft.CodeAnalysis.AnalyzerUtilities.dll has showed up as a SB poison leak in the VMR build. For now, we're going to need to allow this for .NET 8 Preview 7 release. Since poison leaks currently break the build as error, we need to disable that and instead use poison tests with a baseline that includes this file. This will allow for passing builds. These changes revert most of the changes that were made in #16631 which got rid of the poison tests.

@mthalman mthalman requested a review from a team as a code owner July 18, 2023 15:33
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

I am fine with these changes for Preview 7 but I think we need to discuss what long term solution we want to utilize if we have a poison leak that we don't want to block on for whatever reason, we should have a mechanism in place that allows it. Either we keep the PoisonTests in place even when we get poison clean again or we add a mechanism to the CheckForPoison msbuild task to support allowed leaks when FailOnPoisonFound

@mthalman
Copy link
Member Author

I'm in favor of keeping the poison tests in place. I don't want to have to reinvent the wheel just to get rid of the tests.

@mthalman mthalman merged commit 62e6796 into dotnet:main Jul 18, 2023
@mthalman mthalman deleted the poison branch July 18, 2023 18:32
@MichaelSimons
Copy link
Member

I'm in favor of keeping the poison tests in place. I don't want to have to reinvent the wheel just to get rid of the tests.

I am fine with that. One advantage that gives is that if there is a poison leak, we can get a more complete assessment of the quality because all of the tests will run rather than the build failing because of a leak.

FYI @NikolaMilosavljevic

@NikolaMilosavljevic
Copy link
Member

I'm in favor of keeping the poison tests in place. I don't want to have to reinvent the wheel just to get rid of the tests.

I am fine with that. One advantage that gives is that if there is a poison leak, we can get a more complete assessment of the quality because all of the tests will run rather than the build failing because of a leak.

FYI @NikolaMilosavljevic

I think that is better approach - a solution that works. It is proving too costly to allow new poisons to break the build and force the team to work on immediate resolution.

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.

3 participants