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

Add granular suppressions for linker warnings #40691

Merged
merged 28 commits into from
Sep 30, 2020
Merged

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Aug 12, 2020

Addresses #38033.

cc @mateoatr

@layomia layomia added this to the 5.0.0 milestone Aug 12, 2020
@layomia layomia self-assigned this Aug 12, 2020
@ghost
Copy link

ghost commented Aug 12, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

eng/illink.targets Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

In the past we put the rd.xml files into the Properties (AppDesigner) folder where other configuration files might live as well. Is there a reason for the extra ILLink folder?

@eerhardt
Copy link
Member

Is there a reason for the extra ILLink folder?

There are a set of ILLink related files that traditionally have been in the root of the project. Mono started grouping them into a folder specifically for this area, and it has worked out well as we've been adding more files in this space. We are already putting ILLink related files in src/ILLink folders. This change is just making all libraries uniform.

In the worst case, we have almost 10 ILLinker files for one library - https://github.com/dotnet/runtime/tree/master/src/libraries/System.Private.CoreLib/src/ILLink

eng/illink.targets Outdated Show resolved Hide resolved
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Assuming the CI is green, this LGTM. Thanks for pulling this off - great work.

@ViktorHofer
Copy link
Member

There are a set of ILLink related files that traditionally have been in the root of the project.

Right I agree that we shouldn't have them in the project root. I was asking if it would make more sense to put them into the Properties folder which is used for other config files as well?

@eerhardt
Copy link
Member

I was asking if it would make more sense to put them into the Properties folder which is used for other config files as well?

The only existing places I see using a Properties folder is for InternalsVisibleTo.cs files.

This PR is just following the established pattern and making the repo consistent. If you feel strongly the existing pattern should be changed, can you log an issue for it?

@eerhardt eerhardt merged commit 994dca6 into dotnet:master Sep 30, 2020
eerhardt added a commit to eerhardt/runtime that referenced this pull request Sep 30, 2020
Changes dotnet#40691 and dotnet#42824 conflicted. One added a new ILLink suppress warnings file, while the other added more warnings. This causes the build to break.

The fix is to regenerate the suppressions file with the latest code.

Fix dotnet#42926
eerhardt added a commit that referenced this pull request Oct 1, 2020
Changes #40691 and #42824 conflicted. One added a new ILLink suppress warnings file, while the other added more warnings. This causes the build to break.

The fix is to regenerate the suppressions file with the latest code.

Fix #42926
@layomia layomia deleted the baselines branch October 1, 2020 02:26
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants