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

Add net6.0 as a target framework #616

Merged
merged 15 commits into from May 9, 2022

Conversation

Jevonius
Copy link
Contributor

@Jevonius Jevonius commented Apr 8, 2022

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

Code Access Security (CASE) is only supported on .Net Framework; from net5.0 onwards it is marked obsolete which causes build warnings. For now, easiest to disable the warnings. If/when fully deprecated, it may be necessary to be a bit cleverer.
…ling when not on Windows

Marshal.GetIUnknownForObject causes a CA1416 warning unless the code is only running on Windows. It is unclear how well Mono handles COM interoperability, so whether special handling is needed for Mono.
…tests for `DiagnostlicsLogger`

There are `CAS1416` errors if attempting to use `EventLog` on `net6.0` without handling to ensure the platform is Windows, hence the need to use `SupportedOSPlatform` at the class level for the DiagnosticsLogger-related classes.
@Jevonius
Copy link
Contributor Author

Jevonius commented Apr 8, 2022

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 Jevonius marked this pull request as ready for review April 8, 2022 23:52
@Jevonius
Copy link
Contributor Author

Jevonius commented Apr 9, 2022

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
Copy link
Contributor Author

Jevonius commented Apr 9, 2022

Need to update README and CHANGELOG. Done.

The sole usage of DOTNET462 (PublicApiTestCase) now uses NET6_0. There are symbols such as NET462 and NET462_OR_GREATER (supplied by the SDK) if such switching is required in future.
Copy link
Member

@stakx stakx left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me! 👍 Apart from some comments, I really only have a single request:

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.

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?

README.md Show resolved Hide resolved
src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
src/Castle.Core/DynamicProxy/ProxyGenerator.cs Outdated Show resolved Hide resolved
ref/Castle.Core-net6.0.cs Show resolved Hide resolved
src/Castle.Core.Tests/PublicApiTestCase.cs Outdated Show resolved Hide resolved
src/Castle.Core.Tests/PublicApiTestCase.cs Show resolved Hide resolved
@stakx
Copy link
Member

stakx commented Apr 15, 2022

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.

src/Castle.Core.Tests/Castle.Core.Tests.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Outdated Show resolved Hide resolved
src/Castle.Core/Castle.Core.csproj Show resolved Hide resolved
src/Castle.Core/DynamicProxy/ProxyGenerator.cs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@Jevonius
Copy link
Contributor Author

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

@Jevonius
Copy link
Contributor Author

Jevonius commented Apr 30, 2022

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
Copy link
Member

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 😆 .

@jonorossi jonorossi added this to the v5.0.0 milestone May 9, 2022
@jonorossi jonorossi merged commit 51f4f92 into castleproject:master May 9, 2022
@Jevonius Jevonius deleted the add-net6-targets branch May 9, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants