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

Remove unused suppressions for System.Linq.Expressions #89524

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jul 26, 2023

fixes #89206

@ericstj ericstj added the source-build Issues relating to dotnet/source-build label Jul 26, 2023
@ericstj ericstj self-assigned this Jul 26, 2023
@ghost
Copy link

ghost commented Jul 26, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

fixes #89206

Author: ericstj
Assignees: ericstj
Labels:

area-Infrastructure-libraries, source-build

Milestone: -

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Approving to unblock.

Two questions for my own education:

@mthalman
Copy link
Member

Approving to unblock.

Two questions for my own education:

The error only shows up when using the 8.0 Preview 7 SDK to build. This repo isn't using that.

@ericstj
Copy link
Member Author

ericstj commented Jul 26, 2023

Correct, we cannot use the Preview7 SDK yet because it is not done. We cannot use the nightly build of APICompat because it depends on preview7 API that was added to runtime. So we're in a situation for a few weeks where we'll not have protection for these.

@mthalman another option here would be to just disable the consistency check until we can turn it on in dotnet/runtime. To do that you can set ApiCompatPermitUnnecessarySuppressions to true.

@marek-safar marek-safar merged commit e4cca32 into dotnet:main Jul 27, 2023
@ivanpovazan
Copy link
Member

ivanpovazan commented Jul 27, 2023

Regarding:

Before the change, we were building/shipping System.Linq.Expressions.dll differently for iOS-like platforms (ios/tvos/maccatalyst) so the suppressions were put into place to accomodate this (note the extra entries which had platform name in the paths in Right element)

I had the same question, so thanks @mthalman @ericstj for the clarification in your comments.

If I understand correctly, I should have had built the runtime with /p:ApiCompatGenerateSuppressionFile=true to update the suppressions and made it part of my PR.

@ericstj
Copy link
Member Author

ericstj commented Jul 27, 2023

I should have had built the runtime with /p:ApiCompatGenerateSuppressionFile=true

You could have, but I honestly wouldn't expect you to realize that. Once we can update to the Preview7 SDK it will be obvious, and hopefully the steps you need to take will be apparent in the error message.

Also - you don't need to do it for the whole repo - you could simply do this when building a single src csproj.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries source-build Issues relating to dotnet/source-build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary suppression errors showing up in build using .NET 8 Preview 7 SDK
5 participants