Introduce cross-platform compatibility and pipeline #205

swernli · 2021-01-20T00:14:12Z

This change introduces infrastructure for cross-platform build of native components in LibLlvm, allowing the eventual .NET core package to build in all the binaries and enable execution on Linux and MacOS. This also updates the GitHub Actions workflow as a first attempt to support the full cross-platform build and test.

This addresses #193

…st passes

…pgraded to CppSharp 0.10.2

… of the build

This is a big change because it includes cached binaries and headers from the release versions of Mac and Linux LLVM along with the cached build output for Windows. The default behavior of the pipeline is now to use the cached binaries, resulting in about a 20 min build time, while the full 3 hour build of LLVM is still an option. Lots of clean-up on the scripts is still left to do, as well as creating a multistage pipeline that can actually produce and test the fully formed nuget package, but this is a big chunk of the way there.

Enable multi-stage pipeline with skipped tests.

swernli · 2021-01-20T23:24:04Z

Converting this PR to draft in favor of breaking it up into smaller chunks to make reviewing easier.

alan-geller

Wow, this is awesome!!!

alan-geller · 2021-01-21T00:03:16Z

src/Interop/LlvmBindingsGenerator/Templates/T4/ExportsTemplate.cs

@@ -18,7 +18,7 @@ namespace LlvmBindingsGenerator.Templates
    /// Class to produce the template output
    /// </summary>

-    #line 1 "D:\GitHub\Ubiquity.NET\Ubiquity.NET.Llvm\src\Interop\LlvmBindingsGenerator\Templates\T4\ExportsTemplate.tt"
+    #line 1 "D:\github\Llvm.NET\src\Interop\LlvmBindingsGenerator\Templates\T4\ExportsTemplate.tt"


Is this change to match the new build scripts?

This file is generated by T4 (https://docs.microsoft.com/en-us/visualstudio/modeling/code-generation-and-t4-text-templates) and inherits line numbers from the system where they were generated. At some point it might be worth adding these files to the gitignore and then incorporating T4 generation into the build instead of relying on a developer opening the project in VS to regenerate them, but I didn’t find an easy way to do that, so deferred to the existing process.

alan-geller · 2021-01-21T00:04:55Z

src/Interop/Ubiquity.NET.Llvm.Interop/NativeMethods.cs

@@ -19,61 +19,6 @@ namespace Ubiquity.NET.Llvm.Interop
    /// <summary>Interop methods for the Ubiquity.NET LibLLVM library</summary>
    public static partial class NativeMethods


Is there a reason not to delete this file? It looks empty...

The static string in there is needed by the infrastructure that uses the dll name, but I can put that into another file to avoid the seemingly empty file.

Yeah, if that const string is all that is left, it can move easy enough

smaillet · 2021-01-23T20:07:26Z

Build-All.ps1

@@ -6,11 +6,11 @@ Param(
    [System.String]$BuildMode = 'All'
 )

-pushd $PSScriptRoot
+Push-Location $PSScriptRoot


please retain the original alias forms, they are standard powershell commands

smaillet · 2021-01-23T20:08:19Z

Build-All.ps1

@@ -26,19 +26,31 @@ try
    if((Test-Path -PathType Container $buildInfo['BuildOutputPath']) -and $ForceClean )
    {
        Write-Information "Cleaning output folder from previous builds"
-        rd -Recurse -Force -Path $buildInfo['BuildOutputPath']
+        Remove-Item -Recurse -Force -Path $buildInfo['BuildOutputPath']


Please use the alias names, the style for this project prefers the alias names.

smaillet · 2021-01-23T20:09:14Z

Build-All.ps1

-    md $buildInfo['NuGetOutputPath'] -ErrorAction SilentlyContinue | Out-Null
+    $includePath = (Join-Path $PSScriptRoot llvm include)
+    $libPath = (Join-Path $PSScriptRoot llvm lib)
+    if ((Test-Path -PathType Container $includePath) -and $ForceClean) {


Code style for this project places braces on a new line for all languages

smaillet · 2021-01-23T20:11:10Z

Build-DotNet.ps1

@@ -0,0 +1,31 @@
+Param(


Why is this script needed? It's mostly overhead compared to what it does. This functionality is better suited inlined as it was.

smaillet · 2021-01-23T20:11:48Z

Build-Interop.ps1

        pushd (Join-Path $buildInfo.BuildOutputPath 'bin\PatchVsForLibClang\Release\netcoreapp3.1')
        try
        {
            Write-Information "Patching VS CRT for parsing with LibClang..."
            .\PatchVsForLibClang.exe $vs.InstallationPath
+            if ($LASTEXITCODE -ne 0) {


Style, please follow style of this repo regarding braces on a new line

smaillet · 2021-01-24T01:50:40Z

src/Interop/LibLLVM/LibLLVM.vcxproj

@@ -157,7 +157,6 @@
    <ClInclude Include="targetver.h" />


Why change this if CMAKE is used to build the DLL, shouldn't this just be deleted?

smaillet · 2021-01-24T01:52:07Z

src/Interop/LibLLVM/PassManagerBindings.cpp

@@ -18,6 +18,7 @@
 #include <llvm/IR/Module.h>
 #include <llvm/Transforms/Instrumentation.h>
 #include <llvm/PassRegistry.h>
+#include <llvm/LinkAllPasses.h>


Why was this added? It wasn't needed to build the code before this change...

smaillet · 2021-01-24T01:52:22Z

src/Interop/LibLLVM/TripleBindings.cpp

@@ -2,6 +2,7 @@
 #include <llvm/ADT/Triple.h>
 #include <llvm/Support/CBindingWrapping.h>
 #include <llvm/Support/TargetParser.h>
+#include <llvm/Config/llvm-config.h>


Why is this added?

smaillet · 2021-01-24T01:53:42Z

src/Interop/LlvmBindingsGenerator/LlvmBindingsGenerator.csproj

@@ -11,8 +11,8 @@
        Due to current limitations of the CppSharp library NUGET package, this only supports the Windows Desktop .NET Framework (64 bit)
        This means that this project can't use language features > 7.3 (Limits for .NET Framework)
        -->
-        <TargetFramework>net47</TargetFramework>
-        <Nullable />
+        <TargetFramework>net48</TargetFramework>


Why change the TFM for this? It can't run on anything other than windows .NET framework anyway so why make this change?

smaillet · 2021-01-24T02:04:23Z

src/Ubiquity.NET.Llvm.Tests/TestObjectFile.cs

@@ -72,13 +72,15 @@ public static void Initialize( TestContext ctx )
            tm.EmitToFile( module, TestObjFileName, CodeGenFileType.ObjectFile );
        }

+        [TestCategory("ObjFiles")]


If all of these tests need to be skipped on some target, then something is either fundamentally wrong with the tests, or the object file support. so a bug should be filed for this as disabling these all for a platform is cutting off a big chunk of functionality of the library.

swernli · 2021-01-24T22:39:52Z

@smaillet Thanks for all the feedback. I'll respond to a few key repeated points here, and then work on different PRs that focus on specific parts of the infrastructure to hopefully allow for more targeted discussion.

Regarding removal of unused powershell, I didn't realize the module here was a copy of one that you use in other places. Perhaps a readme in that folder that points this outl could help until you take the steps you've described of making this its own repo that is treated as a true common source of the code. As I restructure these PRs, I'll avoid making any changes to this module that are not strictly necessary and not try to do any cleanup based on what seems to be unused.
This "all-up" PR includes a lot of changes and infrastructure introduced to support building the full LLVM source on Azure DevOps which is not compatible with the current GitHub Actions pipeline. Rather than merge that in we can just refer back to some of the working branches where that was developed, so I won't include that in future PRs until we know we want to start pulling that in.
Some of the changes to scripts and how errors were reported was done to make the scripts compatible with Azure DevOps, where error reporting from powershell works slightly differently. Given that we want to rely on GitHub Action workflows instead, I will not be making those changes.
As you mentioned on one of the other PRs the caching of LLVM binaries into the repo directly is probably the most controversial element of the changes, so rather than start there I will focus on the build changes that can be made without affecting the binary download step. That should give us some time to brainstorm alternate approaches to solving that problem.
Unfortunately, powershell aliases do not have consistent behavior across platforms, so to get the scripts ready for running on Linux and Mac we will need to replace aliases with the full cmldets. The issue of powershell cmdlet aliases shadowing Linux built-in shell commands has been a long discussed one (see Figure out aliases PowerShell/PowerShell#929 for some of the debate around this), but the end result was that several aliases were removed from the Linux version in order to avoid shadowing. The end result is that using a given alias is not guaranteed to resolve to the same command on every platform, possibly leading to script errors (which it did with these scripts). The recommendation is to avoid using alias in scripts in favor of the full cmdlet in order to ensure the right functionality is invoked and have the highest chance of compatibility. This is also why most powershell editor extensions treat aliases as warnings.

I should have a fresh PR with just first incremental step available for review shortly, and to avoid confusion I will just submit one PR at a time instead of using drafts to imply readiness.

Alan Geller and others added 24 commits January 19, 2021 12:52

All files found, symbols still not matching

6224e5b

Compiling OK, RuntimeLibrary mismatch on build

fe6e591

Build is working on Windows using cmake

817c04c

First steps towards Linux/Mac build scripts

b5e0e7e

Updated native library load to work with .NET Core 3.1+

d6f3a8e

Windows CMAKE build is now creating proper symbol exports; interop te…

bce5a4e

…st passes

WIP Updated scripts, breaking them into more steps; not working

da93f31

WIP Cleaning up build scripts

1057c31

WIP last couple of changes before Thanksgiving

6fc6879

8000
Added LLVM as a submodule, builds LLVM on WIndows

400bd96

Added script to move the LLVM build output where it's expected, and u…

34d7659

…pgraded to CppSharp 0.10.2

Set up CI with Azure Pipelines (#1)

cdf6511

Added move of LLVM build output to the expected location for the rest…

2ed281b

… of the build

Updating CMake-Helpers.ps1 for xplat build support

e6ae6af

swernli/pipeline (#4)

4b55688

Enable multi-stage pipeline with skipped tests.

Set fetch depth to 0

8000

b089bdc

Remove explicit cached headers in favor of zip file

868b8db

Fix Build-Source

66241cc

Use force for header expansion

9a9caa8

Update for latest changes from development branch

f85cd75

Update to support GitHub actions

916425c

Rearrange build interop script

f4a4cd9

Try fixing build pipeline

697a364

swernli force-pushed the swernli/xplat-pipeline branch 2 times, most recently from 58b8ab5 to 697a364 Compare January 20, 2021 09:48

swernli added 4 commits January 20, 2021 09:08

Add more output when patching VS

70656f5

Try remove explicit shell selection for dotnet build

f2f0375

Propagate llvm includes through pipeline

64b3bc2

Skip creating llvm dir

09d5235

swernli added 9 commits January 20, 2021 10:50

Set tests into blame mode

43d67bd

Fix bindings config for inline functions

9346639

Add comment for test blame mode.

516d504

One more bindings fix

dd73e7c

Try running Object Files tests.

d7cc95b

Update release workflow to perform xplat build

484ee69

Programmatically skip bad Linux/MacOS tests

bbb2dec

Misc. clean up

f98667e

Fix typos

7d6f406

swernli mentioned this pull request Jan 20, 2021

[Feature] - Add support for cross-platform use #193

Open

swernli marked this pull request as draft January 20, 2021 23:23

alan-geller approved these changes Jan 21, 2021

View reviewed changes

swernli mentioned this pull request Jan 21, 2021

Cache LLVM binaries in repo instead of downloading (xplat stage 1) #206

Closed

smaillet requested changes Jan 24, 2021

View reviewed changes

swernli closed this Jan 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce cross-platform compatibility and pipeline #205

Introduce cross-platform compatibility and pipeline #205

		@@ -19,61 +19,6 @@ namespace Ubiquity.NET.Llvm.Interop
		/// <summary>Interop methods for the Ubiquity.NET LibLLVM library</summary>
		public static partial class NativeMethods

Introduce cross-platform compatibility and pipeline #205

Introduce cross-platform compatibility and pipeline #205

Conversation

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

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