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

[GH1781] .NET Core 2 support #1812

Merged
merged 1 commit into from Feb 21, 2018
Merged

[GH1781] .NET Core 2 support #1812

merged 1 commit into from Feb 21, 2018

Conversation

@adamhathcock
Copy link
Contributor

@adamhathcock adamhathcock commented Sep 19, 2017

#1781

Tests and Cake.exe all target - net461;netcoreapp2.0
Cake DLLs all target - netstandard2.0

Updated/adjustment references where necessary.

Notes:

  • Unit test with Encoding not working on net461 and netcoreapp2.0 but works on netcoreapp1.0 (bug report filed https://github.com/dotnet/corefx/issues/26544 )
  • Need to figure out how to run integration tests.
  • Adjust building for netcoreapp2.0
  • Adjust packaging for netcoreapp2.0
  • Adjust integration tests for netcoreapp2.0

Looking for:

  • Notes if this is the right path
  • What else to needs looking at.
@devlead
Copy link
Member

@devlead devlead commented Sep 19, 2017

Perhaps this work will sort it?
#1778

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 19, 2017

Ill see if I can merge that. I did the package updates ignored the warnings. That work should sort that.

Not sure if that will help my problem tests but we’ll see.

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Sep 20, 2017

@adamhathcock #1778 should fix the xUnit tests not running on .NET Core 2.0.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 20, 2017

The tests do work for me. There's only two specifically that don't.

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Sep 20, 2017

@adamhathcock Which are those? On my branch all tests work.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 20, 2017

Should_Transform_Xml_String_And_Xsl_String_To_Result_String_With_Utf32Xml_Declaration and Should_Add_References_To_Session

I haven't looked into detail but my first guess is that you're not actually executing the tests on netcoreapp2.0 but on netcoreapp1.0 The Xunit updates allow the .NET Core 2 SDK to run tests on either runtime which is still good.

Hopefully, today I'll take your work and merge it to see what happens.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 20, 2017

Merged into a branch locally. Gets rid of all the xunit warnings :)

I still get this error for netcoreapp2.0 and net461 (under mono) but not under netcoreapp1.0 testing:

    Cake.Common.Tests.Unit.XML.XmlTransformationTests+TheTransformMethod.Should_Transform_Xml_String_And_Xsl_String_To_Result_String_With_Utf32Xml_Declaration [FAIL]
      Assert.Equal() Failure
                ↓ (pos 0)
      Expected: <?xml version="1.0" encoding="utf-32"?>
      Actual:   <?xml version="1.0" encoding="utf-32"?
                ↑ (pos 0)

Some encoding thing that needs to be figured out I guess. The other failing test I just need to adjust compile flags.

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Sep 22, 2017

@adamhathcock FYI, my xUnit PR has just been merged.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 22, 2017

@ErikSchierboom I know :) I've already merged develop into my branch. Looking at integration tests and nuget packaging (I think there should only be one package)

build.cake Outdated

// .NET Core 2.0
var coreclrFiles2 = GetFiles( parameters.Paths.Directories.ArtifactsBinNetCore2.FullPath + "/**/*");
Zip(parameters.Paths.Directories.ArtifactsBinNetCore2, parameters.Paths.Files.ZipArtifactPathCoreClr, coreclrFiles2);

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Sep 22, 2017
Contributor

I see that the .NET Core 1.0 and 2.0 files are combined in a single zip-file, is that correct?

This comment has been minimized.

@adamhathcock

adamhathcock Sep 22, 2017
Author Contributor

Nope!

This comment has been minimized.

@ErikSchierboom

ErikSchierboom Sep 22, 2017
Contributor

Okay, I misread then :)

This comment has been minimized.

@devlead

devlead Sep 22, 2017
Member

Yip zip files should be separate, nuget if possible combined.

@devlead
Copy link
Member

@devlead devlead commented Sep 22, 2017

Yes, if it doesn't blow up too big proper tools folder with framework moniker folders should be the way to go.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 22, 2017

Now building just one nuget. Of course it's 15 megs vs 5 megs per moniker. Even though it's big, I'd rather just have one.

@devlead
Copy link
Member

@devlead devlead commented Sep 22, 2017

agreed

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 24, 2017

Currently stuck on this integration test:

Installing NuGet package xunit.assert...
Executing: /Users/adam/git/personal/cake/tests/integration/tools/nuget.exe install "xunit.assert" -OutputDirectory "/Users/adam/git/personal/cake/tests/integration/tools/Addins/xunit.assert.2.3.0-beta5-build3769" -Version "2.3.0-beta5-build3769" -Prerelease -ExcludeVersion -NonInteractive
Error: System.ComponentModel.Win32Exception (0x80004005): Operation not permitted
   at Interop.Sys.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Int32& lpChildPid, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd)
   at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo)
   at System.Diagnostics.Process.Start()
   at System.Diagnostics.Process.Start(ProcessStartInfo startInfo)
   at Cake.Core.IO.ProcessRunner.Start(FilePath filePath, ProcessSettings settings) in /Users/adam/git/personal/cake/src/Cake.Core/IO/ProcessRunner.cs:line 89
   at Cake.NuGet.NuGetPackageInstaller.InstallPackage(PackageReference package, DirectoryPath path) in /Users/adam/git/personal/cake/src/Cake.NuGet/NuGetPackageInstaller.cs:line 183
   at Cake.NuGet.NuGetPackageInstaller.Install(PackageReference package, PackageType type, DirectoryPath path) in /Users/adam/git/personal/cake/src/Cake.NuGet/NuGetPackageInstaller.cs:line129
   at Cake.Core.Scripting.ScriptProcessor.InstallAddins(IReadOnlyCollection`1 addins, DirectoryPath installPath) in /Users/adam/git/personal/cake/src/Cake.Core/Scripting/ScriptProcessor.cs:line 93
   at Cake.Core.Scripting.ScriptRunner.Run(IScriptHost host, FilePath scriptPath, IDictionary`2 arguments) in /Users/adam/git/personal/cake/src/Cake.Core/Scripting/ScriptRunner.cs:line 152
   at Cake.Commands.BuildCommand.Execute(CakeOptions options) in /Users/adam/git/personal/cake/src/Cake/Commands/BuildCommand.cs:line 35
   at Cake.CakeApplication.Run(CakeOptions options) in /Users/adam/git/personal/cake/src/Cake/CakeApplication.cs:line 45
   at Cake.Program.Main() in /Users/adam/git/personal/cake/src/Cake/Program.cs:line 74

I guess nuget.exe still has to be a dependency?

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 24, 2017

I guess I'll look into seeing why #1768 isn't being used here.

@devlead
Copy link
Member

@devlead devlead commented Sep 24, 2017

@adamhathcock it's opt in at the moment, i.e. via command line --nuget_useinprocessclient=true

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 24, 2017

I didn’t see that. So much to do :(

I’ll go over the compile flags much more closely.

This would be so much easier without mono worries.

@gep13
Copy link
Member

@gep13 gep13 commented Sep 24, 2017

@adamhathcock what mono worries?

@mholo65
Copy link
Member

@mholo65 mholo65 commented Sep 24, 2017

yeah, we'd probably want to resolve that on runtime? If it's not possible, leave it as netstandard1.6 but then we can't load netstandard2.0 addins.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 24, 2017

Basically, the above. Managing which runtime to execute on.

I think I’m just moaning that managing entry points is hard and I’d like to remove more things to make my life easier.

Just ignore me :) I don’t really have anything concrete to suggest.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 25, 2017

Hah, new problem.

The net461 tests seem to want to use .net standard 2.0 libraries. This messes up the runtime detection as it's mostly compile flag based. This means running under net461 tries to use netstandard2.0 librares. It thinks it's .net core 2.0 :( There's no proper API https://github.com/dotnet/corefx/issues/17452

I guess I can introduce some reflection.

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Sep 28, 2017

@adamhathcock Any updates?

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Sep 28, 2017

I was going down the route of breaking changes, I'm afraid.

The current issue with the tests is that it uses build time flags (as does CakeRuntime) to know what runtime is currently executing. In the .NET Standard 2.0 world, this won't fly. It actually wasn't too robust before. A .NET Standard 2.0 library can be run on net461 and higher and netcoreapp2.0 This breaks a lot of the assumptions in a few key areas of Cake.

I was creating a new Runtime enum that is similar to the TestRuntime enum that would replace CakeRuntime.TargetFramework (which only had two possible values). The new values for Runtime would be Clr, CoreClr and CoreClr2.

With these flags, the tests would better know what it's being ran on instead of using build time flags.

I was going to do some more to prove it out but I don't see a way around it with out breaking changes.

I'm going pretty slow between work and a terrible personal life at the moment :(

@patriksvensson
Copy link
Member

@patriksvensson patriksvensson commented Sep 28, 2017

@adamhathcock No worries. First make sure that you take care of yourself and the personal life. Cake is always there, so no rush with the PR :)

You're raising a good point with the ICakeRuntime stuff that we need to find some solution to. The architecture behind that choice was made pre .NET Core 1.0, so many assumptions about things might be wrong now.

@cake-build/cake-team Any input on this?

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Sep 28, 2017

@adamhathcock Breaking changes may be needed, as you are basically supporting an entire new platform. Good luck with your personal situation!

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Oct 18, 2017

@adamhathcock @patriksvensson Is there any plan on how to proceed with this PR?

Copy link
Member

@mholo65 mholo65 left a comment

Great work! Some minor findings/questions.

I'm going to take this for a spin now to test it out. I'll approve if everything works as expected (debugging, intellisense, etc...).

build.cake Outdated
CopyFileToDirectory("./LICENSE", parameters.Paths.Directories.ArtifactsBinFullFx);
CopyFileToDirectory("./LICENSE", parameters.Paths.Directories.ArtifactsBinNetCore);
CopyFileToDirectory("./LICENSE", parameters.Paths.Directories.ArtifactsBin + "/net461");
CopyFileToDirectory("./LICENSE", parameters.Paths.Directories.ArtifactsBin + "/netcoreapp2.0");

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Why was this changed?

This comment has been minimized.

@adamhathcock

adamhathcock Feb 7, 2018
Author Contributor

It was necessary when I added a third target but that changed. Will revert

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<metadata
xmlns="http://schemas.microsoft.com/packaging/2010/07/nuspec.xsd">

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

This formatting hurt my eyes 😄 Please change back.

@@ -182,7 +182,8 @@ public void Should_Transform_Xml_String_And_Xsl_String_To_Result_String_With_Xml
Assert.Equal(htm, result, ignoreLineEndingDifferences: true);
}

[Fact]
// this feels wrong to be CLR only
[RuntimeFact(TestRuntime.Clr)]

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Why is this needed?

This comment has been minimized.

@devlead

devlead Jan 29, 2018
Member

Because dotnet/corefx#26544

This comment has been minimized.

<PropertyGroup>
<AssemblyName>Cake.Common.Tests</AssemblyName>
<TargetFrameworks>net46;netcoreapp1.0</TargetFrameworks>
<PlatformTarget>anycpu</PlatformTarget>
<TargetFrameworks>net461;netcoreapp2.0</TargetFrameworks>

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Why not net46 as Cake.Common targets net46 and netstandard2.0?

@@ -1,57 +1,45 @@
<Project Sdk="Microsoft.NET.Sdk">

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Don't know... I kind of liked the empty rows. I know that at least Rider doesn't like them and is always removing them for me.

This comment has been minimized.

@adamhathcock

adamhathcock Feb 7, 2018
Author Contributor

I'd rather leave it consistent with no empty lines.

// Not a valid path. Try loading it by its name.
return Assembly.Load(new AssemblyName(path.FullPath));
// Not a valid path. Try loading it by its name.
return Assembly.Load(new AssemblyName(path.FullPath));

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Wrong indentation?

@@ -31,17 +31,35 @@ public static bool Is64BitOperativeSystem()
public static PlatformFamily GetPlatformFamily()
{
#if NETCORE
if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
try

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Why was these try-catch introduced? At what point will we see PlatformNotSupportedException?

This comment has been minimized.

@adamhathcock

adamhathcock Feb 7, 2018
Author Contributor

Not all platforms support this kind of probing. I saw this in another code base that was doing the same thing to try to determine the environment.

<PropertyGroup>
<AssemblyName>Cake.NuGet.Tests</AssemblyName>
<TargetFrameworks>net46;netcoreapp1.0</TargetFrameworks>
<PlatformTarget>AnyCpu</PlatformTarget>
<TargetFrameworks>net461;netcoreapp2.0</TargetFrameworks>

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Why not net46 as Cake.NuGet targets net46 and netstandard2.0?

/// </summary>
/// <returns>The target framework.</returns>
FrameworkName TargetFramework { get; }
Runtime Runtime { get; }

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Breaking changes... We sould also bump the breaking change version here

This comment has been minimized.

@adamhathcock

adamhathcock Feb 7, 2018
Author Contributor

Fixed

<PropertyGroup>
<AssemblyName>Cake.Testing.Xunit</AssemblyName>
<TargetFrameworks>net46;netstandard1.6</TargetFrameworks>
<TargetFrameworks>netstandard2.0</TargetFrameworks>

This comment has been minimized.

@mholo65

mholo65 Jan 29, 2018
Member

Should this also target net46?

@mholo65
Copy link
Member

@mholo65 mholo65 commented Jan 29, 2018

Debugging (.NET Framework and .NET Core) + Intellisense seems to work fine. 👍

@devlead
Copy link
Member

@devlead devlead commented Feb 6, 2018

@adamhathcock ping us when you've had a chance to look at the last feedback given by @mholo65, and we'll then do a final review :shipit:

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Feb 7, 2018

In general, I don't think there should be a net46 target and should let downstream projects move up to the net461 service release which is also netstandard2.0 but that's not my call. I'm fixing the targetting and some formatting things.

@mholo65
Copy link
Member

@mholo65 mholo65 commented Feb 7, 2018

@adamhathcock, unfortuneately there must be a net46 target for intellisense to work today. OmniSharp.exe loads Cake.Core.dll from the cake path in order to get the Script Host object type.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Feb 7, 2018

I've read through OmniSharp/omnisharp-roslyn#915 and it feels depressing. Oh well.

@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Feb 7, 2018

There's still some issue because NuGet.PackageManagement.NetStandard targets a netstandard 1 but not 2. It's probably not a real problem but it's annoying to see package downgrade notices.

@adamhathcock adamhathcock force-pushed the adamhathcock:netcore2 branch from b0b1e8f to b4a378a Feb 7, 2018
@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Feb 7, 2018

Updated and commits flattened

@adamhathcock adamhathcock force-pushed the adamhathcock:netcore2 branch from b4a378a to 02632b6 Feb 7, 2018
@adamhathcock
Copy link
Contributor Author

@adamhathcock adamhathcock commented Feb 7, 2018

Updated the Constants.cs to 0.26

@joskoanicic
Copy link

@joskoanicic joskoanicic commented Feb 13, 2018

When this gets pulled is there going to be a new release shortly after or will you wait for some other changes to get into the next release? Just evaluating if I should wait for the official release. (Sorry if this is not the place to ask this question, not looking to spam).

@devlead
Copy link
Member

@devlead devlead commented Feb 13, 2018

@joskoanicic yes, next release will be this PR.

@cgibbons
Copy link

@cgibbons cgibbons commented Feb 21, 2018

Any indication on when this will be merged and a release cut? I'm trying to find a build tool that meets my needs, i.e. builds in a docker container whether windows or linux with no mono or full net framework dependencies.

@mholo65 mholo65 merged commit 9d62efa into cake-build:develop Feb 21, 2018
6 checks passed
6 checks passed
Cake Develop (Cake) TeamCity build finished
Details
CodeFactor No issues found.
Details
ci/bitrise/7a9d707b00881436/pr Passed - OSX Cake
Details
ci/bitrise/b811c91a26b1ea80/pr Passed - Ubuntu Cake
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@mholo65
Copy link
Member

@mholo65 mholo65 commented Feb 21, 2018

@adamhathcock thank you for your contribution! 👍 Awesome work!

@ErikSchierboom
Copy link
Contributor

@ErikSchierboom ErikSchierboom commented Feb 22, 2018

Thanks a lot @adamhathcock! Looking for the next release.

@gep13
Copy link
Member

@gep13 gep13 commented Feb 23, 2018

@adamhathcock thanks again for doing this work! Really great work! 😄

@adamhathcock adamhathcock deleted the adamhathcock:netcore2 branch Feb 23, 2018
@SeanFarrow
Copy link

@SeanFarrow SeanFarrow commented Feb 23, 2018

@gep13
Copy link
Member

@gep13 gep13 commented Feb 23, 2018

@SeanFarrow soon.

I know that isn't the best answer, but we are just confirming a few things, prior to the release.

@bonesoul
Copy link

@bonesoul bonesoul commented Dec 16, 2018

any updates on this?

@devlead
Copy link
Member

@devlead devlead commented Dec 16, 2018

@bonesoul merged and shipped a several months ago.

@bonesoul
Copy link

@bonesoul bonesoul commented Dec 16, 2018

sorry i guess i missed stuff on documentation.

still couldn't find how to get started with coreclr version.

update: sorry found it: https://cakebuild.net/blog/2018/08/cake-v0.30.0-released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.