Provide .NET 4.6.1 framework target #1606

devlead · 2018-09-09T17:21:09Z

As discuessed in #1605

Adds .NET 4.6.1 framework target
adds Pollyfill for System.Runtime.InteropServices.RuntimeInformation
fixes Consider providing a net461 moniker in nuget package #1605

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

devlead · 2018-09-09T17:41:10Z

It would seem MSBuild property libgit2_hash isn't populated when I change from <TargetFramework>netstandard2.0</TargetFramework> to <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>. Will see if I can see where it's set.

jrgcubano · 2018-09-09T18:36:46Z

@devlead wooo 👍 .. It would be wonderful if you could get this... I'm trying to migrate https://github.com/YellowLineParking/Appy.GitDb and we want to keep both frameworks too.

devlead · 2018-09-10T14:54:21Z

Ok checking MSBuild log seems LibGit2Sharp.NativeBinaries.props isn't imported so that explains why libgit2_hash isn't populated could be related to props in build and not buildMultiTargeting will see if I can work around that.

bording · 2018-09-10T14:56:13Z

@devlead Yeah, since this PR adds multi-targeting, that will be an issue. We'll have to update LibGit2Sharp.NativeBinaries first before we can move forward with this.

devlead · 2018-09-10T19:03:19Z

@bording tested with package generated from libgit2/libgit2sharp.nativebinaries#75

https://ci.appveyor.com/project/devlead/libgit2sharp-nativebinaries/build/1.0.1

And then I could successfully build a nuget package.

ethomson · 2018-09-13T15:03:01Z

Neat! Once the NativeBinaries changes are ready to go, we'll merge this. Thanks!

bording · 2018-09-13T15:28:34Z

@ethomson Just an FYI I do have some review comments to make before we merge this, but I was waiting for the NativeBinaries update.

devlead · 2018-09-15T11:36:01Z

@bording I have now updated libgit2/libgit2sharp.nativebinaries#75 to import and not copy.
I could add a temporary nuget.config pointing to my MyGet feed if you would like this to build on CI and review before native binaries are shipped, and I can revert and rebase when they have.

bording · 2018-09-17T21:47:10Z

libgit2/libgit2sharp.nativebinaries#75 has been released as LibGit2Sharp.NativeBinaries 1.0.233, so you can update this PR to use it.

devlead · 2018-09-17T21:59:25Z

@bording native binaries updated and PR rebased.

devlead · 2018-09-17T22:06:31Z

2018-09-17T22:02:17.4868779Z ##[section]Async Command Start: Update Build Number
2018-09-17T22:02:17.4892551Z ##[section]Async Command End: Update Build Number

Azure Pipelines currently doesn't allow setting build numbers on PRs, whish thet did, but that's why builds are failing. But Mac / Linux Test Run Successful. Windows still running.

bording · 2018-09-17T23:03:53Z

@bording native binaries updated and PR rebased.

Cool, I should be able to look over this tomorrow then.

bording · 2018-09-18T21:49:30Z

Directory.Build.props

@@ -7,4 +7,8 @@
    <DefineConstants Condition=" '$(ExtraDefine)' != '' ">$(DefineConstants);$(ExtraDefine)</DefineConstants>
  </PropertyGroup>

+  <PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
+    <DefineConstants>$(DefineConstants);NETCORE</DefineConstants>


Instead of defining a new constant, let's use the built-in ones: NETCOREAPP, NETSTANDARD, and NETFRAMEWORK

bording · 2018-09-18T21:56:17Z

LibGit2Sharp/Core/Platform.cs

@@ -13,11 +13,16 @@ internal enum OperatingSystemType
    internal static class Platform
    {
        public static string ProcessorArchitecture => IntPtr.Size == 8 ? "x64" : "x86";
+#if !NETCORE
+        private static bool? _isRunningOnMac;
+        public static bool IsRunningOnMac() => _isRunningOnMac ?? (_isRunningOnMac = TryGetIsRunningOnMac()) ?? false;


Not sure we need this to be public.

Internal class, but sure made it private.

bording · 2018-09-18T22:01:42Z

LibGit2Sharp/Core/Platform.cs

+
+#if !NETCORE
+#pragma warning disable IDE1006 // Naming Styles
+        [DllImport("libc")]


Is there a danger here of this throwing? For example, what if we're running on a musl libc instead of glibc?

Unlikely to find a system without libc (even if it’s not glibc) but this is not ideal. uname takes a struct of char arrays (of a well defined size) not a single char array. So this should work everywhere (since it’s an appropriately large buffer) but I don’t love it.

I don’t think that libgit2 has a mechanism to tell you what OS you’re running on, but it seems like we could.

But without that, I think I would recommend sysctl instead of uname.

Is there a danger here of this throwing? For example, what if we're running on a musl libc instead of glibc?

libc should not be an issue.

I don’t think that libgit2 has a mechanism to tell you what OS you’re running on, but it seems like we could.

As this is used to decide how to load libgit, I don't see how we could use it, right?

But without that, I think I would recommend sysctl instead of uname.

Used uname trick for years in Mono projects, so didn't think of sysctl will investigate.

Haven't looked at sysctl implementation yet, but while seeking inspiration it does seem many use uname for os detection and then sysctl for kernel revision detection, wonder if there's some reasoning around that.
https://github.com/dotnet/corefx/blob/0566028e42a8f2fa86b8e0dd856d1f13d0fdce05/src/Native/Unix/System.Native/pal_runtimeinformation.c#L25-L39

Could be a legacy from the project "k" days, which I now see also used uname
https://github.com/aspnet/PlatformAbstractions/blob/174f51f3180ac9718de48d155b819e8a05310ab0/src/Microsoft.Extensions.PlatformAbstractions/Native/NativeMethods.Unix.cs#L10-L27

Maybe sysctl kern.ostype is good enough will give it a go and refactor PR.

Refactored PR to use sysctl and kern.ostype.

devlead · 2018-09-19T10:14:37Z

@bording @ethomson think I've addressed all feedback and PR ready for review again.

bording · 2018-09-19T22:00:58Z

LibGit2Sharp/Core/Platform.cs

@@ -13,11 +13,16 @@ internal enum OperatingSystemType
    internal static class Platform
    {
        public static string ProcessorArchitecture => IntPtr.Size == 8 ? "x64" : "x86";
+#if !NETSTANDARD


I want to change this eventually, but Platform.cs is also included in the Tests project, so using NETSTANDARD here means the test project has these members defined when they don't need to be.

To make this work in both places, basing the logic on NETFRAMEWORK is going to be the way to go.

* adds Pollyfil for System.Runtime.InteropServices.RuntimeInformation * fixes libgit2#1605

jrgcubano · 2018-09-22T16:13:59Z

@bording ready for next release? 🙄

devlead · 2018-09-25T17:22:47Z

@jrgcubano well @bording seems happy ;)
@ethomson had some feedback too, but think I've addressed that.

jrgcubano · 2018-09-27T12:25:23Z

@bording @ethomson Some news? -> Thanks a lot

ethomson · 2018-10-04T17:43:18Z

Thanks @devlead for doing this, and thanks @bording for the review.

devlead · 2018-10-04T18:46:39Z

@bording @ethomson splendid 👍

AArnott · 2018-10-16T19:15:16Z

Can we get a new release to nuget.org with this fix please?

devlead · 2018-10-17T15:09:24Z

Interesting the GH release 0.25.3 has this PR in it
v0.25.3...master

But the NuGet release seems to have been done from
https://github.com/libgit2/libgit2sharp/commits/ethomson/release_0_25_3

Which doesn't have this pr in it, so currently the release doesn't match the release binaries 🤔

But perhaps that was unintentional and this will come in 0.26.0?

AArnott · 2018-10-17T15:23:05Z

@devlead:

Interesting the GH release 0.25.3 has this PR in it v0.25.3...master

That link shows you what isn't in 0.25.3.

The last release was 0.26.0-preview-0027, built from this commit, which does not include this PR.

devlead · 2018-10-17T15:28:32Z

Ok I just clicked commits on the release
https://github.com/libgit2/libgit2sharp/releases/tag/v0.25.3

AArnott · 2018-10-17T15:30:08Z

That's a misleading link, I guess. But look carefully at the wording of that link:

63 commits to master since this release

devlead · 2018-10-17T15:35:05Z

Ah sorry you're correct.

ethomson · 2018-10-19T12:15:59Z

Can we get a new release to nuget.org with this fix please?

@AArnott: you bet - https://www.nuget.org/packages/LibGit2Sharp/0.26.0-preview-0054

Provide .NET 4.6.1 framework target

devlead mentioned this pull request Sep 9, 2018

Consider providing a net461 moniker in nuget package #1605

Closed

devlead commented Sep 9, 2018

View reviewed changes

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj Show resolved Hide resolved

devlead mentioned this pull request Sep 10, 2018

Added BuildMultiTargeting props libgit2/libgit2sharp.nativebinaries#75

Merged

devlead force-pushed the feature/net461nodep branch from 450a223 to 4583888 Compare September 17, 2018 21:58

bording requested changes Sep 18, 2018

View reviewed changes

devlead force-pushed the feature/net461nodep branch 2 times, most recently from 9a2f0b5 to 9fce228 Compare September 19, 2018 10:12

devlead force-pushed the feature/net461nodep branch from 9fce228 to 7da0e1e Compare September 19, 2018 10:54

bording requested changes Sep 19, 2018

View reviewed changes

Added .NET 4.6.1 target

d6f6322

* adds Pollyfil for System.Runtime.InteropServices.RuntimeInformation * fixes libgit2#1605

devlead force-pushed the feature/net461nodep branch from 7da0e1e to d6f6322 Compare September 20, 2018 04:42

bording approved these changes Sep 20, 2018

View reviewed changes

ap0llo mentioned this pull request Sep 27, 2018

GetBuildVersion task fails for full Framework builds dotnet/Nerdbank.GitVersioning#228

Closed

ethomson merged commit 57b6079 into libgit2:master Oct 4, 2018

jcansdale mentioned this pull request Dec 17, 2018

Pull Request inline comment appears on GitHub.com but not in extension github/VisualStudio#1469

Closed

alex-weaver pushed a commit to alex-weaver/libgit2sharp that referenced this pull request Mar 23, 2023

Merge pull request libgit2#1606 from devlead/feature/net461nodep

587aee7

Provide .NET 4.6.1 framework target

Provide .NET 4.6.1 framework target #1606

Provide .NET 4.6.1 framework target #1606

Conversation

devlead commented Sep 9, 2018 • edited Loading

devlead commented Sep 9, 2018

jrgcubano commented Sep 9, 2018 • edited Loading

devlead commented Sep 10, 2018 • edited Loading

bording commented Sep 10, 2018

devlead commented Sep 10, 2018

ethomson commented Sep 13, 2018

bording commented Sep 13, 2018

devlead commented Sep 15, 2018

bording commented Sep 17, 2018

devlead commented Sep 17, 2018

devlead commented Sep 17, 2018

bording commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devlead commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrgcubano commented Sep 22, 2018

devlead commented Sep 25, 2018

jrgcubano commented Sep 27, 2018

ethomson commented Oct 4, 2018

devlead commented Oct 4, 2018

AArnott commented Oct 16, 2018

devlead commented Oct 17, 2018

AArnott commented Oct 17, 2018

devlead commented Oct 17, 2018

AArnott commented Oct 17, 2018

devlead commented Oct 17, 2018

ethomson commented Oct 19, 2018

devlead commented Sep 9, 2018 •

edited

Loading

jrgcubano commented Sep 9, 2018 •

edited

Loading

devlead commented Sep 10, 2018 •

edited

Loading