build: Fix various shared library build issues.

[lux01]

Node.js unofficially supports a shared library variant where the main node executable is a thin wrapper around node.dll/libnode.so. The key benefit of this is to support embedding Node.js in other applications, for example the product I work on embeds a Node.js runtime in 3 separate applications (alongside the JVM and CLR in the same processes) in addition to having some standalone Node.js applications.

Since Node.js 12 there have been a number of issues preventing the shared library build from working correctly with the most significant issues being on Windows:

• A number of functions used executables such as mksnapshot are not exported from libnode.dll using a NODE_EXTERN attribute
• A dependency on the Winmm system library is missing
• Incorrect defines on executable targets leads to node.exe claiming to export a number of functions that are actually in libnode.dll
• Because node.exe attempts to export symbols, node.lib gets generated causing native extensions to try to link against node.exe not libnode.dll.
• Similarly, because node.dll was renamed to libnode.dll, native extensions don't know to look for libnode.lib rather than node.lib.
• On macOS an RPATH is added to find libnode.dylib relative to node in the same folder. This works fine from the out/Release folder but not from an installed prefix, where node will be in bin/ and libnode.dylib will be in lib/.
• Similarly on Linux, the only RPATH that is added is $ORIGIN so LD_LIBRARY_PATH needs setting correctly for bin/node to find lib/libnode.so.

For the libnode.lib vs node.lib issue there are two possible options:

1. Ensure node.lib from node.exe does not get generated, and instead copy libnode.lib to node.lib. This means addons compiled when referencing the correct node.lib file will correctly depend on libnode.dll. The down side is that native addons compiled with stock Node.js will still try to resolve symbols against node.exe rather than libnode.dll.
2. After building libnode.dll, dump the exports using dumpbin, and process this to generate a node.def file to be linked into node.exe with the /DEF:node.def flag. The export entries in node.def would all read

   my_symbol=libnode.my_symbol
   

   so that node.exe will redirect all exported symbols back to libnode.dll. This has the benefit that addons compiled with stock Node.js will load correctly into node.exe from a shared library build, but means that every embedding executable also needs to perform this same trick.

I went with the first option as it is the cleaner of the two solutions in my opinion. Projects wishing to generate a shared
library variant of Node.js can now, for example,

.\vcbuild dll package vs2019

to generate a full node installation including libnode.dll, Release\node.lib, and all the necessary headers. Native addons can then be built against the shared library easily by specifying the correct nodedir option.

For example

>npx node-gyp configure --nodedir C:\Users\User\node\Release\node-v18.0.0-win-x64
...
>npx node-gyp build
...
>dumpbin /dependents build\Release\binding.node
Microsoft (R) COFF/PE Dumper Version 14.29.30136.0
Copyright (C) Microsoft Corporation.  All rights reserved.

Dump of file build\Release\binding.node

File Type: DLL

  Image has the following dependencies:

    KERNEL32.dll
    libnode.dll
    VCRUNTIME140.dll
    api-ms-win-crt-string-l1-1-0.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-runtime-l1-1-0.dll
...

I have tested my changes on Linux/x86_64, Linux/s390x, Linux/ppc64le (all on RHEL 7.9 devtoolset-10), Windows/x86_64 (VS2019 on Windows Server 2019 Datacenter Edition), and macOS/x86_64 (macOS 12.1 with Apple Clang 13.0.0). I have tried to test on AIX but the GCC 8 installation on the machine I have access to is currently broken...

Fixes #34539
Fixes #41559

This PR is essentially a set of patches that I have been maintaining for the Node.js 14.x branch internally in my day job. Ideally I would love to see these changes get back ported but I can understand if this is not desireable.

Short of making shared library builds of Node.js the default, as is common for other embeddable runtimes such as the JVM or the CLR, or by building both the standard build and the shared library build together in the CI, I don't see a way to prevent changes that break the shared library from being merged in the future.

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/startup
• @nodejs/v8-update

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/42762/

[Trott]

@nodejs/platform-windows

[Trott]

Welcome @lux01 and thanks for the pull request! Can you rebase this against the current master branch and resolve the conflict?

[Trott]

@nodejs/build

[mhdawson]

Short of making shared library builds of Node.js the default, as is common for other embeddable runtimes such as the JVM or the CLR, or by building both the standard build and the shared library build together in the CI, I don't see a way to prevent changes that break the shared library from being merged in the future.

We do have this nightly job - https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-as-shared-lib/ (and the AIX equivalent which is broken). Not as good as being part of the regular PR builds, but better than nothing.

[mhdawson]

For the libnode.lib vs node.lib issue there are two possible options

@codebytere does Electron have a similar problem?

[mhdawson]

Build of the nightly AIX shared library runs to see if it resolves the existing failures - https://ci.nodejs.org/job/node-test-commit-aix-shared-lib/nodes=aix71-ppc64/1424/

[mhdawson]

I think this needs to have the comment be in lines above the flags so that we can keep it within 80 char limit we have set in the project.

[lux01]

Sorry I pushed some work-in-progress AIX changes to this branch instead of a new one! I'll remove the AIX changes from the PR.

[mhdawson]

I went with the first option as it is the cleaner of the two solutions in my opinion. Projects wishing to generate a shared
library variant of Node.js can now, for example,

I'm guessing that this breaks the ability to pre-compile addons with node-addon-api and have them run with different node binaries? It would mean that any module that ships pre-compiled binaries would either fail for shared library based exes, or have to fall back to compiling? In option 2) are there additional complications if the name of the exe is going to be different than node ?

[mhdawson]

The run against the AIX shared library test job still failed: https://ci.nodejs.org/job/node-test-commit-aix-shared-lib/nodes=aix71-ppc64/1424/console

with

63d331e9623ae28300e3332ea10b0ecd73b171f.intermediate fac787f71094027d2f939f80bd708084107816d7.intermediate
if [ ! -r node ] || [ ! -L node ]; then \
  ln -fs out/Release/node node; fi
Could not load program ./node:
	Dependent module libnode.105.a could not be loaded.
Could not load module libnode.105.a.
System error: No such file or directory
gmake[1]: *** [Makefile:457: benchmark/napi/.buildstamp] Error 255

@lux01 That's actually failing earlier, as the job used to try to build addons and was filing with

[mhdawson]

Job on OSX testing with --shared option - https://ci.nodejs.org/view/All/job/node-test-commit-osx-as-shared-mdawson/nodes=osx1015/2/console

One test seems to fail, but I don't believe it is related to this PR as it failed on main as well:

18:17:27 not ok 3236 pummel/test-heapdump-http2
18:17:27   ---
18:17:27   duration_ms: 1.751
18:17:27   severity: crashed
18:17:27   exitcode: -11
18:17:27   stack: |-
18:17:27     (node:11723) internal/test/binding: These APIs are for internal testing only. Do not use them.
18:17:27     (Use `node --trace-warnings ...` to show where the warning was created)
18:17:27   ...

otherwise the test suite seems to run clean with the --shared config option.

[mhdawson]

CI run for linux - https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-as-shared-lib/1439/

[mhdawson]

@lux01 I'm wondering why the node.lib goes into a new sub directory called Release ? The install.py seems to install libnode.lib and libnode.dll so not quite sure why there is a node.lib being copied here.

[lux01]

This is needed when using the --nodedir option to build native addons. When this is specified, node-gyp explicitly looks for node.lib in the Release or Debug subfolder of the node installation.

https://github.com/nodejs/node-gyp/blob/91eb407f8418235d51c72b6a8df9c454ba882554/lib/configure.js#L209-L211

[mhdawson]

k thanks.

[mhdawson]

Also wondering what this is needed for

[lux01]

As above, node-gyp tries to find common.gypi in the root of the nodedir if its not at include/node/common.gypi. I think this may be a remnant left over from before I figured out the HEADERS_ONLY install below. I'll try removing it

[mhdawson]

A number of functions used executables such as mksnapshot are not exported from libnode.dll using a NODE_EXTERN attribute

@lux01, I'm wondering why this is not a problem in the regular builds but is for the shared library build?

[mhdawson]

Is this necessary for all platforms? I'm wondering since the only issues you mentioned for linux/osx don't seem related to this.

[lux01]

I think you may be right that these are only necessary on Windows. Their purpose is to ensure that libuv and v8 classes are correctly annotated with __declspec(dllimport) rather than __declspec(dllexport). I will refactor this condition block to only apply to Windows.

[bnoordhuis]

It shouldn't hurt (and is more correct) to enable these on non-Windows.

[lux01]

A number of functions used executables such as mksnapshot are not exported from libnode.dll using a NODE_EXTERN attribute

@lux01, I'm wondering why this is not a problem in the regular builds but is for the shared library build?

The mksnapshot executable unconditionally depends on the node_lib_target:

node/node.gyp

Lines 1398 to 1402 in 0367b5c

	'target_name': 'node_mksnapshot',
	'type': 'executable',
	'dependencies': [
	'<(node_lib_target_name)',

In both the main build and the shared library build cases this resolves to libnode.lib, but crucially in the main build this is a static library containing all the code, whereas in the shared library build it is the implib for libnode.dll. This means that in the main build libnode.lib will contain all the symbols needed from the main node codebase for these executables, but in the shared library build these symbols will not be exported and so not visible. Adding NODE_EXTERN to them allows the build to continue and we end up with executables that depend on libnode.dll at runtime.

---

I'm guessing that this breaks the ability to pre-compile addons with node-addon-api and have them run with different node binaries? It would mean that any module that ships pre-compiled binaries would either fail for shared library based exes, or have to fall back to compiling? In option 2) are there additional complications if the name of the exe is going to be different than node ?

You're right, I hadn't considered that as a use case. The product I work on only uses a small number of native addons, none of which are shipped pre-compiled. Addons that only ship pre-compiled binaries would not work at all, others would have to be rebuilt.

With regards to the executable name issue, node-gyp quite helpfully already handles this for us. Firstly it tells the linker to mark node.exe as a [delay load] dependency

https://github.com/nodejs/node-gyp/blob/0da2e0140dbf4296d5ba7ea498fac05e74bb4bbb/addon.gypi#L81

Secondly it injects a delay load hook into the native addon which is run when Windows tries to resolve the delayed node.exe. It intercepts the request to find node.exe and instead returns a handle to the host process executable. This means that all node.exe symbols will be resolved against the host process.

https://github.com/nodejs/node-gyp/blob/91eb407f8418235d51c72b6a8df9c454ba882554/src/win_delay_load_hook.cc#L23-L35

As long as the host process exports the necessary symbols, its name does not matter.

For option (2) to work we have to rely on this delay load behaviour and ensure that the host process, whatever it is (be it a renamed node.exe or an embedder) exports the symbols itself (which is what happens in standard builds) or provides a redirected export back to libnode.dll.

From what I can tell, Electron doesn't have this issue because it doesn't use the shared library build of node, at least on Windows. I'm inferring this because running dumpbin /exports on an Electron app (such as VS Code) shows that the .exe is directly exporting all the napi_ functions as well as other Node.js internal functions (such as node::Start()). Furthermore if one dumps the node.lib file for an electron release, e.g. https://electronjs.org/headers/v6.0.9/win-x64/node.lib, its symbols refer to node.exe rather than a .dll

  Version      : 0
  Machine      : 8664 (x64)
  TimeDateStamp: 00000000
  SizeOfData   : 0000002A
  DLL name     : node.exe
  Symbol name  : napi_acquire_threadsafe_function
  Type         : code
  Name type    : name
  Hint         : 0
  Name         : napi_acquire_threadsafe_function

To get option (2) to work we would need to run the following steps after building libnode.dll but before building node.exe.

1. Dump the symbols from libnode.dll using dumpbin /exports (or using some sort of PE32+ parser)
2. Generate a module-definition (.def) file which contains a redirect for each exported symbol in libnode.dll back to libnode.dll.
3. Pass in /def:path\to\libnode.def as a parameter to the linker when building node.exe
4. Include libnode.def in the release image for shared library builds so that embedders of Node.js can also re-export the symbols.

This is very achievable but is also very messy and means your host executable is tightly coupled with libnode.dll in a non-obvious way. The product I work on currently does this but we would prefer not to given the choice.

We have a similar issue on AIX where native addons currently assume all their symbols can be found in the host executable. I think there we only have option (2) available to us. I'm still investigating...

[mhdawson]

I build/ran on Windows and see some errors. I assume it is likely something I've not set up properly because I don't build on windows very often. @lux01 do you get a clean test pass and did you have to set up anything beyond what is outlined in https://github.com/nodejs/node/blob/master/BUILDING.md#option-1-manual-install to get everything passing?

EDIT2 setting up the linux tools in the path AND running as admin, I'm down to 1 failures running with python tool\test.py

C:\Users\midawson\node>python tools\test.py
Skipping pseudo-tty tests, as pseudo terminals are not available on Windows.
=== release test-fs-stat ===
Path: parallel/test-fs-stat
node:assert:991
    throw newErr;
    ^

AssertionError [ERR_ASSERTION]: ifError got unwanted exception: EISDIR: illegal operation on a directory, fstat
    at C:\Users\midawson\node\test\common\index.js:374:12
    at C:\Users\midawson\node\test\common\index.js:411:15
    at FSReqCallback.oncomplete (node:fs:197:21) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: [Error: EISDIR: illegal operation on a directory, fstat] {
    errno: -4068,
    code: 'EISDIR',
    syscall: 'fstat'
  },
  expected: null,
  operator: 'ifError'
}

Node.js v18.0.0-pre
Command: C:\Users\midawson\node\out\Release\node.exe C:\Users\midawson\node\test\parallel\test-fs-stat.js
[05:55|% 100|+ 3369|-   1]: Done

And I found - #40006 so clearly the issue is not related to the PR. I'll take my results as a pass running the Node.js on windows using the shared library build with the wrapper node.exe and libnode.dll

[mhdawson]

@lux01 - Thanks for the detailed reply in #41850 (comment)

I think you are right that electron likely builds an exe that has statically linked in what it needed from Node.js versus using an external shared library. I believe that is also the case for boxnode so the shared library may be a different use case than those two. In terms of native addons, my take is that is an area where we could improve, but I don't think it should be a blocker for this PR as I don't think it breaks existing functionality, just improves what does work when built on a shared library (Correct me if I'm wrong on that).

From my testing so far I can confirm that on Windows, Linux and OSX it results in a shared library and a node.exe wrapper that builds and passes the test suites (well, close enough that I think the single failure I see on each platform is not related to this PR). That leaves AIX which I think you were going to cover in a follow on PR?

[mhdawson]

The main remaining question I have is related to NODE_EXTERN and if there would be any concern about the symbols being exported signaling some sort of commitment for support. I think that on non-windows platforms they are exported already, and from the explanation in #41850 (comment) they are exported in the windows static builds already so my thinking is that is should not change anything with respect to public exported API.

I looked through our documentation and did not find anything with guidance on when to/when not to add NODE_EXTERN.

I asked @jasnell and he indicated he thought it was ok as well but that maybe we should see if @bnoordhuis would chime in as well. @bnoordhuis do you have any concerns with us adding NODE_EXTERN to the methods as done in this PR?

[mhdawson]

@lux01 can you comment on why this change is needed?

[lux01]

This is needed when the path to python.exe has a space in it. Python 3.10 when installed for all users defaults to C:\Program Files\Python310\python.exe which fails without this change:

PS C:\source\node> .\vcbuild.bat debug vs2019 x64 dll
Looking for Python
Python found in C:\Program Files\Python310\\python.exe
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
Not an executable Python program
Could not find Python.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43840/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43845/

[mhdawson]

Landed in 1af7054. @lux01 thanks for the help in getting this done. I'll update the PR for the related docs next.

[lux01]

Thank you @mhdawson! Glad to see this finally close :)