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.

Sign up for GitHub

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

Jump to bottom

Apply BuiltInComInterop feature switch to managed code #54056

Merged
merged 3 commits into from
Nov 18, 2021
Merged

Apply BuiltInComInterop feature switch to managed code #54056

merged 3 commits into from
Nov 18, 2021

Conversation

marek-safar
Copy link
Contributor

@marek-safar marek-safar commented Jun 11, 2021 •
edited
Loading

Follow up on #43501

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@marek-safar marek-safar marked this pull request as ready for review June 11, 2021 11:44
@ghost
Copy link

ghost commented Jun 11, 2021

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

Issue Details

Follow up on #43501

Author: marek-safar
Assignees: -
Labels:

area-System.Runtime
Milestone: -

@AaronRobinsonMSFT
Copy link
Member

/cc @jkoritzinsky @elinor-fung

@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Jun 29, 2021
Merged
AaronRobinsonMSFT
AaronRobinsonMSFT approved these changes Jul 1, 2021
View reviewed changes
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

I'd like to see the suggestion from @LakshanF in #54056 (comment) done prior to check-in. After that looks good.

@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@eerhardt
Copy link
Member

eerhardt commented Nov 4, 2021

I see there are conflicts to resolve. @marek-safar - do you want me to take over this PR and push it forward?

@marek-safar
Copy link
Contributor Author

@eerhardt please go ahead and finish this.

@eerhardt
Copy link
Member

@LakshanF - do those BuiltInComInterop.IsSupported"), false tests get run in CI now? Or is there a way for me to do that run on this PR?

@eerhardt
Copy link
Member

do those BuiltInComInterop.IsSupported"), false tests get run in CI now? Or is there a way for me to do that run on this PR?

I chatted offline to @LakshanF about running the above tests. We decided that running the tests with BuiltInComInterop=false set inside coreclr wouldn't be beneficial with this change. The reason being that the changes here only affected trimmed/linked managed code. Since those tests wouldn't actually trim the managed code, these changes won't be affected.

With that, I believe this change is ready to merge. @AaronRobinsonMSFT @MichalStrehovsky - Can you give this one more quick check before I merge it?

@eerhardt eerhardt merged commit 8ae135a into dotnet:main Nov 18, 2021
@MichalStrehovsky MichalStrehovsky mentioned this pull request Nov 20, 2021
Merged
@eerhardt eerhardt mentioned this pull request Nov 23, 2021
Merged
vitek-karas added a commit to vitek-karas/runtime that referenced this pull request Nov 24, 2021
@vitek-karas
Suppress trim warning caused by recent attribute removal changes
fbadd4f 
This fixes a new warning generated by trimming some apps which was introduced in dotnet#54056.
The `ComVisibleAttribute` in this case is referenced, but if it's removed it doesn't change functionality in any way.
@vitek-karas vitek-karas mentioned this pull request Nov 24, 2021
Merged
vitek-karas added a commit that referenced this pull request Nov 26, 2021
@vitek-karas
Suppress trim warning caused by recent attribute removal changes (#62023
90b8c70 
)

This fixes a new warning generated by trimming some apps which was introduced in #54056.
The `ComVisibleAttribute` in this case is referenced, but if it's removed it doesn't change functionality in any way.
This was referenced Nov 29, 2021
Closed
Closed
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@LakshanF LakshanF LakshanF left review comments

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT approved these changes

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marek-safar @AaronRobinsonMSFT @eerhardt @LakshanF @MichalStrehovsky