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

Feature switch to turn off COM support in Windows #50662

Merged
merged 6 commits into from
Apr 27, 2021

Conversation

LakshanF
Copy link
Contributor

@LakshanF LakshanF commented Apr 2, 2021

Initial work to have the ability to turn off built-in COM support in windows. This change will allow COM to be turned off but the current behavior will be for built-in COM support to work with no changes for default case.

The initial changes to fix issue #50500. Added an internal property, Marshal.IsComSupported, that reads an AppContext value System.Runtime.InteropServices.Marshal.IsComSupported to set this property which is true by default. Added relevant checks in code for the processor directives, FEATURE_COMINTEROP_UNMANAGED_ACTIVATION and FEATURE_COMINTEROP, and throw NotSupportedException if this is set to false. Validated with some existing basic unit COM tests in NETClient and NativeClient.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Validated with some existing basic unit COM tests in NETClient and NativeClient.

Are you planning on adding tests in this change or a follow-up (or both)? I know you've mentioned dealing with testing, but I don't see anything called out in #50500, so I want to make sure that doesn't get lost.

I could see:

  1. System.Runtime.InteropServices libraries tests validating the behaviour of public APIs with the switch enabled/disabled
  2. ComWrappers (working) running with the switch disabled
  3. COM activation (failure) running with the switch disabled

(2) and (3) would probably require some work in the test infrastructure, since corerun doesn't allow configuring runtime properties right now.

@elinor-fung
Copy link
Member

Just to clarify - I'm not arguing that all the tests and updated test infrastructure absolutely have to be in this PR, just that they need to be part of the built-in COM support feature switch work and handled as part of #50500. I would like to see the libraries tests (which shouldn't need corerun/test infrastucture updates) as part of this PR though.

@jkotas
Copy link
Member

jkotas commented Apr 11, 2021

I would like to see the libraries tests (which shouldn't need corerun/test infrastucture updates) as part of this PR though.

It would be useful to get a run of all libraries tests with this disabled so that we get a good idea what are all dotnet/runtime features that get impacted by this.

@LakshanF
Copy link
Contributor Author

Created issues #51265 and #51266 to track coreclr tests and the library test run with comsupport disabled

@elinor-fung
Copy link
Member

Why move all the existing tests? That seems inconsistent with how other libraries tests are structured for testing config switches that need to be set for the entire project.

I think they keep normal/default cases at the top-level and then have a subfolder for the test project using a non-default config switch. For example, src/libraries/System.Diagnostics.DiagnosticSource/tests has the test project/cases for the default configuration with a subfolder TestWithConfigSwitches for a specific config switch. Similarly, src/libraries/System.Globalization/tests has a top-level test project for the default configuration and then Invariant and NlsTests subfolders for testing with different configuration switches.

@elinor-fung
Copy link
Member

For consistency with other libraries tests, I would expect something more like:

src/libraries/System.Runtime.InteropServices/tests/
    System.Runtime.InteropServices.Tests.csproj
    << existing test sources >>
    ComDisabled/
        System.Runtime.InteropServices.ComDisabled.Tests.csproj
        << test sources specific to IsComSupported=false >>

And System.Runtime.InteropServices.ComDisabled.Tests.csproj would reach back up out of the subfolder for any shared source as necessary.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Product changes look fine to me.

For tests, I think we should have basic coverage for every public API we know this blocks - the current tests only hit some of them. We shouldn't need use all the different kinds of test data that the normal tests do, but I think one case for each API (with input such that it should hit the added checks/throws) would be nice.

I know the normal Marshal tests have a separate file for each API, but I think for the ComDisabled ones, it would be reasonable to group them (instead of basically having a lot of files with one test each).

@LakshanF LakshanF merged commit 1c9e200 into dotnet:main Apr 27, 2021
@LakshanF LakshanF deleted the ComWrn1 branch April 27, 2021 11:03
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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.

8 participants