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.

Sign up for GitHub

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

Jump to bottom

[v3.0] Add support for ARM64 variants of Debian, Ubuntu #407

Merged
merged 12 commits into from
Nov 20, 2019
Merged

[v3.0] Add support for ARM64 variants of Debian, Ubuntu #407

merged 12 commits into from
Nov 20, 2019

Conversation

qmfrederik
Copy link
Contributor

@qmfrederik qmfrederik commented Nov 17, 2019 •
edited
Loading

  • Bump Microsoft.SourceLink.GitHub to the most recent version
  • Add CI leg for .NET Core 3.0, on Ubuntu Bionic and Ubuntu Disco
  • Add RID mapping for x64 Ubuntu 19.04, 19.10. This fixes loading of libgit2.so on these platforms.
  • Add RID mapping for arm64 Debian and ARM. This enables loading of libgit2.so on these platforms.
  • Update the nbgv tool to also use GitLoaderContext. This fixes loading of libgit2.so when invoking nbgv on the platforms listed above.

@qmfrederik
Copy link
Contributor Author

Odd, I've now seen two tests fail with UserNotConfigured. Not quite sure why.

@qmfrederik
Copy link
Contributor Author

  • Most CI jobs are failing because LD_LIBRARY=all is set, which is causing output to stderr. They are not actually failing, though
  • nbgv is failing on ubuntu.19.04-x64.
    • libgit2sharp should consume libgit2.so built for ubuntu.18.04-x64
    • As far I can tell, linux-x64 is being loaded (the clue is the probe for libssl1.0 instead of libssl1.1).
    • Looking at the RID graph, it seems ubuntu.19.04-x64 imports ubuntu.18.10 but not ubuntu.18.10-x64. That might explain this.

Let's see if there's a way to make this work.

@qmfrederik qmfrederik changed the title [WIP] [v3.0] Add support for ARM64 variants of Debian, Ubuntu [v3.0] Add support for ARM64 variants of Debian, Ubuntu Nov 17, 2019
@qmfrederik
Copy link
Contributor Author

@AArnott Allright, finally got this working. In the end, it boils down to

  • Updating the RuntimeIdMap to account for new RIDs
  • Using the GitLoaderContext in nbgv, too.

CI for Ubuntu Disco is now green!

Let me know if you think this can be merged, looking forward to (finally!) using nbvg on my Raspberry Pi 4 😄 .

AArnott
AArnott reviewed Nov 20, 2019
View reviewed changes
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for getting this working!
A few questions, possibly with follow-up changes depending on your answers, and we'll get this merged.

azure-pipelines/xplattest-pipeline.yml Show resolved Hide resolved
src/NerdBank.GitVersioning.NativeBinaries/NerdBank.GitVersioning.NativeBinaries.csproj Outdated
<!-- Download version 2.0.280-armpreview01 of LibGit2Sharp.NativeBinaries. This builds from the same libgit2 commit (git2-572e4d8) as
0.27.0-preview-0007, but includes arm64 binaries.
This explicit dependency should be removed when upgrading to a newer version of LibGit2Sharp. -->
<PackageReference Include="LibGit2Sharp.NativeBinaries" Version="2.0.280-armpreview01" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this reference the entire reason for this project?
If so, can we simply move it into an existing project (and set PrivateAssets="all" on it) or use the PackageDownload item?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know about the PackageDownload item. Looks much cleaner now. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the downside, PackageDownload works start with .NET SDK 2.2 which somehow broke the layout of the nbgv.cli folder used by the npm package. I replaced the copy mechanism with the output of dotnet publish, that seems to do the trick.

src/Nerdbank.GitVersioning.Tasks/GitLoaderContext.cs Outdated
@@ -15,6 +15,11 @@ internal class GitLoaderContext : AssemblyLoadContext
{
public static readonly GitLoaderContext Instance = new GitLoaderContext();

// When invoked as a MSBuild task, the native libraries will be at
// ../runtimes. When invoked from the nbgv CLI, the libraries
// will be at ./runtimes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we beef up this comment to explain where this property gets set to ./runtimes? I finally found it elsewhere in this PR, but just looking at the comment it is not at all clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I updated the comments.

@qmfrederik
Copy link
Contributor Author

@AArnott. I think I adressed your comments. Let me know if there's anything else you need!

AArnott
AArnott approved these changes Nov 20, 2019
View reviewed changes
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. I made a couple minor changes and will merge.

@AArnott AArnott merged commit cce1afd into dotnet:v3.0 Nov 20, 2019
@qmfrederik qmfrederik deleted the features/arm64-v3 branch November 20, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AArnott AArnott AArnott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@qmfrederik @AArnott