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

Merge [v3.0] -> [master] #400

Merged
merged 15 commits into from
Nov 21, 2019
Merged

Merge [v3.0] -> [master] #400

merged 15 commits into from
Nov 21, 2019

Conversation

qmfrederik
Copy link
Contributor

@qmfrederik qmfrederik commented Oct 28, 2019

libgit2/libgit2sharp.nativebinaries#91 added arm64 support to libgit2sharp.nativebinaries.

This PR

  • Updates the RuntimeIdMap to add the RIDs for the arm64 variants Debian 9 and Ubuntu 16.04 through 19.10 (and x64 versions of Ubuntu 19.04 and 19.10).
  • Bumps SourceLink to a more recent version with arm64 support

I tested this locally on a Rasperry Pi 4 with Ubuntu 19.10, though it took some voodoo to get things to compile locally on arm64.

Fixes #392

@qmfrederik
Copy link
Contributor Author

Oh, this is odd:

/home/vsts_azpcontainer/.nuget/packages/nerdbank.gitversioning/3.1.28-beta-g82f7bc08c4/build/Nerdbank.GitVersioning.targets(63,5): error MSB4018: ---> System.IO.DirectoryNotFoundException: Could not find a part of the path '/home/vsts_azpcontainer/.nuget/packages/nerdbank.gitversioning/3.1.28-beta-g82f7bc08c4/build/MSBuildCore/runtimes'. [/__w/1/s/lib/lib.csproj]

The problem is that the path somehow resolves to ~/.nuget/packages/nerdbank.gitversioning/3.1.28-beta-g82f7bc08c4/build/MSBuildCore/runtimes instead of ~/.nuget/packages/nerdbank.gitversioning/3.1.28-beta-g82f7bc08c4/build/runtimes.

I assumed this was because I misconfigured something on my Raspberry Pi, but apparently it repros here as well. (I workaround this by just copying the folder).

Do you know why we'd end up with build/MSBuildCore/runtimes instead of build/runtimes? I don't think this PR broke this, but it would be worth fixing, anyway ;-)

@qmfrederik
Copy link
Contributor Author

I'm fairly sure that error is a regression introduced by libgit2/libgit2sharp#1714, it constructs a path to the directory which should contain the native libraries: https://github.com/libgit2/libgit2sharp/pull/1714/files#diff-64eda99ded0bc32ff7cc97deb440b2dbR155-R166 . I'm not sure that path is ever correct, though.

That said, it's in the fallback logic so this should be avoidable. Let me have a second look at this.

@AArnott
Copy link
Collaborator

AArnott commented Oct 28, 2019

Thanks, @qmfrederik.

The macOS failure repros on master too, so don't worry about that. It's due to https://github.com/microsoft/azure-pipelines-image-generation/issues/531 which I'm looking for a workaround for.

But yes, as your PR alleges to make Ubuntu 19 work, I'd like to see the Disco validation pass as part of this PR. Also I wonder where you got the updates to the RuntimeIdMap.cs file. Did you hand-author this? If you copied it from somewhere we should perhaps update the top line saying where we copied it from.

@AArnott
Copy link
Collaborator

AArnott commented Oct 28, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AArnott
Copy link
Collaborator

AArnott commented Oct 28, 2019

I've worked around the macOS issue in master.

@qmfrederik
Copy link
Contributor Author

@AArnott

I'll see if I can either submit a PR to LibGit2Sharp or find a workaround (or both).

@qmfrederik
Copy link
Contributor Author

This will only work once libgit2/libgit2sharp#1732 is merged (and NB.GV consumes a new version of LibGit2Sharp)

@AArnott
Copy link
Collaborator

AArnott commented Oct 30, 2019

I think you copied the original file from the SourceLink repository. The RutnimeIdMap.cs file no longer exists there, so I think they switched to a new algorithm alltogether.

Correct. The new algorithm is that they completely ditched libgit2(sharp) in favor of an all managed implementation that reads the .git folder's contents. That's a very expensive sounding idea and not one I care to replicate unless they want to share the all-managed git implementation as a library I can simply reship. But I suspect they are implementing just enough to support SourceLink, which is much less than what we need in this library. So IMO we should proceed with our current design of using libgit2sharp.

@qmfrederik
Copy link
Contributor Author

Sounds fair enough to me. Just to be clear, that means you're OK with manually updating the RuntimeIdMap.cs file for now?

@AArnott
Copy link
Collaborator

AArnott commented Oct 30, 2019

Yes, updating RuntimeIdMap.cs manually is fine as long as you know what you're doing. :) I guess we own it now.

@qmfrederik
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 400 in repo AArnott/Nerdbank.GitVersioning

@qmfrederik
Copy link
Contributor Author

@AArnott I ran some more tests (sorry for all the spam this has generated).

  • I started to think libgit2sharp is quite broken on .NET Core 3.0. To verify this assumption, I added a CI leg which uses .NET Core 3.0 on Ubuntu Bionic. Sure enough, CI passes on Bionic when using .NET Core 2.1, but fails when using .NET Core 3.0
  • I downgraded to a previous version of libgit2sharp (without the new NativeLibrary probing mechanism), and sure enough CI starts passing on 3.1 on Bionic & Disco. On Disco, there's another issue with nbgv which I haven't had the time to dig into yet.
  • Unfortunately, it looks like downgrading would bring back libgit2sharp leaves _git2_ junction directories behind #396

Now, for master, it seems we're stuck between a rock and a hard place - issues on .NET Core 3.0, or living with #396 .

The good news is that this seems fixable for version nbgv v3.0, as v3.0 is still on 0.27.0-preview-0007.

I'll create a separate PR for v3.0.

@qmfrederik qmfrederik changed the title Add support for ARM64 variants of Debian, Ubuntu [master] Add support for ARM64 variants of Debian, Ubuntu Nov 17, 2019
@AArnott
Copy link
Collaborator

AArnott commented Nov 20, 2019

Now, for master, it seems we're stuck between a rock and a hard place - issues on .NET Core 3.0, or living with #396 .

Or a third option: downgrading libgit2sharp even further to the same version we have in NBGV 3.0, where we had neither of these, but we also lacked worktree support (which is why I upgraded in the first place).
I was really happy to finally have worktree support, but not having ubuntu 19 support is more painful. And I think leaving directory junctions lying around is likely to cause upset.

Am I understanding you correctly, and do you agree we could downgrade enough to solve all this?
Of course, if the only significant feature in NBGV's master branch is in fact worktree support, maybe I'll just keep it as-is (and unreleased) until libgit2sharp offers us a better option.

@qmfrederik
Copy link
Contributor Author

@AArnott Yeah, downgrading will to the same version as v3.0 will get you a build that works on Ubuntu 19.xx .

There are two other options I can think of:

  • Fix native library loading on Linux libgit2/libgit2sharp#1732 gets merged (which is a more straightforward patch, and can perhaps be fast-tracked by the LibGit2Sharp team)
  • I think just creating an empty directory at {assemblyDirectory}/runtimes would be enough to work around the issue in LibGit2Sharp's NativeMethods.ResolveDll to allow native library resolution to fall back to the AssemblyLoadContext, allowing you to consume a recent version of LibGit2Sharp and have Ubuntu 19.xx support.

@qmfrederik
Copy link
Contributor Author

@AArnott So, this PR now basically is:

  • v3.0 -> master
  • Upgrade LibGit2Sharp

At least Ubuntu Bionic with .NET Core 3.0 is now working. That's progress!

Ubuntu Disco gives a segfault, which is very odd. Could you try to restart that leg of the job to see if it's persistent?

@AArnott
Copy link
Collaborator

AArnott commented Nov 20, 2019

@qmfrederik I restarted that job and it failed again.
If what we have is progress, I may not mind if we back out the failing part and complete a PR to lock in some of that goodness, and keep plugging away in a new PR to get the rest of it working. It's up to you. I really appreciate your work here.

BTW, the 18 commits that GitHub claims to be bringing in with this PR look like there's some duplication there. Where there cherry-picked commits that are now redundant with those being merged in? Can we perhaps do a fresh merge from v3.0 to master and then add the extra stuff in one more commit then force-push it to your source branch? That might make it less likely to hit merge conflicts like it just did as well.

@qmfrederik qmfrederik changed the title [master] Add support for ARM64 variants of Debian, Ubuntu Merge [v3.0] -> [master] Nov 21, 2019
@qmfrederik
Copy link
Contributor Author

@AArnott This PR now essentially is a merge from v3.0 into master. I did a git reset --hard upstream/master of the branch to master, and then a git merge upstream/v3.0.

I'd say we get this done first, otherwise we'll keep fighting merge conflicts every time something changes in master.

The Ubuntu Disco build leg is disabled for now, let's tackle one problem at a time (an admittedly, I spent more time than planned on this 😄 ).

@AArnott
Copy link
Collaborator

AArnott commented Nov 21, 2019

Thank you. Do you have time to investigate the remaining failure?

@qmfrederik
Copy link
Contributor Author

@AArnott Yup, this should be OK now

@AArnott AArnott added this to the v3.1 milestone Nov 21, 2019
@AArnott AArnott merged commit 2885cfd into dotnet:master Nov 21, 2019
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.

Builds broken on Ubuntu 19.04
2 participants