castleproject / Core

Castle Core, including Castle DynamicProxy, Logging Services and DictionaryAdapter
http://www.castleproject.org/
Other
2.2k stars 468 forks source link

Add `net6.0` as a target framework #616

Closed Jevonius closed 2 years ago

Jevonius commented 2 years ago

Adds net6.0 as a target framework, including handling cases where functions are Windows-only for net6.0.

Jevonius commented 2 years ago

Having tweaked how MONO_EVENTLOG_TYPE is being set in DiagnosticsLoggerTestCase, I briefly considered adding logic in a static constructor for DiagnosticsLogger to automatically set MONO_EVENTLOG_TYPE when using Mono. Realised that probably warrants some discussion though, given it would be a change to default behaviour and could be undesirable. Code would have been along the lines of:

static DiagnosticsLogger()
{
    if (RunningOnMono() && string.IsNullOrEmpty(Environment.GetEnvironmentVariable("MONO_EVENTLOG_TYPE")))
    {
        Environment.SetEnvironmentVariable("MONO_EVENTLOG_TYPE", "local:" + Environment.CurrentDirectory);
    }
}

private static bool RunningOnMono()
{
    // taken from http://www.mono-project.com/FAQ:_Technical
    return Type.GetType("Mono.Runtime") != null;
}
Jevonius commented 2 years ago

Not entirely sure why for commit e078044 the GitHub Ubuntu build failed to respect SupportedOSPlatform, whereas the AppVeyor Ubuntu build did, but have switched to the NUnit Platform attribute which is more consistent with elsewhere in the codebase anyway.

There are a few places I've had to make code "Windows only" which are probably worthy of review - I've generally limited this to when on net6.0 but there is a change in DynamicProxy around COM interop that it would be good to get a second opinion on. I tried checking the Mono source and limiting to Windows here is probably correct, but Marshal.GetIUnknownForObject's behaviour on Mono is unclear to me (see IsComObject, and GetIUnknownForObject).

Jevonius commented 2 years ago

~Need to update README and CHANGELOG.~ Done.

stakx commented 2 years ago

I briefly considered adding logic in a static constructor for DiagnosticsLogger to automatically set MONO_EVENTLOG_TYPE when using Mono. Realised that probably warrants some discussion though, given it would be a change to default behaviour and could be undesirable.

I'd keep such a change out of this PR, since it isn't required for adding the net6.0 target.

Jevonius commented 2 years ago

Apologies for the delay getting back to this - have been swamped. Hoping to get on it again later this week.

Jevonius commented 2 years ago

Please check whether the Mono runtime guard for Marshal.IsComObject, and the related additional NuGet dependency on net462 is truly necessary. I would personally have gone with simply suppressing the compiler warning instead.

I've tweaked the guards that were using the nuget package to work with preprocessor directives instead. Keen to keep the Windows checks in place for net6+ so that cross-platform consumers don't get unexpected errors.

Also, it seems that you're using a different username / email in your commits than on GitHub, is that intentional or did you want to change the commit author to match your GitHub profile?

This wasn't intentional, just an oversight, but I've added that email as a secondary on my account so it should all link up properly now - thanks for the prompt.

I think I've covered what's been raised, except for squashing commits - not sure if that was just a preference for future reference, or a request for this PR?

jonorossi commented 2 years ago

Apologies for the delay getting back to this - have been swamped. Hoping to get on it again later this week.

Cheers. Sorry for not getting to reviewing this again until now, last week was crazy.

I've tweaked the guards that were using the nuget package to work with preprocessor directives instead. Keen to keep the Windows checks in place for net6+ so that cross-platform consumers don't get unexpected errors.

Sounds good.

I think I've covered what's been raised, except for squashing commits - not sure if that was just a preference for future reference, or a request for this PR?

I'll have a browse through it again now. Just a preference, @stakx's commits are a clean masterpiece hiding all of his mess of back and forth 😆 .