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

Fix BadImageFormatException on .NET Core 3.0 #1057

Merged
merged 11 commits into from Jun 2, 2019

Conversation

Projects
None yet
4 participants
@lg2de
Copy link
Contributor

commented May 29, 2019

This is a PR to provide testing environment for .NET Core for now. We would like to verify e.g. #1047.
Later this PR can be used to add the target for 3.0 in next release.

Regarding double format changes please note https://devblogs.microsoft.com/dotnet/floating-point-parsing-and-formatting-improvements-in-net-core-3-0/

I would recommend to adopt the expectation because this seems to be default behavior in .NET Core 3.

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented May 29, 2019

I think this doesn't work because we still use msbuild (through a Cake API).

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

Also from build.cake

settings.ToolVersion = MSBuildToolVersion.VS2017;
@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

And from https://github.com/cake-build/cake/releases there has been some updates from the 0.30.0 that we use to 0.33.0 about NugetRestore and msbuild 16.

@lg2de

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

With the recent comment the build.ps1 was running fine on my PC.
I guess the build server need to have installed the .NET Core 3 SDK.

Lets retry when appveyor has finished the VS2019 (16.1) image.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

Could it be that the build server do have 3.0 installed, but since it's preview it's not picked by default?

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 29, 2019

Hmm... I can't really see from the build logs if:

  • .net core 3.0 is not installed,
  • Restore-NuGet-Packages can't find .net core 3.0, or
  • Restore-NuGet-Packages actively chooses .net core 2.2 over .net core 3.0

Raising the logging level from DotNetCoreVerbosity.Normal to DotNetCoreVerbosity.Diagnostic
could maybe shed some light on those questions.

Maybe related links:
https://help.appveyor.com/discussions/problems/22448-dotnet-core-30-and-netstandard-21
cake-build/cake#2519

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I pushed a commit that switched our Nuget restore and build steps to dotnet. The Visual Studio 2019 Preview agent has SDK 3.0.100-preview-009812 installed. It now complains about C# issues.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

I don't see any complaints about C# issues?
It fails before finishing restoring nuget packages.

Do we want to add a netcoreapp3.0 target of the Library?
It should suffice to add a netcoreapp3.0 test project.

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I don't see any complaints about C# issues?

I got the entire script running on my local machine, and it even reports the IL errors reported in #1047.

Do we want to add a netcoreapp3.0 target of the Library?

Not until we need to support assertions on specific types introduced by .NET Core 3.0

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

Aha, the #1047 c# issues causing runtime problems.
I thought you were calling out some sort of c# compile time issues.

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I thought you were calling out some sort of c# compile time issues.

As well. But that's probably because that agent is using an old preview of .NET Core 3.0

C:\projects\fluentassertions\Tests\Shared.Specs\MethodBaseAssertionSpecs.cs(829,77): error CS8107: Feature 'private protected' is not available in C# 7.0. Please use language version 7.2 or greater.

I'm trying to figure out a way to install the right dotnet SDK as part of our build process. We do this as well in our projects.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

@lg2de As #1060 has been merged it should now be possible to build .net core 3.0 targets.
As we're not utilizing any .net core 3.0 features (yet!), please remove that from the core library target frameworks.
When I tried locally I could reproduce #1047 with a netcoreapp2.0 core library and a netcoreapp3.0 test project.

@lg2de lg2de changed the title add target for .NET Core 3.0 Fix BadImageFormatException on .NET Core 3.0 May 31, 2019

@lg2de

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I have reworked as requested and added the fix mentioned by @jnyrup.
Please review my changes and comment the TODOs added.
The build failed now "only" due to timing issues.

@dennisdoomen
Copy link
Member

left a comment

Great work. Only one remark.

Show resolved Hide resolved Tests/Benchmarks/Benchmarks.csproj
@jnyrup
Copy link
Collaborator

left a comment

Thanks for the rework, only a few comments from here.
(I really dislike that GitHub marks "request changes" with a big red alarming cross) It has changed

@@ -14,7 +14,7 @@
<Authors>Dennis Doomen;Jonas Nyrup</Authors>
<PackageDescription>
A very extensive set of extension methods that allow you to more naturally specify the expected outcome of a TDD or
BDD-style unit tests. Targets .NET Framework 4.5 and 4.7, as well as .NET Core 2.0, .NET Standard 1.3, 1.6 and 2.0.
BDD-style unit tests. Targets .NET Framework 4.5 and 4.7, as well as .NET Core 2.0, .NET Core 3.0, .NET Standard 1.3, 1.6 and 2.0.

This comment has been minimized.

Copy link
@jnyrup

jnyrup Jun 1, 2019

Collaborator

We're no longer targeting net core 3.0

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Jun 1, 2019

Member

But we do support it officially. So I would keep it in.

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen Jun 1, 2019

Member

Maybe the PR can also update the docs folder.

This comment has been minimized.

Copy link
@lg2de

lg2de Jun 1, 2019

Author Contributor

Done.
Please check.

Show resolved Hide resolved Src/FluentAssertions/FluentAssertions.csproj Outdated
Show resolved Hide resolved build.cake
Show resolved Hide resolved Tests/Shared.Specs/Utilities.cs Outdated
Show resolved Hide resolved Tests/Shared.Specs/EventAssertionSpecs.cs Outdated

lg2de added some commits Jun 1, 2019

@dennisdoomen dennisdoomen requested a review from jnyrup Jun 1, 2019

@jnyrup

jnyrup approved these changes Jun 2, 2019

Copy link
Collaborator

left a comment

Looks great!

@jnyrup jnyrup merged commit c7f0e65 into fluentassertions:master Jun 2, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details

@lg2de lg2de deleted the lg2de:core3 branch Jun 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.