Port to .NET Core #1503

Merged
merged 59 commits into from Jan 21, 2017

Projects

None yet

3 participants

@mderriey
Contributor

As mentioned in #1419 I'm trying to port Octokit to .NET Core.

Very early stage at the moment but I'd rather PR and get feedback quickly.
Right now only targeting netstandard1.1 and net45.

Any comment/feedback more than welcome.

ryangribble and others added some commits Sep 28, 2016
@ryangribble ryangribble Create a new build framework for Octokit.Next using Cake.Frosting
f12f538
@ryangribble ryangribble Exclude new "build" folder from the FAKE dotnet core build step, whic…
…h previuosly picked up all project.json files in the whole tree
c500757
@ryangribble ryangribble Tidy up formatting 12796ce
@ryangribble ryangribble set nicer task names 75d51fb
@ryangribble ryangribble Add task to update project.json versions 921ba80
@ryangribble ryangribble comment out existing appveyor commands and add new commands to dotnet…
… restore+run the cake.frosting build script
270488c
@ryangribble ryangribble playing with appveyor
d357e68
@ryangribble ryangribble see if a powershell script is better than multiple appveyor commands
70b9b5c
@ryangribble ryangribble Remove ps1 bootstrap in favour of 2 native appveyor commands
Fix working directory to handle being run from build directory or root directory
6ce9498
@ryangribble ryangribble add clean task
c06f47f
@mderriey mderriey remove all .csproj and packages.config files c910f24
@mderriey mderriey port Octokit to .NET Core targeting netstandard1.1 and net45 ad561d7
@mderriey mderriey remove all .csproj and packages.config for Octokit.Reactive 3658ba0
@mderriey mderriey port Octokit.Reactive to .NET Core targeting netstandard1.1 and net45
f0e6dbb
@mderriey mderriey remove silly line in Octokit project.json 2d1a634
@mderriey mderriey fix missing parameter in FormatUserAgent after previous change 77c8a30
@mderriey mderriey port Octokit.Tests to .NET Core targeting netcoreapp1.0. One test kee…
…ps failing
c95cd15
@mderriey mderriey fix the failing test - hopefully in a not too ugly way e69169c
@mderriey mderriey port Octokit.Tests.Conventions to .NET Core targeting netcoreapp1.0
3e3cb85
@mderriey mderriey change AppVeyor and Travis configs to get build and tests running on CI
a6206ad
@mderriey mderriey Merge remote-tracking branch 'gh/master' into port-to-dotnet-core
# Conflicts:
#	Octokit/Clients/IOrganizationsClient.cs
#	Octokit/Octokit-Mono.csproj
#	Octokit/Octokit-MonoAndroid.csproj
#	Octokit/Octokit-Monotouch.csproj
#	Octokit/Octokit-Portable.csproj
#	Octokit/Octokit-netcore45.csproj
#	Octokit/Octokit.csproj
44f6012
@mderriey mderriey remove integration tests restoring since they have not been ported to…
… .NET Core yet
b3b4c91
@mderriey mderriey thou shall not learn F# that quickly
6748d6b
@mderriey
Contributor

I'm struggling to get the FAKE script running. If a kind soul comes by and spots the error 🤗 Otherwise I'll take another look at it a bit later.

@ryangribble
Collaborator
ryangribble commented Nov 25, 2016 edited

I can't help on the Fake front so much, but I'd like to combine the CAKE changes I was working on (#1440 / #1476) with this branch since it seems logical to me to do the cake switch as part of the dotnet core work. I should be able to do some work on this this weekend

@mderriey
Contributor

Sweet, that'd be great!

ryangribble added some commits Nov 27, 2016
@ryangribble ryangribble Merge remote-tracking branch 'upstream/master' into cake-frosting
7fa7e51
@ryangribble ryangribble Merge branch 'cake-frosting' into port-to-dotnet-core
# Conflicts:
#	appveyor.yml
292976b
@ryangribble ryangribble remove sample "Octokit.Next" projects ecee392
@ryangribble ryangribble Include the dnx projects in cake script and remove Octokit.Next 719ad73
@ryangribble ryangribble remove Octokit.Next from old FAKE script 6cf47b6
@ryangribble ryangribble Add Package cake step d210739
@ryangribble ryangribble make project.json formats compatible with json writer
cd35b7d
@ryangribble
Collaborator
ryangribble commented Nov 27, 2016 edited

I merged your changes in with mine from #1476 and built+tested everything fine on AppVeyor using the Cake build 😁

Basically the cake build is doing GitVersion to calculate version number, package restore, build, test and then packaging nuget packages, setup as artifacts in AppVeyor. I also removed the Octokit.Next projects since they were just a sample before you got stuck into the real thing.

As youll see from the commands in the AppVeyor.yml file - to run the build you just change into ./build directory and dotnet restore dotnet run the Cake.Frosting build project. Or if you want to debug the build you simply open up /build/build.sln in Visual Studio and hit F5. Ultimately I will make it easier to run a build (eg a bootstrap build.ps1 in the root directory) but for now its just the 2 commands, until we actually delete the Fake f# and associated scripts

Take a look at it on branch https://github.com/TattsGroup/octokit.net/tree/port-to-dotnet-core and merge it into your branch if you think it's a go 👍

@mderriey
Contributor

Wow, great, I'll take a look at it soon. Thanks 👍

mderriey added some commits Nov 29, 2016
@mderriey mderriey replicate AppVeyor config in Travis CI
4879a69
@mderriey mderriey use forward slashes in Travis CI build file
283d85a
@mderriey
Contributor

Thank you so much @ryangribble!
Next steps:

  • get the integration test project ported too, and restored/built/tested in CI
  • bump System.Reactive to v3.x

@shiftkey based on #1419 (comment), should I just get rid of net45 and keep just netstandard1.1?

@shiftkey
Member

@mderriey I'd leave net45 in there unless you're having specific issues with it

mderriey added some commits Dec 10, 2016
@mderriey mderriey add Octokit.Integration to the .NET Core solution 3c0078b
@mderriey mderriey add Octokit.Tests.Integration to the build
cee3d85
@mderriey mderriey fix find/replace gone wrong
ac21f8a
@mderriey
Contributor

@shiftkey / @ryangribble

The integration test project has been ported to netstandard1.1 and it turns out System.Reactive had already been bumped to 3.x since it's the only version compatible with netstandard.

What I think would be good is for you to have a look at some specific methods that were ported to .NET Core and see if you're happy with the implementation:

There might be others, I'll have deeper look.
Could you also let me know what you think is missing / what we have to focus on to get closer to a merge?

Thanks for the support, it's really appreciated 👍

@ryangribble

The change to the Serializer exception is 👍

The changes to the test are 👍
(I briefly played around trying to get dynamic and ExpandoObject etc working but couldnt... so I think the approach you've settled on is the least amount of ugly to still succeed :) Unless the whole test can be written as most other tests are, where the mock is setup to match the call parameters then we just verify .Received(1) on the mock... rather than the way this one uses the Arg.Do to assign the passed in variable to another varialbe and later assert it.

But really it's just a test and whatever works is fine I reckon...

The change to the user agent string I think there might actually be a problem with the old format line being left in place on one of the #elif cases... ive marked it up as a review item

Octokit/Http/Connection.cs
+ productInformation,
+ RuntimeInformation.OSDescription,
+ RuntimeInformation.OSArchitecture.ToString().ToLowerInvariant(),
+#elif NETFX_CORE
"{0} ({1} {2}; {3}; {4}; Octokit {5})",
@ryangribble
ryangribble Dec 22, 2016 Collaborator

since format is being set above and included in all cases, I think this line may be spurious?

infact do we even really need this hardcoded WindowsRT/Windows8 etc stuff below @shiftkey?

Can this NETFX_CORE case here just be removed and either user Environment or RuntimeInformation according to the HAS_ENVIRONMENT preprocessor definition now added?

@mderriey
mderriey Dec 23, 2016 Contributor

noted, thanks. for now I removed the WindowsRT/Windows8 specific stuff since I don't think the target is to create platform-specific packages.

@ryangribble

A few more review items

- <ProjectGuid>2740af44-cb2e-4fd8-a221-248693978a0c</ProjectGuid>
- <RootNamespace>Octokit.Next.Tests</RootNamespace>
+ <ProjectGuid>a04124dc-d057-4db0-8e5f-ecd114fa371a</ProjectGuid>
+ <RootNamespace>OctokitBidon</RootNamespace>
@ryangribble
ryangribble Dec 22, 2016 Collaborator

what is "OctokitBidon" ?

@mderriey
mderriey Dec 23, 2016 Contributor

sorry, some hacky stuff I must have done at the beginning and forgot to remove them.

Octokit/Clients/MiscellaneousClient.cs
using System.Collections.ObjectModel;
-#endif
+
@ryangribble
ryangribble Dec 22, 2016 Collaborator

extra whitespace line (there are a few similar occurences on other files). this is definitely sraping the bottom of the barrel in terms of what I can pick up in review!

@mderriey
mderriey Dec 23, 2016 Contributor

🔥 them all!

Octokit/Http/ApiInfoParser.cs
@@ -8,7 +8,7 @@ namespace Octokit.Internal
internal static class ApiInfoParser
{
const RegexOptions regexOptions =
-#if NETFX_CORE
+#if !HAS_REGEX_COMPILED_OPTIONS
@ryangribble
ryangribble Dec 22, 2016 Collaborator

this block could be written the same as here for consistency

@mderriey
mderriey Dec 23, 2016 Contributor

done, thank you.

Octokit/Octokit.xproj
- <ProjectGuid>4052af53-4c1b-48d1-b873-bfd0351481c3</ProjectGuid>
- <RootNamespace>Octokit.Next</RootNamespace>
+ <ProjectGuid>104e8324-c2b9-43be-8040-36b736a64d45</ProjectGuid>
+ <RootNamespace>OctokitBidon</RootNamespace>
@ryangribble
ryangribble Dec 22, 2016 Collaborator

OctokitBidOn again

@ryangribble
Collaborator

In terms of the #defines

  • Some code is referencing HAS_REGEX_COMPILED_OPTIONS but this isnt defined for any project. Im thinking the net45 one should set this? There are also perhaps other #defines in a similar boat (eg code that uses them but no projects define them, or projects that define something which isnt actually used anywhere in code)

  • The HAS_TYPEINFO define is used to wrap most calls to GetTypeInfo() but there are some in the code base that are "unguarded". Is this a problem? If it's not a problem, then why do we need to guard any of them?

@ryangribble
Collaborator

This is really great stuff @mderriey I can't spot anything too serious and everything is locally working for me including builds in VS2015 and cake script, unit tests, integration tests etc.

I guess the next steps are to actually prove things are working (eg a running dotnet core app on on mac/linux/windows using octokit.core library) as well as a traditional full framework dotnet app on windows using the newly built library. We can also work on our nuget packages (AppVeyor should be spitting them out) to see if they are working etc...

The last piece of the puzzle was source linking in the PDB files but I dont think either of the 2 options mentioned (SourceLink and GitLink) will work for us at this time. I found another fork/derivative recently that Im going to look at PdbGit

@mderriey
Contributor

Thanks for the review @ryangribble, much appreciated. I pushed some changes according to your comments.

You were right for the HAS_REGEX_COMPILED_OPTIONS, it was meant for net45 but I forgot it.

I made some changes to the reflection code based on a trick @Haacked used, which is to define the GetTypeInfo method for platforms that don't have it. This way, you can use GetTypeInfo consistently in your code without having to guard for it. A few other extensions methods were created to create missing APIs in netstandard1.1 that exist in net45, like GetProperty or GetMember.

@mderriey
Contributor
mderriey commented Jan 6, 2017

@ryangribble regarding making sure everything works, do you want to publish a pre-release package on NuGet and asking people who are keen to try to update and make sure their apps still work? Or were you leaning towards us making sure it looks OK?

build.fsx
+ [ "./Octokit.Tests"
+ "./Octokit.Tests.Conventions" ]
+ |> Seq.iter (fun d ->
+ Fake.DotNetCli.Test (fun p ->
@shiftkey
shiftkey Jan 9, 2017 Member

There seems to be a signature issue with this function:

Error:

        build.fsx(138,9): error FS0001: This expression was expected to have type
            unit
        but here has type
            'a -> unit

Throwing a |> ignore on the end seems to do the trick :trollface:

Fake.DotNetCli.Test (fun p ->
    { p with
        WorkingDir = d }) |> ignore
@ryangribble
ryangribble Jan 9, 2017 Collaborator

if we are going to CAKE from fake anyway this F# voodoo may not be required anymore :)

@shiftkey
shiftkey Jan 9, 2017 Member

Fair point - I just pulled it down to do a .\build but I guess that's now handled by something else...

@mderriey
mderriey Jan 9, 2017 Contributor

I'm happy to remove the FAKE file and edit the build.cmd and build.sh to mimic what happens on AppVeyor. Sorry about the confusion.

mderriey added some commits Jan 10, 2017
@mderriey mderriey Merge remote-tracking branch 'gh/master' into port-to-dotnet-core
# Conflicts:
#	Octokit/Octokit-Mono.csproj
#	Octokit/Octokit-MonoAndroid.csproj
#	Octokit/Octokit-Monotouch.csproj
#	Octokit/Octokit-Portable.csproj
#	Octokit/Octokit-netcore45.csproj
#	Octokit/Octokit.csproj
81862a2
@mderriey mderriey remove FAKE build file and align build.cmd and build.sh
1bf7d56
@mderriey
Contributor
mderriey commented Jan 10, 2017 edited

My bash-fu is not great, I'll have a look at what happens later.

You can now use .\build.cmd and it will invoke CAKE. Parameters can be specified with the --name=value syntax.

Here are the parameters I found in the build project and their default values:

Parameter name Default value Other possible values
target Default Dotnet-Unit-Tests, Dotnet-Integration-Tests
configuration Release Debug
forcepublish false true
mderriey added some commits Jan 10, 2017
@mderriey mderriey add quotes around arguments in build.sh
b4c1cd4
@mderriey mderriey use forward slashes in build.sh
and fix a typo...
4d9e808
@mderriey mderriey Merge remote-tracking branch 'gh/master' into port-to-dotnet-core
# Conflicts:
#	Octokit.Tests/Octokit.Tests.csproj
#	Octokit/Octokit-Mono.csproj
#	Octokit/Octokit-MonoAndroid.csproj
#	Octokit/Octokit-Monotouch.csproj
#	Octokit/Octokit-Portable.csproj
#	Octokit/Octokit-netcore45.csproj
#	Octokit/Octokit.csproj
6aae297
@mderriey mderriey move summary to packOptions for NuGet packages and remove from test p…
…rojects
9a1568d
@mderriey
Contributor

@shiftkey / @ryangribble I updated the build files so they use the CAKE build.

How do you think you'll integrate PdbGit in the process? It looks to me that we need to hook this into the CAKE pipeline since NuGet packages are created as part of it. A very quick search leads me to think that we can't just execute an .exe from CAKE, we'd need to create some code that wraps that execution. Is this correct?

Cheers!

mderriey added some commits Jan 12, 2017
@mderriey mderriey hack together a CAKE tool to run PdbGit against pdb files
c66fd1d
@mderriey mderriey remove Mono from Travis as we do not need it anymore and it slows the…
… build quite a bit
0d3c1d6
@mderriey
Contributor

Alright, I took a shot at integrating PdbGit in the CAKE pipeline. It was quickly put together and possibly not very robust, but it works on both my local and AppVeyor.

@ryangribble
Collaborator

Wow that's awesome! Have you actually tried source debugging the built dll/pdb?

@mderriey
Contributor
mderriey commented Jan 12, 2017 edited

Edit:

I tried with the symbols NuGet package artifacts of the latest AppVeyor build. I can see VS trying to load a file from GitHub. It doesn't work though as it's trying to get the file from the octokit.net organisation repository, which is the first issue. The second thing is what I suppose is the commit SHA it's linking to, and I can't find it in either octokit.net/octokit#master not mderriey/octokit.net#port-to-dotnet-core.

Again, this is all very new to me so maybe I'm making false assumptions.


I haven't 😖. The instructions look pretty simple but every time I tried it in the past I was never able to make it work...

Sorry about that. Do you think you could give it a go? If not I'll try my luck once again 👍

As an aside, I'm very new to source linking, so please forgive my ignorance. Could you tell me why neither SourceLink nor GitLink would work?

@ryangribble
Collaborator

Im also not very across this stuff either by the way. My basic understanding is that source linking allows stepping into the source of a nuget loaded dll without requiring symbol servers etc. It somehow embeds the links of the files on github into the PDB file.

SourceLink and GitLink both scraped csproj files and so on, so they werent compatible with dotnet core (due to project.json) but then even with the "new" csproj for dotnet core now, I suspect they will still need to be changed since the csproj doesnt actually contain all files anymore and just has globs etc. PdbGit was another one I found which seemed to take a different approach and you pointed at the actual PDB files rather than the source project file, although I have no idea if that means it would work with dotnet core PDB files or not etc... It's pretty much stuff that needs to be researched... and in the meantime I believe one or both of SourceLink/GitLink are making moves to support dotnet core now that the new project structure is here.

In terms of PdbGit it probably makes sense that an AppVeyor build from a PR in this repo wouldnt know to reference files/commits from the contributors fork/branch and is instead trying to hit the official repo... although it's weird that the commit hash doesnt seem to exist anywhere :grining:

I guess we can always look at the PdbGit source code or reach out to the author to answer some of these questions. It also could be possible to pass parameters to PdbGit for the SHA and repository to use (but then Im not sure if AppVeyor even knows those details).

It's not really important to build source linked PDB files from pending PR's though anyway... we really just want the "official" builds from the octokit repo to have that capability. What I'd like to do when we cut over to this dotnet core and CAKE stuff, is have every build from master branch publish a pre-release package to nuget.org, and then periodically do the "official" releases like we do now.

@mderriey
Contributor
mderriey commented Jan 13, 2017 edited

Alright, after cloning the repo like AppVeyor does, posh-git shows me that 326116e commit SHA that PdbGit uses to link sources. I don't know where it comes from, though.

So what I did is a fresh clone of the repository that lives under my account, built it, created a console application pointing to the newly built assembly with the modified pdb next to it, and gave it a go.

It partially works, in the sense that I can F11 into the code from the Octokit assembly, but it's off by a line in the file that I tried. Trying to set a breakpoint on a specific line of code would have VS show a popup saying The following breakpoint cannot be set [...] The breakpoint failed to bind.


class Program
{
    static void Main(string[] args)
    {
        var task = DoWork();
        task.GetAwaiter().GetResult();
    }

    static async Task DoWork()
    {
        var client = new Octokit.GitHubClient(new Octokit.ProductHeaderValue("OctokitSourceLinking"));
        var repo = await client.Repository.Get("mderriey", "octokit.net");
    }
}
@mderriey mderriey try to determine better where the code comes from when running PdbGit
308ae45
@mderriey
Contributor

Unfortunately this doesn't work. As far as AppVeyor is concerned, the repo comes from octokit.net/octokit. I know it's not a big deal but I wanted to see if I could get this working.

Now here's a crazy: how about we publish a prerelease package of Octokit targeting netstandard1.1 and use it in this PR to get the correct repo and commit information since AppVeyor gives us the PR number? I know it's a very small test and won't cover much, though.

What are your thoughts?

@ryangribble
Collaborator

Yep I think we can publish a prerelease alpha package off this branch (without source linking in the PDB) so we can play around with it further in test projects and get something in the hands of our users so they can kick the tyres too.

If you also want to play with writing a CAKE helper that uses that octokit dotnetcore library to find out the correct URL to pass to PdbGit so source linking of pending PR builds works from the contributors source branch that's cool from a "nailed it!" perspective... although not "necessary" if you dont want to, since I dont think we would ever publish packages from a pending PR.

I do plan to step up the release frequency this year, including automatic publishing by the CAKE build script of every merged PR to master as a prerelease package to nuget, but I dont think packages from inflight PR's would be a thing wanted or needed. Still, for "science" it could be cool to see if you can get it working 😛

mderriey and others added some commits Jan 19, 2017
@mderriey mderriey Merge remote-tracking branch 'gh/master' into port-to-dotnet-core
# Conflicts:
#	Octokit.Tests.Conventions/ClientConstructorTests.cs
8678259
@ryangribble ryangribble get project.json formatting consistent with UpdateVersionInfo step 00520d9
@ryangribble ryangribble Remove integration tests from the automated pipeline, these need to b…
…e run manually as there are so many of them it triggers ratelimit/abuse thresholds
b4624a0
@ryangribble ryangribble remove custom task names from build targets as they arent just displa…
…y names they also affect cmdline usage
8ecc6b2
@ryangribble ryangribble Add log message to UpdateVersionInfo build task
6ac028b
@mderriey mderriey add forgotten embedded resources
this caused a fe wintegration tests to fail
99d9219
@mderriey mderriey change implementation of IsTypeDictionary in SimpleJson
9033e8b
@ryangribble ryangribble fix integration test - setup in ctor wasnt waiting for completion
81da4e4
@ryangribble
Collaborator

@mderriey and I have made a few more tweaks and all the integration tests are now passing.

I'm pleased to announce we now have Pre-Release packages up on NuGet!

https://www.nuget.org/packages/Octokit/0.24.1-alpha0001
https://www.nuget.org/packages/Octokit.Reactive/0.24.1-alpha0001

@ryangribble ryangribble added this to the dotnetcore milestone Jan 21, 2017
@ryangribble ryangribble added the feature label Jan 21, 2017
@ryangribble ryangribble changed the base branch to octokit:dotnetcore from octokit:master Jan 21, 2017
@ryangribble
Collaborator

Im going to merge this PR into the dotnetcore branch I just created.

We can then correct any further issues via specific targeted PR's for better tracking.

Ive also created a Milestone to track everything related to dotnetcore

@ryangribble ryangribble changed the title from [WIP] Port to .NET Core to Port to .NET Core Jan 21, 2017
@ryangribble ryangribble merged commit 13d5dab into octokit:dotnetcore Jan 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryangribble
Collaborator

@mderriey - you sir are a LEGEND 👍

@shiftkey
Member

Congrats to the both of you on getting this preview out! Keep up the awesome work 💖!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment