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

Use high-level library load API #217

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/NodeApi/Runtime/NodejsPlatform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,39 @@ public sealed class NodejsPlatform : IDisposable
/// <summary>
/// Initializes the Node.js platform.
/// </summary>
/// <param name="libnodePath">Path to the `libnode` shared library, including extension.</param>
/// <param name="libnode">
/// Name of the `libnode` shared library.
/// Has to be a full file path when using .NET Framework.
/// </param>
/// <param name="args">Optional application arguments.</param>
/// <param name="execArgs">Optional platform options.</param>
/// <exception cref="InvalidOperationException">A Node.js platform instance has already been
/// loaded in the current process.</exception>
public NodejsPlatform(
string libnodePath,
string libnode,
string[]? args = null,
string[]? execArgs = null)
{
if (string.IsNullOrEmpty(libnodePath)) throw new ArgumentNullException(nameof(libnodePath));

if (Current != null)
{
throw new InvalidOperationException(
"Only one Node.js platform instance per process is allowed.");
}

nint libnodeHandle = NativeLibrary.Load(libnodePath);
#if NET7_0_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

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

Can this be !NETFRAMEWORK? The 3-parameter overload of NativeLibrary.Load() is present on .NET 6.

Copy link
Author

Choose a reason for hiding this comment

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

This code was removed in the newest revision.

var entryAssembly = System.Reflection.Assembly.GetEntryAssembly();
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider using DllImportSearchPath.AssemblyDirectory instead? I think technically that would use the current assembly rather than the entry assembly, but that might actually be better.

Copy link
Author

Choose a reason for hiding this comment

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

Even using AssemblyDirectory, you still need to pass an assembly. I'd rather use the system default but I can change the assembly to get the current assembly instead of the entry.


nint libnodeHandle =
entryAssembly != null
? NativeLibrary.Load(libnode, entryAssembly, null)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little concerned about the lack of functional parity on .NET Framework 4.x. Would it be feasible to add code in this repo's NativeLibrary.cs to at least replicate the logic for loading from the same directory as the assembly?

Copy link
Author

Choose a reason for hiding this comment

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

I've implemented a partially-featured replica of the original method, but I can't test it because I don't have access to a Windows machine.

: NativeLibrary.Load(libnode);
#else
if (string.IsNullOrEmpty(libnode))
throw new ArgumentNullException(nameof(libnode));

nint libnodeHandle = NativeLibrary.Load(libnode);
#endif

Runtime = new NodejsRuntime(libnodeHandle);

Runtime.CreatePlatform(args, execArgs, (error) => Console.WriteLine(error), out _platform)
Expand Down