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

#1175 - NetStandard #1269

Merged
merged 90 commits into from
Jun 28, 2018
Merged

#1175 - NetStandard #1269

merged 90 commits into from
Jun 28, 2018

Conversation

dazinator
Copy link
Member

@dazinator dazinator commented Jul 29, 2017

Current Status

GitVersionTask is working on dotnet core.
Also there is a new nuget package called: GitVersion.CommandLine.DotNetCore which contains the GitVersion commandline, except built for dotnet core, so you can run GitVersion via dotnet GitVersion.dll.
And there is a new dotnetcore based docker image.

To try out the build task:

  1. Install the dotnet core sdk. (Need a minimum of version 1.1 installed).
  2. Add the appveyor nuget feed: https://ci.appveyor.com/nuget/gitversion-8nigugxjftrw
  3. Add the nuget package:
 <PackageReference Include="GitVersionTask" Version="4.0.0-pullrequest1269-1535">
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>

Here is a sample consumer project building on appveyor with dotnet sdk

Be aware when building or restoring using dotnet commands you need to use the --disable-parallel flag (#1381)

To try GitVersion DotNetCore CLI

  1. Install the dotnet core runtime. (Need a minimum of version 2.0 installed).
  2. Add the appveyor nuget feed: https://ci.appveyor.com/nuget/gitversion-8nigugxjftrw
  3. Install the GitVersion.CommandLine.DotNetCore nuget package.
  4. CD into the /tools folder of the nuget package and run: dotnet gitversion.dll

Also there are a couple of artifacts in this PR not yet being built / uploaded to appveyor:

  • Tfs Build Task
  • Gem Package

This is because since converting the projects to sdk style, the AfterTargets logic in some of the csproj files doesn't seem to run. I have moved most of the logic (for all of the other artifacts) out of the csproj files and into the cake script instead - but haven't yet done the TFS or the Gem package. If someone has some time to contribute, please start there..

Remaining Tasks

  • Get the build to produce TFS task and Gem package again.

Completed Tasks

  • Updated all of the projects to SDK style (vs2017) format.
  • Upgraded various dependencies, including GitTools.Core, GitTools.Testing and a few others to newer versions that that support netstandard.
  • GitVersion command line now builds for netcoreapp20 in addition to just net40 and the additional nuget package is uploaded to appveyor.
  • Removed Pepita (now use standard /t:Pack mechanism)
  • Removed Fody
  • GitVersion.sln now builds using dotnet core sdk, so this might pave the way to building under a
    linux/dotnet core image, as opposed to linux/mono image.
  • Removed ILMerge and ILRepack from the GitVersionTask project. The DESKTOP version of the assembly has all of its dependencies included as separate dll's inside the nuget package. The .NETCORE version of the assembly, has all of it's dependencies in it's nuspec, and they are restored and resolved at runtime thanks to UtilPack and therefore doesn't need to worry about embedding them (which is fortunate because ILMerge and Costura do not work with netcore assemblies so I know no way of embedding assemblies with netcore anyway).
  • Removed dependency that ContinuaCI build server support had on registry (I fixed this separately in master and merged that into this pr)
  • Removed dependency on System.Web (I fixed this separately Remove System.Web dependency #1274 in master and merged that into this pr)

Other Notes

  • Encoding.GetEncodings() isn't available on NETStandard1.3. (it's in netstandard2 I am manually populating a collection of common encodings for netstandard.

  • Web Requests look different in netstandard api. (had to use compilation directives and alternative methods in many places to get to compile)

  • I am using AsyncBridge nuget package for async / await support in net40 of GitVersionCore. I have replaced Thread.Sleep with Task.Delay - as the former not available under netstandard and the latter is better practice anyway.
    -System.ComponentModel.WarningException --> GitTools.WarningException

  • I have moved a lot of the packaging logic out of csproj files, and into the cake script.

  • I added a Directory.Build.props file to control some common dependency versions at a global level.

Haven't been able to resolve some compilation errors yet due to lack of replacemnt API's on netstandard.
@asbjornu
Copy link
Member

asbjornu commented Aug 3, 2017

@dazinator: I would love to review this, but I have no experience with .NET Core and can thus provide very little guidance. Sorry. 😕

@GeertvanHorrik
Copy link
Contributor

I had to remove ContinuaCI support from NETStandard build - as lack of registry API's.

Why does it need access to the registry? It should just check if environment variables are registered, same as on Cake:

https://github.com/cake-build/cake/blob/08907d1a5d97b66f58c01ae82506280882dcfacc/src/Cake.Common/Build/ContinuaCI/ContinuaCIProvider.cs#L62

@GeertvanHorrik
Copy link
Contributor

Just a discussion in general: .NET standard 2.0 is around the corner. That will add a lot of API's to .NET standard that are being worked around in this PR. I think it's wiser to wait for .NET standard 2.0 to be available (or work against the prerelease versions for the meantime).

@agc93
Copy link

agc93 commented Aug 15, 2017

Given .NET Core 2.0 has now been released, is the plan to update to 2.0, or to continue with this work?

As I've mentioned to @JakeGinnivan the current non-Core GitVersion doesn't work on a huge number of Linux platforms with no workaround available..

@dazinator
Copy link
Member Author

dazinator commented Aug 16, 2017

My own personal view is that:

  1. Yes netstandard2 might make this easier. (Would help to verify using api portability analysis tool)
  2. We shouldn't really require netstandard2 and should aim for the lowest netstandard thats reasonable, longer term.

All of the issues mentioned at top of this PR point to things we probably want to tidy up regardless:

  • Relying on system.web for example, is madness and its not unreasonable to want to eliminate this
  • Relying on the registry - fundamentally is windows only.
  • Encodings Apis - neccesary?

I feel gitversion core should atleast be able to match the netstandard level targeted by libgit2sharp.

If anyone wants to aim for netstandard2 as a first step though - nothing stopping you and its better than nothing. Many of the dependencies are now all compatible so its just waiting for someone to do this work! Feel free to base / cherry pick from this branch if it helps.

@dazinator
Copy link
Member Author

dazinator commented Aug 16, 2017

@GeertvanHorrik - have raised #1278 for the continua ci registry issue

@dazinator
Copy link
Member Author

Going to replace Thread.Sleep with Task.Delay. Thread.Sleep isn't available in desired netstandard version.

@GeertvanHorrik
Copy link
Contributor

And Task.Delay is much better (doesn't block CPU).

@dazinator
Copy link
Member Author

dazinator commented Aug 16, 2017

Yup exactly. Sadly Task.Delay isn't available for net40. So I have had to do this (found here)

 public static class TaskHelper
    {

#if NETDESKTOP
        public static Task Delay(int milliseconds)
        {
            var tcs = new TaskCompletionSource<bool>();
            System.Timers.Timer timer = new System.Timers.Timer();
            timer.Elapsed += (obj, args) =>
            {
                tcs.TrySetResult(true);
            };
            timer.Interval = (double)milliseconds;
            timer.AutoReset = false;
            timer.Start();
            return tcs.Task;
        }
#else
        public static Task Delay(int milliseconds)
        {
            return Task.Delay(milliseconds);
        }
#endif
    }

So we call await TaskHelper.Delay(x) now instead.

I have taken another look at the GetEncodings api (or lack thereof), I've decided to workaround this for netstandard by explivitly supporting an easily accessible set of encodings:

#if NETDESKTOP
            var encodings = (Encoding.GetEncodings());
           
#else
            // GetEncodings not available on netstandard, so just manually adding some common ones.
            var encodings = new List<Encoding>() { Encoding.ASCII, Encoding.Unicode, Encoding.UTF32, Encoding.UTF7, Encoding.UTF8 };

#endif

Do you think that would be acceptable?

@dazinator
Copy link
Member Author

dazinator commented Aug 16, 2017

Hmm so I am struggling to get Tasks to compile in net40.

GitVersionCache.cs(49,13,49,48): error CS1061: 'Task' does not contain a definition for 'GetAwaiter' and no extension method 'GetAwaiter' accepting a first argument of type 'Task' could be found (are you missing a using directive or an assembly reference?)
1> Helpers\ThreadSleep.cs(9,13,9,49): error CS1061: 'Task' does not contain a definition for 'GetAwaiter' and no extension method 'GetAwaiter' accepting a first argument of type 'Task' could be found (are you missing a using directive or an assembly reference?)

I thought adding the https://www.nuget.org/packages/Microsoft.Bcl.Async nugget package to net4 projects was enough. Perhaps its different with the new csproj file format (i.e PackageReference). I'm checking in what I have done so far, if anyone can resolve this please do. Could we even frop support for net40 and make net45 the new minimum?

UPDATE: Using https://www.nuget.org/packages/AsyncBridge/ seems to work.

…h to `Task.Delay` instead of Thread.Sleep but task's won't compile for net40 for some reason, even with the bcl.async nuget package.
@dazinator
Copy link
Member Author

@JakeGinnivan I'm planning to remove ILMerge in favour of Fody weaving of the msbuild task - think that will simplify things. I'll upgrade the project to vs2017 multitargeting, and remove pepita as well as that should no longer be required.

@dazinator dazinator changed the title #1175 - First pass at GitVersionCore and netstandard - wip. #1175 - NetStandard Aug 17, 2017
@dazinator
Copy link
Member Author

Seperate PR submitted that removes System.Web!

@dazinator
Copy link
Member Author

@kll Sorry to hear of your docker struggles. I am eager to see this PR merged really, without expanding it any further. It delivers what it set out to do. I think it would be good to get this merged and then add support for the 2.1-sdk docker image as a separate issue.

@dazinator
Copy link
Member Author

@JakeGinnivan I am the wrong person to ask about Mac. I thought in theory it should work - can anyone offer any advice to Jake on this?

@kll
Copy link
Contributor

kll commented Jun 13, 2018

I'll start on a new PR based on this one but will keep it distinct so this one can be merged first.

In theory at least re-targeting everything including the test projects to just netstandard2.0/netcoreapp2.0 should allow it to work on a mac.

@dazinator
Copy link
Member Author

@kll ok sounds good. Is there anyway you could add netstandard2.0 as an additional target rather than retarget everything to 2.0 as a new baseline? I'd love to keep support for 1.1 if possible

@kll
Copy link
Contributor

kll commented Jun 14, 2018

@dazinator Unfortunately not since LibGit2Sharp only targets netstandard2.0. We are limited by what the libraries we consume target. I got a good start yesterday and I think its a positive change because a lot of the extra complexity that was added in to work around the missing APIs in netstandard1.3 can go back away now since they are present in netstandard2.0.

@dazinator
Copy link
Member Author

dazinator commented Jun 14, 2018

@kll

LibGit2Sharp only targets netstandard2.0

Yeah.. the official release does you are right. This PR though uses a beta package which I think supports netstandard1.5 (or 1.3? can't remember) So to add netstandard2.0 support I was thinking something like this:


<ItemGroup Condition="'$(TargetFramework)'=='netstandard1.5'">
    // current packages as per this pr.
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0'">
    // newer libgit2sharp package targeting netstandard20
</ItemGroup>

However that is a lot of build complexity and just an idea. If this PR gets merged I think most people (inlcuding me) would be perfectly fine with moving the baseline to netstandard20 as of a following release :-)

@JakeGinnivan
Copy link
Contributor

Thoughts on just bringing GitTools.Core into this repo, I don't think it's being used by anything else properly? May simplify things going forward

@petriashev
Copy link

Any plans for release?

@dazinator
Copy link
Member Author

dazinator commented Jun 19, 2018

@JakeGinnivan - probs a good idea to bring gittools in if its not being consumed independently anywhere else - but can we get this PR merged first and track that as a seperate issue if necessary?

Also what are your thoughts about this PR - can it be merged now? Or are there any specific things that need to be addressed? From my perspective its just the tfs task and gem package assets are no longer being produced, but we can extend the cake script to handle that after this is merged, so not sure if that should be a blocker.

@dazinator
Copy link
Member Author

I prefer not to merge my own PRs (just a policy of mine) but in order to get this moving:

Unless there are any objections / concerns raised then in two weeks I will merge this myself. That is unless someone else wants to do the honours :-)

@pauldotknopf
Copy link

2 weeks? Geesh!

I say, by the power granted to me by Mr. Internet himself, you have until Monday of next week to raise your concerns. Or forever hold your peace!

@JeremyCade
Copy link

@dazinator just merge, release a alpha. The world shall go forth and break from there.

@JakeGinnivan
Copy link
Contributor

JakeGinnivan commented Jun 21, 2018

I am fine with you merging @dazinator. Lets just merge, release, and the community can help us straighten it out

Lets get back on the releasing train.

@bugproof
Copy link

bugproof commented Jun 28, 2018

@dazinator

GitVersionTask is working on dotnet core.

So if I understand correctly in order to use GitVersionTask with dotnet core projects I have to use appveyor nuget feed and version 4.0.0-pullrequest1269-1535? It doesn't work with nuget.org package?

@dazinator dazinator merged commit 62df1d0 into GitTools:master Jun 28, 2018
@dazinator
Copy link
Member Author

@LeCoque that's presently correct, however I have just merged this PR, hopefully an official nuget.org release will occur soon - will update back here once that has happened.

@pauldotknopf
Copy link

Any update on this?

@dazinator
Copy link
Member Author

@pauldotknopf - This needs sorting: #1445

@pauldotknopf
Copy link

Is this the package we should be using?

https://www.nuget.org/packages/GitVersionCore

@asbjornu
Copy link
Member

asbjornu commented Dec 3, 2018

@pauldotknopf: That depends on how you're using GitVersion. If you use it as a library, that's the right reference. If you use it as a command line executable, GitVersion.CommandLine.DotNetCore is the right one. If you use it as an MSBuild Task, GitVersionTask is the right one.

@dazinator
Copy link
Member Author

There is also now the dotnet cli global tool which is probably easier to consume if you want the command line usage (on any platform):

install instructions here: https://www.nuget.org/packages/GitVersion.Tool

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.