Framework multi-targeting improvements (#215)

microsoft · Mar 6, 2024 · def23a5 · def23a5
1 parent 52a1d43
commit def23a5
Show file tree

Hide file tree

Showing 25 changed files with 263 additions and 251 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -12,14 +12,27 @@ jobs:
     strategy:
       matrix:
         os: [ windows-latest, macos-latest, ubuntu-latest ]
+        dotnet-version: [ net472, net6.0, net8.0]
         node-version: [ 18.x, 20.x ]
         configuration: [ Release ]
+        exclude:
+          # Exclude Node 18.x on .NET < 8, to thin the matrix.
+          - dotnet-version: net6.0
+            node-version: 18.x
+          - dotnet-version: net472
+            node-version: 18.x
+          # Exclude .NET 4.x on non-Windows OS.
+          - os: macos-latest
+            dotnet-version: net472 
+          - os: ubuntu-latest
+            dotnet-version: net472
+
       fail-fast: false  # Don't cancel other jobs when one job fails
 
     runs-on: ${{ matrix.os }}
 
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
       with:
         fetch-depth: 0 # Deep clone is required for versioning on git commit height
 
@@ -28,23 +41,21 @@ jobs:
       run: sudo ln -s /lib/x86_64-linux-gnu/libdl.so.2 /lib/x86_64-linux-gnu/libdl.so
 
     - name: Setup .NET 6
-      uses: actions/setup-dotnet@v3
+      uses: actions/setup-dotnet@v4
       with:
         dotnet-version: 6.0.x
 
+    # The .NET 8 SDK is required even when the build matrix targets other .NET versions.
     - name: Setup .NET 8
-      uses: actions/setup-dotnet@v3
+      uses: actions/setup-dotnet@v4
       with:
         dotnet-version: 8.0.x
 
     - name: Setup Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v3
+      uses: actions/setup-node@v4
       with:
         node-version: ${{ matrix.node-version }}
 
-    - name: Build ${{ matrix.configuration }}
-      run: dotnet build --configuration ${{ matrix.configuration }}
-
     - name: Build packages
       id: pack
       run: dotnet pack --configuration ${{ matrix.configuration }}
@@ -56,48 +67,29 @@ jobs:
     #      limit-access-to-actor: true
 
     - name: Upload build artifacts
-      uses: actions/upload-artifact@v3
+      if: matrix.dotnet-version == 'net8.0' && matrix.node-version == '20.x'
+      uses: actions/upload-artifact@v4
       with:
         name: ${{ matrix.os }}-${{ matrix.configuration }}-packages
         path: |
           out/pkg/*.nupkg
           out/pkg/*.tgz
 
-    - name: Test .NET 4.7.2
-      if: matrix.os == 'windows-latest' && steps.pack.conclusion == 'success' && !cancelled()
-      env:
-        TRACE_NODE_API_HOST: 1
-      run: >
-        dotnet test -f net472
-        --configuration ${{ matrix.configuration }}
-        --logger trx
-        --results-directory "out/test/netfx47-node${{ matrix.node-version }}-${{ matrix.configuration }}"
-
-    - name: Test .NET 6
-      if: steps.pack.conclusion == 'success' && !cancelled()
-      env:
-        TRACE_NODE_API_HOST: 1
-      run: >
-        dotnet test -f net6.0
-        --configuration ${{ matrix.configuration }}
-        --logger trx
-        --results-directory "out/test/dotnet6-node${{ matrix.node-version }}-${{ matrix.configuration }}"
-
-    - name: Test .NET 8
+    - name: Test
       if: steps.pack.conclusion == 'success' && !cancelled()
       env:
         TRACE_NODE_API_HOST: 1
       run: >
-        dotnet test -f net8.0
+        dotnet test -f ${{ matrix.dotnet-version }}
         --configuration ${{ matrix.configuration }}
         --logger trx
-        --results-directory "out/test/dotnet8-node${{ matrix.node-version }}-${{ matrix.configuration }}"
+        --results-directory "out/test/${{matrix.dotnet-version}}-node${{matrix.node-version}}-${{matrix.configuration}}"
+      continue-on-error: true
 
     - name: Upload test logs
-      if: always() # Update artifacts regardless if code succeeded, failed, or cancelled
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4
       with:
-        name: test-logs-${{ matrix.os }}-node${{ matrix.node-version }}-${{ matrix.configuration }}
+        name: test-logs-${{ matrix.os }}-${{matrix.dotnet-version}}-node${{matrix.node-version}}-${{matrix.configuration}}
         path: |
           out/obj/${{ matrix.configuration }}/**/*.log
           out/obj/${{ matrix.configuration }}/**/*.rsp

diff --git a/.github/workflows/test-report.yml b/.github/workflows/test-report.yml
@@ -16,19 +16,30 @@ permissions:
 jobs:
   report:
     strategy:
-      matrix:
+      matrix: # This must be kept in sync with the PR build matrix.
         os: [ windows-latest, macos-latest, ubuntu-latest ]
-        node-version: [ 18.x ]
+        dotnet-version: [ net472, net6.0, net8.0]
+        node-version: [ 18.x, 20.x ]
         configuration: [ Release ]
-      fail-fast: false  # Don't cancel other jobs when one job fails
+        exclude:
+          # Exclude Node 18.x on .NET < 8, to thin the matrix.
+          - dotnet-version: net6.0
+            node-version: 18.x
+          - dotnet-version: net472
+            node-version: 18.x
+          # Exclude .NET 4.x on non-Windows OS.
+          - os: macos-latest
+            dotnet-version: net472 
+          - os: ubuntu-latest
+            dotnet-version: net472
 
     runs-on: ubuntu-latest
 
     steps:
-    - name: Publish test results (${{ matrix.os }}, node${{ matrix.node-version }}, ${{ matrix.configuration }})
+    - name: Publish test results
       uses: dorny/test-reporter@v1
       with:
-        artifact: test-logs-${{ matrix.os }}-node${{ matrix.node-version }}-${{ matrix.configuration }}
-        name: test results (${{ matrix.os }}, node${{ matrix.node-version }}, ${{ matrix.configuration }})
+        artifact: test-logs-${{ matrix.os }}-${{matrix.dotnet-version}}-node${{matrix.node-version}}-${{matrix.configuration}}
+        name: test results (${{ matrix.os }}, ${{matrix.dotnet-version}}, node${{ matrix.node-version }}, ${{ matrix.configuration }})
         path: test/**/*.trx
         reporter: dotnet-trx
diff --git a/Directory.Packages.props b/Directory.Packages.props
@@ -5,7 +5,7 @@
   <ItemGroup>
     <PackageVersion Include="BenchmarkDotNet" Version="0.13.5" />
     <PackageVersion Include="Microsoft.Bcl.AsyncInterfaces" Version="7.0.0" />
-    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.5.0" />
+    <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="4.3.0" /><!-- 4.3.0 is compatible with .NET 6 -->
     <PackageVersion Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.4" />
     <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.5.0" />
     <PackageVersion Include="Nerdbank.GitVersioning" Version="3.5.119" />
@@ -16,4 +16,4 @@
     <PackageVersion Include="xunit.runner.visualstudio" Version="2.4.5" />
     <PackageVersion Include="Xunit.SkippableFact" Version="1.4.13" />
   </ItemGroup>
-</Project>
+</Project>
diff --git a/bench/Directory.Build.props b/bench/Directory.Build.props
@@ -2,8 +2,8 @@
   <!-- The main purpose of this file is to prevent use of the root Directory.Build.props file,
   because Benchmark.NET does not like the build output paths to be redirected. -->
   <PropertyGroup>
-    <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' and ! $([MSBuild]::IsOsPlatform('Windows')) ">net8.0</TargetFrameworks>
-    <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' and $([MSBuild]::IsOsPlatform('Windows')) ">net8.0;net472</TargetFrameworks>
+    <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' and ! $([MSBuild]::IsOsPlatform('Windows')) ">net8.0;net6.0</TargetFrameworks>
+    <TargetFrameworks Condition=" '$(TargetFrameworks)' == '' and $([MSBuild]::IsOsPlatform('Windows')) ">net8.0;net6.0;net472</TargetFrameworks>
     <LangVersion>12</LangVersion>
     <Nullable>enable</Nullable>
     <PackRelease>false</PackRelease>

diff --git a/src/NodeApi.DotNetHost/NodeApi.DotNetHost.csproj b/src/NodeApi.DotNetHost/NodeApi.DotNetHost.csproj
@@ -34,8 +34,8 @@
       <NodeApiPackScript>$(MSBuildThisFileDirectory)..\node-api-dotnet\pack.js</NodeApiPackScript>
       <RuntimeIdentifierList Condition=" '$(RuntimeIdentifierList)' == '' ">$(RuntimeIdentifier)</RuntimeIdentifierList>
     </PropertyGroup>
-    <Message Importance="High" Text="node $(NodeApiPackScript) $(Configuration) $(RuntimeIdentifierList)" />
-    <Exec Command="node $(NodeApiPackScript) $(Configuration) $(RuntimeIdentifierList)" />
+    <Message Importance="High" Text="node $(NodeApiPackScript) node-api-dotnet $(Configuration) $(RuntimeIdentifierList)" />
+    <Exec Command="node $(NodeApiPackScript) node-api-dotnet $(Configuration) $(RuntimeIdentifierList)" />
   </Target>
 
 </Project>
diff --git a/src/NodeApi.DotNetHost/TypeExporter.cs b/src/NodeApi.DotNetHost/TypeExporter.cs
@@ -127,7 +127,7 @@ public void ExportAssemblyTypes(Assembly assembly, JSObject exports)
                     TypeProxy typeProxy = new(parentNamespace, nestedType);
                     parentNamespace.Types.Add(nestedTypeName, typeProxy);
                     typeProxies.Add(typeProxy);
-                    Trace($"    {parentNamespace}.{typeName}");
+                    Trace($"    {parentNamespace}.{nestedTypeName}");
                     count++;
                 }
             }
@@ -199,7 +199,6 @@ private void ExportExtensionMethod(MethodInfo extensionMethod)
         }
 
         string targetTypeName = TypeProxy.GetTypeProxyName(targetType);
-        Trace($"    +{targetTypeName}.{extensionMethod.Name}()");
 
         // Target namespaces and types should be already loaded because either they are in the
         // current assembly (where types are loaded before extension methods) or in an assembly

diff --git a/src/NodeApi.Generator/NodeApi.Generator.csproj b/src/NodeApi.Generator/NodeApi.Generator.csproj
@@ -9,6 +9,7 @@
     <EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>
     <GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>
     <SelfContained>false</SelfContained>
+    <RollForward>major</RollForward><!-- Enable running on .NET 6 or any later major version. -->
   </PropertyGroup>
 
   <ItemGroup>
@@ -56,4 +57,16 @@
     </ItemGroup>
   </Target>
 
+  <Target Name="NpmPack"
+    AfterTargets="Pack"
+    Outputs="$(PackageOutputPath)node-api-dotnet-generator-$(PackageVersion).tgz"
+  >
+    <PropertyGroup>
+      <NodeApiPackScript>$(MSBuildThisFileDirectory)..\node-api-dotnet\pack.js</NodeApiPackScript>
+      <RuntimeIdentifierList Condition=" '$(RuntimeIdentifierList)' == '' ">$(RuntimeIdentifier)</RuntimeIdentifierList>
+    </PropertyGroup>
+    <Message Importance="High" Text="node $(NodeApiPackScript) node-api-dotnet-generator $(Configuration) $(RuntimeIdentifierList)" />
+    <Exec Command="node $(NodeApiPackScript) node-api-dotnet-generator $(Configuration) $(RuntimeIdentifierList)" />
+  </Target>
+
 </Project>
diff --git a/src/NodeApi/JSError.cs b/src/NodeApi/JSError.cs
@@ -239,27 +239,34 @@ public static void ThrowError(Exception exception)
         JSValue error = (exception as JSException)?.Error?.Value ??
             JSValue.CreateError(code: null, (JSValue)message);
 
-        // When running on V8, the `Error.captureStackTrace()` function and `Error.stack` property
-        // can be used to add the .NET stack info to the JS error stack.
-        JSValue captureStackTrace = JSValue.Global["Error"]["captureStackTrace"];
-        if (captureStackTrace.IsFunction())
+        // A no-context scope is used when initializing the host. In that case, do not attempt
+        // to override the stack property, because if initialization fails the scope may not
+        // be available for the stack callback.
+        if (JSValueScope.Current.ScopeType != JSValueScopeType.NoContext)
         {
-            // Capture the stack trace of the .NET exception, which will be combined with
-            // the JS stack trace when requested.
-            JSValue dotnetStack = exception.StackTrace?.Replace("\r", string.Empty) ?? string.Empty;
-
-            // Capture the current JS stack trace as an object.
-            // Defer formatting the stack as a string until requested.
-            JSObject jsStack = new();
-            captureStackTrace.Call(default, jsStack);
-
-            // Override the `stack` property of the JS Error object, and add private
-            // properties that the overridden property getter uses to construct the stack.
-            error.DefineProperties(
-                JSPropertyDescriptor.Accessor(
-                    "stack", GetErrorStack, setter: null, JSPropertyAttributes.DefaultProperty),
-                JSPropertyDescriptor.ForValue("__dotnetStack", dotnetStack),
-                JSPropertyDescriptor.ForValue("__jsStack", jsStack));
+            // When running on V8, the `Error.captureStackTrace()` function and `Error.stack`
+            // property can be used to add the .NET stack info to the JS error stack.
+            JSValue captureStackTrace = JSValue.Global["Error"]["captureStackTrace"];
+            if (captureStackTrace.IsFunction())
+            {
+                // Capture the stack trace of the .NET exception, which will be combined with
+                // the JS stack trace when requested.
+                JSValue dotnetStack = exception.StackTrace?.Replace("\r", string.Empty)
+                    ?? string.Empty;
+
+                // Capture the current JS stack trace as an object.
+                // Defer formatting the stack as a string until requested.
+                JSObject jsStack = new();
+                captureStackTrace.Call(default, jsStack);
+
+                // Override the `stack` property of the JS Error object, and add private
+                // properties that the overridden property getter uses to construct the stack.
+                error.DefineProperties(
+                    JSPropertyDescriptor.Accessor(
+                        "stack", GetErrorStack, setter: null, JSPropertyAttributes.DefaultProperty),
+                    JSPropertyDescriptor.ForValue("__dotnetStack", dotnetStack),
+                    JSPropertyDescriptor.ForValue("__jsStack", jsStack));
+            }
         }
 
         napi_status status = error.Scope.Runtime.Throw(

diff --git a/src/NodeApi/JSObject.cs b/src/NodeApi/JSObject.cs
@@ -24,6 +24,19 @@ public JSObject() : this(JSValue.CreateObject())
     {
     }
 
+    public JSObject(IEnumerable<KeyValuePair<JSValue, JSValue>> properties) : this()
+    {
+        foreach (KeyValuePair<JSValue, JSValue> property in properties)
+        {
+            _value.SetProperty(property.Key, property.Value);
+        }
+    }
+
+    public JSObject(params KeyValuePair<JSValue, JSValue>[] properties)
+        : this((IEnumerable<KeyValuePair<JSValue, JSValue>>)properties)
+    {
+    }
+
     int ICollection<KeyValuePair<JSValue, JSValue>>.Count
         => _value.GetPropertyNames().GetArrayLength();
 

diff --git a/src/node-api-dotnet/pack.js b/src/node-api-dotnet/pack.js
@@ -7,12 +7,14 @@ if (nodeMajorVersion < 16) {
   process.exit(1);
 }
 
+const packageName = process.argv[2];
 const configuration = ['Debug', 'Release'].find(
-  (c) => c.toLowerCase() == (process.argv[2] ?? '').toLowerCase());
-const rids = process.argv.slice(3);
+  (c) => c.toLowerCase() == (process.argv[3] ?? '').toLowerCase());
+const rids = process.argv.slice(4);
 
-if (!configuration || rids.length === 0) {
-  console.error('Usage: node pack.js Debug|Release rids...');
+if (!packageName || !configuration || rids.length === 0) {
+  console.error('Missing command arguments.');
+  console.error('Usage: node pack.js package-name Debug|Release rids...');
   process.exit(1);
 }
 
@@ -32,9 +34,15 @@ const outPkgDir = path.resolve(__dirname, '../../out/pkg');
 
 if (!fs.existsSync(outPkgDir)) fs.mkdirSync(outPkgDir);
 
-packMainPackage();
-console.log();
-packGeneratorPackage();
+if (packageName === 'node-api-dotnet') {
+  packMainPackage();
+} else if (packageName === 'node-api-dotnet-generator') {
+  packGeneratorPackage();
+} else {
+  console.error('Invalid package name.');
+  console.error('Usage: node pack.js package-name Debug|Release rids...');
+  process.exit(1);
+}
 
 function packMainPackage() {
   const packageJson = require('./package.json');