Use FeatureSwitchDefinition in a few places by sbomer · Pull Request #106564 · dotnet/runtime

sbomer · 2024-08-16T22:07:51Z

This replaces much of our ILLink.Substitutions.xml with nearly equivalent FeatureSwitchDefinitionAttribute. The minor difference is that both true and false are substituted (when the feature switch is set accordingly), whereas some of our substitution xmls only defined a substitution for the false case.

ghost · 2024-08-16T22:07:57Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2024-08-16T22:07:58Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-policy-service · 2024-08-16T22:08:40Z

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

jkoritzinsky

Interop-related switches LGTM

- IsReflectionExecutionAvaliable shouldn't have used FeatureSwitchDefinitionAttribute because it's not a property - Added missing FeatureSwitchDefinitionAttribute to MetadataUpdater.IsSupported

…butes

jtschuster · 2024-08-23T17:20:34Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/RuntimeType.cs

+        [FeatureSwitchDefinition("System.Reflection")]
        private static bool IsReflectionDisabled => false;


Doesn't this property reflect the negated value of that feature switch? Does this account for that somehow?

<type fullname="System.RuntimeType" feature="System.Reflection" featurevalue="false"> <method signature="System.Boolean get_IsReflectionDisabled()" body="stub" value="true" /> </type>

No, great catch, that one can't be handled by FeatureSwitchDefinition. I'm surprised this didn't cause test failures in ci. @MichalStrehovsky

No, great catch, that one can't be handled by FeatureSwitchDefinition. I'm surprised this didn't cause test failures in ci. @MichalStrehovsky

We don't test things with reflection disabled since that mode is unsupported. So if this enabled reflection unconditionally, I'm not surprised it wasn't caught. I'd be surprised if it was the other way and nothing failed.

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs

src/libraries/System.Text.Json/src/System.Text.Json.csproj

jtschuster

LGTM modulo comments, thank you!

…viceProvider.cs Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>

eerhardt · 2024-08-23T21:47:47Z

src/libraries/System.Text.Json/ref/System.Text.Json.csproj

    <Compile Include="$(CoreLibSharedDir)System\Runtime\Versioning\RequiresPreviewFeaturesAttribute.cs" />
  </ItemGroup>

+  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '9.0'))">


Suggested change

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '9.0'))">

<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0')">

?

I took this condition from the matching section in src/System.Text.Json.csproj - I'd rather keep them the same in this change.

eerhardt

Can we do the System.SR one as well (obviously in a separate PR)?

runtime/src/libraries/Common/src/System/SR.cs

Lines 10 to 13 in 269ea26

    
           private static readonly bool s_usingResourceKeys = GetUsingResourceKeysSwitchValue(); 
        
           // This method is a target of ILLink substitution. 
        
           private static bool GetUsingResourceKeysSwitchValue() => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;

sbomer · 2024-08-23T23:13:18Z

Can we do the System.SR one as well

I'll look into that as a follow-up, thanks!

sbomer · 2024-08-27T16:35:27Z

/ba-g "timeout"

This replaces much of our ILLink.Substitutions.xml with nearly equivalent FeatureSwitchDefinitionAttribute. The minor difference is that both true and false are substituted (when the feature switch is set accordingly), whereas some of our substitution xmls only defined a substitution for the false case. --------- Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>

Use FeatureSwitchDefinition in a few places

09d4846

ghost added the area-System.Runtime.InteropServices label Aug 16, 2024

ghost added the new-api-needs-documentation label Aug 16, 2024

dotnet-policy-service bot assigned sbomer Aug 16, 2024

jkoritzinsky reviewed Aug 16, 2024

View reviewed changes

build-analysis bot mentioned this pull request Aug 16, 2024

windows-arm64 release CrossAOT_Mono crossaot builds failing on Windows headers #106521

Closed

sbomer added 4 commits August 20, 2024 11:02

Fix project files

4e0259a

Missed one

75bf9f4

Fix native AOT feature switches

c00e11b

- IsReflectionExecutionAvaliable shouldn't have used FeatureSwitchDefinitionAttribute because it's not a property - Added missing FeatureSwitchDefinitionAttribute to MetadataUpdater.IsSupported

Cleanup

33695c7

build-analysis bot mentioned this pull request Aug 20, 2024

Nullref in FindImplSlotForCurrentType #105304

Closed

Fix System.Text.Json build

f6d6578

This was referenced Aug 22, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Closed

[browser] test failure: Tests timed out. Killing driver service #103524

Open

sbomer added 2 commits August 22, 2024 18:30

Fix allconfigurations libraries build

3f3609b

Merge remote-tracking branch 'origin/main' into useFeatureSwitchAttri…

d397984

…butes

sbomer marked this pull request as ready for review August 23, 2024 16:33

sbomer requested review from MichalStrehovsky and marek-safar as code owners August 23, 2024 16:33

sbomer requested review from eerhardt, jtschuster and vitek-karas August 23, 2024 16:33

jtschuster reviewed Aug 23, 2024

View reviewed changes

Revert System.Reflection FeatureSwitchDefinition

69c1c74

jtschuster reviewed Aug 23, 2024

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs Show resolved Hide resolved

jtschuster reviewed Aug 23, 2024

View reviewed changes

src/libraries/System.Text.Json/src/System.Text.Json.csproj Show resolved Hide resolved

jtschuster approved these changes Aug 23, 2024

View reviewed changes

Update src/libraries/Microsoft.Extensions.DependencyInjection/src/Ser…

cd6455e

…viceProvider.cs Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>

eerhardt reviewed Aug 23, 2024

View reviewed changes

build-analysis bot mentioned this pull request Aug 27, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

This was referenced Aug 27, 2024

Test failure: XmlSerializerTests.Xml_DerivedIXmlSerializable #106940

Closed

Test failure in XmlSerializerTests.Xml_DerivedIXmlSerializable #107043

Closed

sbomer merged commit 0fcba3f into dotnet:main Aug 27, 2024

github-actions bot locked and limited conversation to collaborators Oct 6, 2024

		[FeatureSwitchDefinition("System.Reflection")]
		private static bool IsReflectionDisabled => false;

	<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan('$(TargetFrameworkVersion)', '9.0'))">
	<ItemGroup Condition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net9.0')">

	private static readonly bool s_usingResourceKeys = GetUsingResourceKeysSwitchValue();

	// This method is a target of ILLink substitution.
	private static bool GetUsingResourceKeysSwitchValue() => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;

Conversation

sbomer commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 16, 2024

Uh oh!

ghost commented Aug 16, 2024

Uh oh!

dotnet-policy-service bot commented Aug 16, 2024

Uh oh!

jkoritzinsky left a comment

Choose a reason for hiding this comment

Uh oh!

jtschuster Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

sbomer Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtschuster left a comment

Choose a reason for hiding this comment

Uh oh!

eerhardt Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

sbomer Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

sbomer commented Aug 23, 2024

Uh oh!

sbomer commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbomer commented Aug 16, 2024 •

edited

Loading