Migration to netstandard1.3 is *tough bananas*

[ghost]

What I have learned from this:

1. AppDomain.CurrentDomain.GetAssemblies(No AppDomain in general kills) bites hard. We need a clean way of doing this. Any learnings from Castle.Core we can apply? The only way forward I can see right now is to make a leap to netstandard1.6(https://github.com/dotnet/coreclr/issues/919).

2. [ Type.IsGenericType | Type.IsClass | Type.IsAbstract | Type.IsEnum | Type.IsInterface | Type.IsPrimitive | Type.ReflectedType | Type.IsGenericTypeDefinition | ... ad infinitum ... ] also bites pretty hard. Alot of these in netstandard1.3 have been moved out to Type.GetTypeInfo().[Insert Name Here]. Can we abstract this behaviour in the Desktop CLR version, so we can migrate it easier?

3. Attribute.GetCustomAttribute is not a thing. We need a tidy way of dealing with this.

4. Type.IsInstanceOfType has a substitution. System.Reflection.TypeExtensions has a replacement. Not sure if it works.

5. Type.Assembly has a substitution. System.Reflection.TypeExtensions has a replacement. Not sure if it works.

6. Type.GetInterfaces has a substitution(but Type.GetInterface does not). System.Reflection.TypeExtensions has a replacement. Not sure if it works.

7. We need a Array.ForEach extension. This also cause drama but should be easy to do.

8. BindingFlags.IgnoreReturn is not a thing. We need to understand what this means.

There is heaps of stuff here! My conclusion after a couple of goes at this, is that we need a design for dealing with the abstraction of "typing" and "attribute" mechanisms across the entire Desktop CLR version to make the migration easier.

What do you guys think?

[ghost]

Here was my earlier failed attempt at this: https://github.com/fir3pho3nixx/Windsor/commits/netstandard-1-6-port as an "adaptive change".

Quite scary if you think about it.

[ghost]

This was applying 'grepWin' to get rid of SILVERLIGHT in compile directives.

https://github.com/fir3pho3nixx/Windsor/tree/netstandard-1-6-port-brute-force-silverlight-replace

Still scary ...

[ghost]

Any idea's guys?

[jonorossi]

I've tried to provide some pointers for each comment:

1. Don't we already abstract all GetAssemblies calls to ReflectionUtil because Silverlight is also missing this API? However what value would we get having Windsor target .NET Standard 1.3 rather than 1.6, looking at the compat table 1.6 sounds like the ideal option for Windsor: https://github.com/dotnet/standard/blob/master/docs/versions.md. Might be worth considering targeting .NET Standard 2.0 instead since it'll be RTM very soon.
2. All System.Runtime based assemblies have GetTypeInfo, and yes .NET Core drops the older properties. This is what I did in Castle Core to mostly hide TypeInfos: https://github.com/castleproject/Core/blob/master/src/Castle.Core/Compatibility/IntrospectionExtensions.cs. Have a look at usages of FEATURE_LEGACY_REFLECTION_API and the other files in the Compatibility directory.
3. Not sure if we were using Attribute.GetCustomAttribute in Castle Core, however I would have just changed the code to the newer extension methods. I've shimed/polyfilled those extension methods for .NET 3.5/4.0: https://github.com/castleproject/Core/blob/master/src/Castle.Core/Compatibility/CustomAttributeExtensions.cs, however it is probably time to drop support for .NET <4.5.
4. We are using IsInstanceOfType in Castle Core so it must work.
5. Just use GetTypeInfo.
6. Windsor has 2 usages of GetInterface, we might be able to change the usage to GetInterfaces.
7. We should just change to using the foreach keyword.
8. Only affects COM interop, so not a problem with .NET Core.

[ghost]

Ok,

Using your approach I have managed to get the entire code base to compile in dotnetstandard 1.6. Entire branch is here: https://github.com/fir3pho3nixx/Windsor/tree/dotnet-standard-1-6-compatibility

1. You were right it, is mostly inside the reflection util where this was used so it was not too bad. Things that did bleed out breaking changes were things I created wrappers for(AttributeWrapper,AssemblyWrapper, ConstructorWrapper, DelegateWrapper, TypeWrapper, ResourceManagerWrapper) in the shim class (https://github.com/fir3pho3nixx/Windsor/blob/dotnet-standard-1-6-compatibility/src/Castle.Windsor/Compatibility/DotNetStandard.cs). These are all considerations for net standard 1.6.

2. Grabbed your shim for this and made all the GetTypeInfo changes. Have not tried compiling with net40 yet.

3. This also breaks in dotnet standard.

4. OK.

5. That worked quite well.

6. Will look at that.

7. Great, this was tedious but it is most portable.

8. OK

This is just a discovery branch and wont be submitting any PR's from here!

[jonorossi]

I had a quick skim read through DotNetStandard.cs and have got heaps of comments, especially things we also ran into with Castle Core and solved, however as I mentioned in #145 smaller PRs are the only way to get this done, one big PR is too time consuming to work through so it just stalls too easily.

I have no idea the status of #160, seems stalled, however a PR of that size is likely never going to get merged because so much is happening and it all falls on one person.

I'd love for you to get involved in #145, liven up the discussion and get things moving. I'm not wetted to any one specific person's playtime branches, heaps of people have attempted the low hanging fruit, and glossed over the tougher areas, and then you don't hear back from them.

This comment still applies:

It is up to you, but I think a good first step would be to get a build script set up (maybe like our Castle Core one) that we can set up on TeamCity even if the .NET Core build fails compilation, then submit smaller PRs working towards a working build rather than one giant PR which really slowed things down last time and made code review difficult. (#145 (comment))

[ghost]

Hi,

I agree. Had more time to think about this. I was not clear but this was mainly discovery to see what is involved(https://github.com/fir3pho3nixx/Windsor/blob/dotnet-standard-1-6-compatibility/src/Castle.Windsor/Compatibility/DotNetStandard.cs). I think I know enough to start actioning a few more PR's to get this going.

#160 is a monster! I would not do it. Just close the PR explaining why. Doing it in one hit simply too much work in one go. You were clear on this, I think it might have been accidental miscommunication issue.

Before we jump over to #145. Shall we distill some of our thoughts down until we get a tidy list of intended PR's?

Some initial thoughts in no particular order

• The liberal use of build flags everywhere is ugly.

  • Can this be corralled into the Compatibility folder for 'PROD' code?

  For Example instead of:

// Class that lives outside compatibility folder
public class MeDoThing {
    public void ManyThingsOnDifferentPlatformsWithBuildFlags(){
#if PLATFORM_A
        // ... Do This
#elif PLATFORM_B
        // ... Do That
#endif
        }
}

• We try instead for:

namespace Castle.Windsor.Compatibility {
    public interface IThingWhatDoesStuff {
        void ManyThingsOnDifferentPlatformsWithBuildFlags();
    }
}

// Class that lives outside compatibility folder
public partial class MeDoThing : IThingWhatDoesStuff {
    /*public void ManyThingsOnDifferentPlatformsWithBuildFlags(){
        
    }*/
}

// Class that lives inside compatibility folder
namespace Castle.Windsor {
#if PLATFORM_A
    public partial class MeDoThing : IThingWhatDoesStuff {
        public void ManyThingsOnDifferentPlatformsWithBuildFlags(){
             // ... Do This
        }
    }
#elif PLATFORM_B
    public partial class MeDoThing : IThingWhatDoesStuff {
        public void ManyThingsOnDifferentPlatformsWithBuildFlags(){
             // ... Do That
        }
    }
#endif
}

• This will help for static framework references like Assembly.Load.

  • Like the XWrapper's identified in (https://github.com/fir3pho3nixx/Windsor/blob/dotnet-standard-1-6-compatibility/src/Castle.Windsor/Compatibility/DotNetStandard.cs).

  • Tests are a bit more tricky when it comes to ugly ass build flags everywhere. Not sure how we tackle this. Guessing we go with a Polyfill first approach and then create abstractions for where we can't(like static methods)?

  OR

  • We chuck platform based tests into Castle.Windsor.Tests. so that they are in their own assembly? We tie this back to build.cmd, and make build.cmd <platform> <[RunAllTests|RunUnitTests]> honour the platform argument all the way through to the naming of platform based assemblies which get run as extra's based on the platform argument? Also remember I am thinking linux here for the future as well.

• Core polyfills (https://github.com/castleproject/Core/blob/master/src/Castle.Core/Compatibility/IntrospectionExtensions.cs) worked well

  • I tested the GetTypeInfo() polyfill with great success. We could probably do this as a separate PR without even any changes to the build as it stands.

• Get a build going with missing bits

  • Once all the PR's above have been implemented, effectively we should be in a position to automate a build which compiles but has tests failing because of missing bits which then only need to be filled out in the Comaptibility folder. An example of one that I got hit with was how to enumerate all assemblies in the current app domain :)

• Implement compatibility one PR at a time.

  • This is then something we can probably raise issues for independently, mark them with a label 'up for grabs' and the entire community can get involved. We can rigorously monitor PR's for changes outside the Compatibility folder and question those straight away.

• Compatibility is done, time to pack and deploy it!

  • At this point we can think about a release, so some more PR's to get NuGet into shape and presto. We release it.

[jonorossi]

I agree that conditional compilation is ugly, however abstracting heaps of APIs behind more custom APIs also come at a maintenance cost. I'm not yet convinced of the XWrapper classes because I know so many APIs are coming back with .NET Standard 2.0, so I think this sort of thing will really be a case by case thing we'll just need to discuss to pick the less bad approach. For example, some cases it would be nicer to just add an extension method which we use everywhere to add missing functionality.

Just as I did with Castle Core I want to see the reduction in use of platform conditional compilation (e.g. NET40, NET35), and instead based on features available on the platform which will allow us to much more easily remove the conditional compilation as existing .NET Framework features are lit up in .NET Core.

Because the .NET Core surface area is much smaller we'll still have .NET Framework builds and a .NET Core/Standard build. We have supported Mono to some degree in the past, however with the latest work on Castle Core I removed all usage of __MonoCS__ and MONO symbols so the same built binary works on all operating systems, the unit tests still have to be rebuilt but I'd like for that to change too. I don't see any reason we couldn't do the same with Windsor.

Yes please, definitely submit a pull request for GetTypeInfo early. I did two sort of big PRs in the beginning with Castle Core to reduce the amount of churn in PRs for ports, one was reflection and the other remoting/binary serialization.

There are other smallish changes like replacing usages of ExpectedExceptionAttribute and others to get through too.

Looking forward to seeing what you can do with the Compatibility shims/adapters, it'll be much easier to discuss smaller diffs where we are focused on making a decision for one thing.

I'd close this, jump into #145, write down a brief plan, create some issues and begin. We've already got a v4.0 milestone. 🎉

[jonorossi]

Before we jump over to #145. Shall we distill some of our thoughts down until we get a tidy list of intended PR's?

Nah, I've listed a few PRs to start with which are obvious, further from that I'd prefer if you or others just opened an issue describing what you plan to do, or just send a PR and we can discuss it on the code.

[ghost]

Ok! ;)