Adding System.ComponentModel.Composition #24921
Conversation
maryamariyan
changed the title
Ports MEF1 sources
maryamariyan
force-pushed
the
port.mef1.src-ref
branch
from
44a376c
to
4c5756e
Compare
| @@ -0,0 +1,43 @@ | |||
| // ----------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Please replace all these headers with the usual 3 line license comment in all cs and resx files.
| field.SetValue(instance, value); | ||
| } | ||
|
|
||
| #if FEATURE_CAS_APTCA |
There was a problem hiding this comment.
Delete all blocks with this define. We will never define it.
There was a problem hiding this comment.
and retain code in the #else obviously
There was a problem hiding this comment.
Well, you might be able to delete that too, if it's not publicly exposed
There was a problem hiding this comment.
Confirmed, delete these and calls to them:
public static void DemandMemberAccessIfNeeded(MethodInfo method) { }
+ private static void DemandMemberAccessIfNeeded(ConstructorInfo constructor) { }
+ private static void DemandMemberAccessIfNeeded(FieldInfo field) { }
+ public static void DemandMemberAccessIfNeeded(Type type) { }
|
|
||
| public SerializableCompositionElement(string displayName, ICompositionElement origin) | ||
| { | ||
| #if FEATURE_SERIALIZATION |
There was a problem hiding this comment.
We don't need FEATURE_SERIALIZATION blocks except as needed for serializing exceptions (so, keep any in exceptions, delete the rest, and see if that compiles)
Suggest you do this as a follow up PR as it needs tests also.
| #endif //FEATURE_REFLECTIONCONTEXT && FEATURE_REFLECTIONFILEIO | ||
|
|
||
|
|
||
| //#if FEATURE_REFLECTIONFILEIO |
There was a problem hiding this comment.
All these commented out defines can be removed (retaining the content) ie search for ^//#.*\n
|
Not sure what to do with these ask @weshaggard whether we should keep the code and drop the define, or drop the code and the define. Either way we don't need the define... |
|
|
You can remove all |
|
Ideally cases like this |
|
Also ask Wes about |
| { | ||
| internal sealed class Lock : IDisposable | ||
| { | ||
| #if (FEATURE_SLIMLOCK) |
There was a problem hiding this comment.
delete the #if and retain the code .. we have Slim locks.
| { | ||
| get { return _errors; } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit, use regex to replace double blank lines with single
| { | ||
| internal static class ContractNameServices | ||
| { | ||
| const char NamespaceSeparator = '.'; |
There was a problem hiding this comment.
all fields should have access modifiers, please add private to these
| namespace Microsoft.Internal | ||
| { | ||
| // This class contains utility methods that mimic the mscorlib internal System.Runtime.Versioning.BinaryCompatibility type. | ||
| internal sealed class BinaryCompatibility |
There was a problem hiding this comment.
Nit: source file name has a mispelling.
|
|
||
| public static bool TargetsAtLeast_Desktop_V4_5 { get; private set; } | ||
|
|
||
| [SecuritySafeCritical] |
There was a problem hiding this comment.
Remove all SecuritySafeCritical annotations
| public static bool TargetsAtLeast_Desktop_V4_5 { get; private set; } | ||
|
|
||
| [SecuritySafeCritical] | ||
| static string GetTargetFrameworkNameFromEntryAssembly() |
| internal static class CompositionTraceSource | ||
| { | ||
| #if FEATURE_TRACING | ||
| private static readonly TraceSourceTraceWriter Source = new TraceSourceTraceWriter(); |
There was a problem hiding this comment.
@weshaggard on .NET Core we have TraceSource so I assume we should flip this define so that TraceSourceTraceWriter is the default?
Or just delete DebuggerTraceWriter ?
There was a problem hiding this comment.
Yes we can flip to just TraceSource.
| using System.Diagnostics; | ||
| using System.Diagnostics.Contracts; | ||
| using System.Globalization; | ||
| using System.Reflection; | ||
|
|
||
| namespace Microsoft.Internal | ||
| { | ||
| internal static class Requires | ||
| internal static partial class Requires | ||
| { | ||
| [DebuggerStepThrough] | ||
| [ContractArgumentValidator] |
There was a problem hiding this comment.
You can remove all [ContractArgumentValidator] attributes as we don't use contracts. Keep the code though, unless it's dead.
|
|
||
| namespace System.ComponentModel.Composition.ReflectionModel | ||
| { | ||
| // This |
While I don't think ReflectionOnly or ReflectionContext are super interesting they exist in .NET Standard 2.0 so I would just drop the defines and keep the code. |
| } | ||
|
|
||
| var frameworkName = new FrameworkName(targetFrameworkName); | ||
| TargetsAtLeast_Desktop_V4_5 = (frameworkName.Version >= BinaryCompatibility.Framework45); |
There was a problem hiding this comment.
Is this check even relevant on .NET Core?
There was a problem hiding this comment.
Good catch yes this can surely be hard coded to 'true' and delete this file entirely.
| <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <PropertyGroup> | ||
| <BuildConfigurations> | ||
| netcoreapp2.0; |
There was a problem hiding this comment.
Please move netcoreapp2.0 into a PackageConfiguration and then include that property into the build configurations. Also into BuildConfigurations we need to add netcoreapp for the vertical build from source.
I will look something like:
<PropertyGroup>
<PackageConfigurations>
netcoreapp2.0;
</PackageConfigurations>
<BuildConfigurations>
$(PackageConfigurations);
netcoreapp;
uap;
</BuildConfigurations>
</PropertyGroup>This way its easier to understand why we have netcoreapp2.0 there.
| } | ||
|
|
||
| public int Count | ||
| { |
There was a problem hiding this comment.
Nit => all this can be made one liner methods, since they only have a getter it would look much cleaner. Something like:
public int Count => return Assumes.NotReachable<int>();However I think @danmosemsft mentioned something about "Assumes" so first go through that feedback.
|
|
||
| public static IEnumerable<T> ConcatAllowingNull<T>(this IEnumerable<T> source, IEnumerable<T> second) | ||
| { | ||
| if (second == null || !second.FastAny()) |
There was a problem hiding this comment.
All this ifs can be simplified to !second?.FastAny() maybe in another pass as we want to have the compat package soon.
| { | ||
| case 0: | ||
| return EnumerableCardinality.Zero; | ||
|
|
There was a problem hiding this comment.
nit: extra lines in between cases.
|
Netfx failure is hitting: https://github.com/dotnet/corefx/issues/24903 we should disable those tests with ActiveIssue for netfx. |
maryamariyan
force-pushed
the
port.mef1.src-ref
branch
from
1260f71
to
8b8b268
Compare
|
There are more projects with netfx tests that you need to disable: |
|
@dotnet/dnceng my PR build is succeeding but build is failing afterwards |
|
@donet-bot test this please |
|
|
||
| } | ||
|
|
||
| private static List<T> FastAppendToListAllowNulls<T>( |
| } | ||
| } | ||
|
|
||
| private static void LoadNull(this ILGenerator ilGenerator) |
| private static void LoadInt(this ILGenerator ilGenerator, int value) | ||
| { | ||
| Assumes.NotNull(ilGenerator); | ||
| ilGenerator.Emit(OpCodes.Ldc_I4, value); |
| } | ||
| } | ||
|
|
||
| public static IEnumerable<FieldInfo> GetAllFields(this Type type) |
| <BuildConfigurations> | ||
| netcoreapp; | ||
| uap; | ||
| </BuildConfigurations> |
There was a problem hiding this comment.
I assume we plan to enable netfx testing at some point? Do we have a tracking issue for that?
There was a problem hiding this comment.
Yes, it will be as part of: https://github.com/dotnet/corefx/issues/24903 as that is why we can't add netfx testing because we're testing against the netstandard turd assembly.
There was a problem hiding this comment.
A few minor clean-up comments, but otherwise looks good.
Thanks the work in porting this @maryamariyan
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Debug|AnyCPU'" /> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Release|AnyCPU'" /> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp2.0-Debug|AnyCPU'" /> | ||
| <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp2.0-Release|AnyCPU'" /> |
There was a problem hiding this comment.
Could you add the netstandard configuration in the csproj as well? This for VS to understand the build configurations.
Adding System.ComponentModel.Composition - Ports MEF1 sources - Removes extra lines - Removes all [ContractArgumentValidator] attributes - Replaces old header formats with the usual 3 line license comment in all cs and resx files - Removing code block on true condition of FEATURE_CAS_APTCA - Slight update to sln file - Deleting calls to DemandMemberAccessIfNeeded apis - Simplifies BinaryCompatibility class - Changes Requires.NotNull(element, "element"); to Requires.NotNull(element, typeof(element)); - Removes commented out ifdefs and removing suppressmessage Microsoft.Contracts - Adding private access modifier to unspecified consts and other missing fields - Fixing configuration for netcoreapp - Defines constant FEATURE_TRACING - Removes SecuritySafeCritical annotation - Removes SLIM_LOCK - Removes FEATURE_REFLECTIONONLY, keeping the code it wraps. - Removes FEATURE_REFLECTIONCONTEXT, keeping the code it wraps. - Removes code in the if block of CONTRACTS_FULL - Removes the BinaryCompatibility class and the TargetsAtLeast_Desktop_V4_5 flag, since it is always true. - Removes FEATURE_ADVANCEDREFLECTION, keeping the code it wraps. - Removes MEF_FEATURE_INITIALIZATION, keeping the code it wraps - Removes FEATURE_REFLECTIONFILEIO, keeping the code it wraps. - Change the src project settings to not run apicompat - Adding comment reasoning why we skipped apicompat. also adding netcoreapp config - Updating configuration and removing ApiCompatBaseline file - Update src project - Simplifying Configurations.props files - Removes blank line on csproj and adding Condition to not include compile items when not netstandard - Removes S.CM.C netfx reference from netfxreference.props - Removes index entry in packageIndex - Suppress S.CM.C error - Adds missing ApplicationCatalog. Removing ifdef conditions, Adding APIs to ref - Code formatting for whitespaces on AssemblyCatalog.cs file - Removes uap entry from the inbox list for this S.CM.C in packageindex.json - Disable the tests in netfx which are hitting issue 24903 - Fixing tests in System.ComponentModel.Composition - Formatting whitespace and indentation - Removing this keyword - Removing blank line between switch case lines - Adding the netstandard configuration in the csproj for VS to understand the build configurations Commit migrated from dotnet/corefx@6bac408
Ports MEF1 sources
cc: @danmosemsft @safern