Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

cross platform native modules didn't work for me #1319

Closed
tonywoode opened this issue Sep 24, 2021 · 37 comments · Fixed by #1327, #1331 or #1334
Closed

cross platform native modules didn't work for me #1319

tonywoode opened this issue Sep 24, 2021 · 37 comments · Fixed by #1327, #1331 or #1334

Comments

@tonywoode
Copy link

tonywoode commented Sep 24, 2021

re: #837 - is cross-platform native module packing supposed to work, or not? Prebuild-install is supposed to do this right: https://github.com/vercel/pkg/blob/main/lib/producer.ts#L200

pkg 5.3.2
souce os: macos 10.15.7
target: win
how am I configuring pkg? Just with cmd parameters in my package.json: pkg src/my.js -t win --out-path /Volumes/[C]/bin/myname (I tried specifying the target with node version and architecture, made no difference)

What happened?

pkg packaged up the macos version of iconv's module: iconv.node into the windows package. When I ran the exe, Windows errored this wasn't a win32 application

What did I expect to happen?

From the readme or --help I wasn't sure: Either to bundle the correct file, or If the target os's native module isn't available, some automatic way to fall back to the old behaviour of expecting the right .node file to be available after compile in the location of the binary, failing that, a config setting that will give me the old behaviour or allow me to bundle the correct .node file manually (does specifying an asset override native module compilation, or not?). Currently I don't get how to achieve any of those (since i've been best with the post-compile same-location method until now)

How did I temporarily/manually fix my problem?

I copied the windows native module iconv.node into the location in mac's node_modules where the mac iconv.node should be, then ran the packaging again, then once the binary was made, I copied the mac iconv.node back where it should be.

How can I automate that better, or just get the old behaviour back (easiest is just how do I stop pkg trying to bundle the .node file here?)

Also how can we better-document what the correct approach is to cross-platform native module compilation?

tonywoode added a commit to tonywoode/quickPlay that referenced this issue Sep 24, 2021
pkg recently added support for native modules, sadly not great: expat
doesn't register as a native module, and iconv copies the
macos executable into the windows exe! opened
vercel/pkg#1319 - to fix manually I copied
the windows exe to the macs path and build again!
@tonywoode
Copy link
Author

tonywoode commented Sep 27, 2021

The situation is slightly more intereresting with --debug.

Situation 1: with the additional problem of #1075 because I'm using npm not yarn:

> [debug] prebuild-install failed[/Users/me/mymodule/node_modules/iconv/build/Release/iconv.node]:
  Error: spawnSync /Users/me/mymodule/node_modules/pkg/node_modules/.bin/prebuild-install ENOENT

Situation 2: if I solve that problem with a quick mkdir node_modules/pkg/node_modules/.bin && ln -s node_modules/prebuild-install/bin.js node_modules/pkg/node_modules/.bin/prebuild-install

prebuild-install WARN install No prebuilt binaries found (target=v14.17.6 runtime=node arch=x64 libc= platform=win32)
> [debug] prebuild-install failed[/Users/me/mymodule/node_modules/iconv/build/Release/iconv.node]:
  Error: Command failed: /Users/me/mymodule/node_modules/pkg/node_modules/.bin/prebuild-install --target v14.17.6 --platform win32 --arch x64
prebuild-install WARN install No prebuilt binaries found (target=v14.17.6 runtime=node arch=x64 libc= platform=win32)

In both cases its the macos iconv.node file which has been brute-copied into the binary...

I tried using pkg/bin properties in package.json, and I did manage to trigger this .bak generation once: https://github.com/vercel/pkg/blob/main/lib/producer.ts#L221, but it triggered against my manually downloaded windows iconv.node file, and I didn't get the right .node file in my compile

tonywoode added a commit to tonywoode/quickPlayNode that referenced this issue Sep 27, 2021
see vercel/pkg#1319 <-- tried making a pkg
property in package.json specifying assets, resolving prebuild-install
for npm to no avail. There's no way to stop pkg packaging the macos
binary, so patch in the windows iconv node file before build, and
put the macos iconv node file back again once compiling completes (for
dev). There is another node file for expat, but this doesn't pick up and
the old behaviour of expecting it in the destination folder is
maintained - is this due to an obfuscated require? If so, that could be
a more elegant sloution?
@robertsLando
Copy link
Contributor

robertsLando commented Sep 29, 2021

The fix proposed in #1075 could also fix this

@tonywoode
Copy link
Author

tonywoode commented Oct 1, 2021

#1075 DOES fix this as of pkg 5.3.3! Thank you so much!!! Or at least, I think it fixes this because that error has gone away. Now we're onto a new error when I try to run the windows executable. I should mention the second issue I have with native modules is that I have a second transitively-installed native module in my project: node-expat

With pkg 5.3.2: node-expat's .node file isn't picked up by pkg's new native module compilation abilities at all, and the old behaviour was maintained (I could just put the windows .node file in the .exe's directory as I always did)

With pkg 5.3.3:

C:\mymodule>themodule.exe
pkg/prelude/bootstrap.js:1740
      throw error;
      ^

TypeError: Cannot read property '1' of null
    at process.dlopen (pkg/prelude/bootstrap.js:2071:69)
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:1131:18)
    at Module.load (internal/modules/cjs/loader.js:937:32)
    at Function.Module._load (internal/modules/cjs/loader.js:778:12)
    at Module.require (internal/modules/cjs/loader.js:961:19)
    at Module.require (pkg/prelude/bootstrap.js:1719:31)
    at require (internal/modules/cjs/helpers.js:92:18)
    at bindings (C:\snapshot\mymodule\node_modules\bindings\bindings.js:112:48)
    at Object.<anonymous> (C:\snapshot\mymodule\node_modules\node-expat\lib\node-expat.js:4:34)
    at Module._compile (pkg/prelude/bootstrap.js:1794:22)

note these modules are transtitive dependencies required by the module i'm actually using (so I have little control over how they are imported). Also note that prior to pkg 4.5.0 everything worked fine (once I placed the platform-specific .node files in the compiled binary's directory)

tonywoode added a commit to tonywoode/quickPlayNode that referenced this issue Oct 1, 2021
see vercel/pkg#1319, this makes the package
fail because of vercel/pkg#1075 rather than
fail in the same way because of the missinng dependency
tonywoode added a commit to tonywoode/quickPlayNode that referenced this issue Oct 1, 2021
pkg 4.5.0 added the native module functionality which hasn't worked for
me - see vercel/pkg#1319
in the latest 5.5.3 iconv.node now compiles but node-expat now craps
out. To enable dev work to continue, fix to ~ minor version before
this was added
tonywoode added a commit to tonywoode/quickPlay that referenced this issue Oct 1, 2021
instead of trying to otherwise mitigate (see
vercel/pkg#1319)
@robertsLando
Copy link
Contributor

I think that error could happen while trying to parse the module by using a regexp, if you check the change proposed in the fix you will see the code, maybe we should check for the regex match length before trying to access props, would you like to submit a fix?

@ghost
Copy link

ghost commented Oct 4, 2021

I'm experiencing the same issue with the tdl-tdlib-addon module. The below line in the module gives the error.

this._addon = require('../build/Release/td.node')

Error:

pkg/prelude/bootstrap.js:1740
      throw error;
      

TypeError: Cannot read property '1' of null
    at process.dlopen (pkg/prelude/bootstrap.js:2071:69)
    at Object.Module._extensions..node (node:internal/modules/cjs/loader:1183:18)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at Module.require (pkg/prelude/bootstrap.js:1719:31)
    at require (node:internal/modules/cjs/helpers:94:18)
    at new TDLib (C:\snapshot\mymodule\node_modules\tdl-tdlib-addon\dist\index.js:37:19)
    at Object.<anonymous> (C:\snapshot\mymodule\index.js)
    at Module._compile (pkg/prelude/bootstrap.js:1794:22)

@robertsLando
Copy link
Contributor

I'm quite sure the error happens here. Could you make some tests? @kosener , you could manually edit bootstrap code of your installed pkg module to add a debug before the regex to see what module it's tring to import

@robertsLando
Copy link
Contributor

Ok I have found the problem, fix ready on #1327 if you want to give it a try

@tonywoode
Copy link
Author

tonywoode commented Oct 4, 2021

thanks again!

hmmm, fraid I'm seeing a regression; what i'm seeing suggests its my process that's wrong, but I can't see how:

# with pkg 5.3.3 npm installed
$ npm run packageParallels

> mymodule@1.0.0 packageParallels /Users/me/mymodule
> pkg mymodule/myrunner.js -t win --out-path /Volumes/[C]\myImage/bin

> pkg@5.3.3

# this gets me iconv.node packaged but node-expat.node not packaged and of course the regex error disscussed above

$ npm link pkg
/Users/me/mymodule/pkg -> /Users/me/.nvm/versions/node/v14.16.1/lib/node_modules/pkg -> /Users/me/pkg-repo-git-cloned/pkg

# now with commit bf8c6 npm linked
$ npm run packageParallels

> mametool@1.0.0 packageParallels /Users/me/mymodule
> pkg src/QPNode.js -t win --out-path /Volumes/[C]\myImage/bin

> pkg@5.3.3
prebuild-install WARN install No prebuilt binaries found (target=v14.17.6 runtime=node arch=x64 libc= platform=win32)

adding --debug shows that the problem is back to prebuild-install finding a cache of iconv.node:

> [debug] prebuild-install failed[/Users/me/mymodule/node_modules/iconv/build/Release/iconv.node]:

how can the prebuild-install in /Users/me/pkg-repo-git-cloned/pkg not find the appropriate iconv.node, when pkg 5.3.3 seems to? Can't be the change you made, since that affected someting only downstream of here, once the compiled binary is run

$ cd /Users/me/pkg-repo-git-cloned/pkg && pwd && git log | head -5
/Users/me/pkg-repo-git-cloned/pkg
commit bf8c62c428a81cc7873ef2a73f0306a6e36ed0db
Author: Daniel Lando <******.com>
Date:   Mon Oct 4 11:01:30 2021 +0200

    bootstrap: skip regex when dlopen path isn't inside snapshot (#1327)

@robertsLando
Copy link
Contributor

Did you added the node to assets in package.json?

@tonywoode
Copy link
Author

no there's was no pkg key in my package.json in either of the compilation runs above (npm run packageParallels). Note in the first run no error, second run error.

@robertsLando
Copy link
Contributor

You have to add the .node files to assets in order to work, example:

"pkg": {
    "assets": [
      "./node_modules/sharp/build/Release/**",
      "./node_modules/sharp/vendor/**"
    ]
  },

@tonywoode
Copy link
Author

pkg 5.3.3@bf8c6 installed. This key in root of myModule'spackage.json:

 "pkg": {
    "assets": [
      "./node_modules/iconv/build/Release/**",
      "./node_modules/node-expat/build/Release/**"
    ]
  },

But the compile still fails to produce a working binary, warning about iconv:

prebuild-install WARN install No prebuilt binaries found (target=v14.17.6 runtime=node arch=x64 libc= platform=win32)
> [debug] prebuild-install failed[/Users/my/mymodule/node_modules/iconv/build/Release/iconv.node]:
  Error: Command failed: /Users/me/pkg-repo-git-cloned/pkg/node_modules/.bin/prebuild-install --target v14.17.6 --platform win32 --arch x64
prebuild-install WARN install No prebuilt binaries found (target=v14.17.6 runtime=node arch=x64 libc= platform=win32)

Does this suggest the need for a flag for reverting to the behaviour of 4.4.9 and not trying to build native modules?

@robertsLando
Copy link
Contributor

Ignore the warning, does it run then? If not what's the error thrown?

@tonywoode
Copy link
Author

tonywoode commented Oct 8, 2021

no the binary produced is 35 meg when the working binary was 75 meg, so presume it bombed out (plus that's the last message in the --debug output). The error in windows at this point:

pkg/prelude/bootstrap.js:1740
      throw error;
      ^

TypeError: Cannot read property '1' of null
    at process.dlopen (pkg/prelude/bootstrap.js:2072:71)
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:1131:18)
    at Module.load (internal/modules/cjs/loader.js:937:32)
    at Function.Module._load (internal/modules/cjs/loader.js:778:12)
    at Module.require (internal/modules/cjs/loader.js:961:19)
    at Module.require (pkg/prelude/bootstrap.js:1719:31)
    at require (internal/modules/cjs/helpers.js:92:18)
    at Object.<anonymous> (C:\snapshot\myApp\node_modules\iconv\lib\iconv.js:27:14)
    at Module._compile (pkg/prelude/bootstrap.js:1794:22)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)

@robertsLando
Copy link
Contributor

@tonywoode that error is fixed on main branch, try to use that, 5.3.3 has a bug

@tonywoode
Copy link
Author

tonywoode commented Oct 8, 2021 via email

@robertsLando
Copy link
Contributor

robertsLando commented Oct 8, 2021

Try to patch bootstrap and add a log here

console.log(moduleFolder)

After doing that create the bianry and run and show me the oputput

@tonywoode
Copy link
Author

tonywoode commented Oct 8, 2021

When we fail at line 1740, I have closest to me this

 Module {
  id: 'C:\\snapshot\\quickPlayNode\\node_modules\\iconv\\lib\\iconv.js',
  path: 'C:\\snapshot\\quickPlayNode\\node_modules\\iconv\\lib',
  exports: { Iconv: [Function: Iconv] },
  parent: Module {
    id: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\lib\\xml-stream.js',
    path: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\lib',
    exports: {},
    parent: Module {
      id: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\index.js',
      path: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream',
      exports: {},
      parent: [Module],
      filename: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\index.js',
      loaded: false,
      children: [Array],
      paths: [Array]
    },
    filename: 'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\lib\\xml-stream.js',
    loaded: false,
    children: [ [Module], [Module], [Circular *1] ],
    paths: [
      'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\lib\\node_modules',
      'C:\\snapshot\\quickPlayNode\\node_modules\\xml-stream\\node_modules',
      'C:\\snapshot\\quickPlayNode\\node_modules',
      'C:\\snapshot\\node_modules',
      'C:\\node_modules'
    ]
  },
  filename: 'C:\\snapshot\\quickPlayNode\\node_modules\\iconv\\lib\\iconv.js',
  loaded: false,
  children: [
    Module {
      id: 'C:\\snapshot\\quickPlayNode\\node_modules\\safer-buffer\\safer.js',
      path: 'C:\\snapshot\\quickPlayNode\\node_modules\\safer-buffer',
      exports: [Object],
      parent: [Circular *1],
      filename: 'C:\\snapshot\\quickPlayNode\\node_modules\\safer-buffer\\safer.js',
      loaded: true,
      children: [],
      paths: [Array]
    }
  ],
  paths: [
    'C:\\snapshot\\quickPlayNode\\node_modules\\iconv\\lib\\node_modules',
    'C:\\snapshot\\quickPlayNode\\node_modules\\iconv\\node_modules',
    'C:\\snapshot\\quickPlayNode\\node_modules',
    'C:\\snapshot\\node_modules',
    'C:\\node_modules'
  ]
}

and arguments is [Arguments] { '0': '../build/Release/iconv.node' }

@tonywoode
Copy link
Author

tonywoode commented Oct 8, 2021

and moduleFolder at line 2066 prints twice, here they are:

P:\QUICKPLAY\QuickPlayFrontend\qp\bin
C:\snapshot\quickPlayNode\node_modules\iconv\build\Release

@robertsLando
Copy link
Contributor

OK I think the problem is that the regex is not working with windows paths, I need to update it

@robertsLando
Copy link
Contributor

I think this could work:

const sep = path.sep
const parts = moduleFolder.split(sep)

const mIndex = parts.indexOf('node_modules') + 1

const modulePackagePath = parts.slice(mIndex).join(sep)
const modulePackageName = parts[mIndex]
const modulePkgFolder = parts.slice(0, mIndex + 1).join(sep)

Replace https://github.com/vercel/pkg/blob/main/prelude/bootstrap.js#L2069-L2078 with that code and retry

@robertsLando robertsLando reopened this Oct 8, 2021
@robertsLando
Copy link
Contributor

PR opened: #1331

jesec pushed a commit that referenced this issue Oct 9, 2021
Seems that the regex fails in Windows, this is because the path separator is hardcoded in the regex.

This fix uses a different and cleaner approach and will work on every OS. It also handles cases where
the required file isn't inside node_modules folder

Fixes: #1319
@tonywoode
Copy link
Author

tonywoode commented Oct 9, 2021

thanks for this! I'm now on e9ad32628d, still having this in my package.json:

"pkg": {
    "assets": [
      "./node_modules/iconv/build/Release/**",
      "./node_modules/node-expat/build/Release/**"
    ]
  },

still making a binary half the size of the working one, now the error in windows is back to:

pkg/prelude/bootstrap.js:1740
      throw error;
      ^

Error: C:\Users\me\AppData\Local\Temp\pkg\f7cfc5f151ddc2ef2a7b0fc56cbb56ccbdb035475df82ccd886e41d19050fb6a\iconv\build\Release\iconv.node is not a valid Win32 application.

the compile on debug still telling me, just before logging ends

> [debug] prebuild-install failed[/Users/me/mymodule/node_modules/iconv/build/Release/iconv.node]:

@robertsLando
Copy link
Contributor

Seems like you have built your node_modules for linux

@tonywoode
Copy link
Author

i think it will be macos: I think this loops us back to my initial issue post, since despite your coding efforts, the behaviour I get is now the same as when I initially posted, and it seems I still have the same questions:

is cross-platform native module packing supposed to work, or not? Prebuild-install is supposed to do this right: https://github.com/vercel/pkg/blob/main/lib/producer.ts#L200
souce os: macos 10.15.7
target: win
how am I configuring pkg? Just with cmd parameters in my package.json
What happened? pkg packaged up the macos version of iconv's module
What did I expect to happen? From the readme or --help I wasn't sure

@robertsLando
Copy link
Contributor

is cross-platform native module packing supposed to work, or not? Prebuild-install is supposed to do this right:

Nope, It cannot work. Native modules MUST be built with the required os. I use docker for such purposes

@tonywoode
Copy link
Author

tonywoode commented Oct 11, 2021

Yes, glad we understand each other now! I don't have a docker option, therefore to carry on using pkg now, the easiest way would be a flag to prevent automatically-including native modules and fallback to the previous (pkg ~4.4.9) behaviour. (Maybe you can see that pkg ^5.3 DID always automatically include my macos .node file, that's why I posted in the first place, because now there is no way for me to get a working binary anymore - capisci?)

It would be helpful, though not critical, if the code could also understand that the target operating system is not the same as the source, and fallback automatically, fail, or warn

thanks for all the efforts though!

@robertsLando
Copy link
Contributor

I think the problem is not that change but this commit: 3b947c8

What we would need here is to add an options to skip native modules build

@robertsLando
Copy link
Contributor

@tonywoode Check my last PR and tell me if adding --no-native-build fixes your problem

@tonywoode
Copy link
Author

tonywoode commented Oct 13, 2021

thank you very much indeed!!! I've built on commit 0435a43 and invoked pkg with --no-native-build. That's stopped any errors on compile, but still gives me the same error trying to run the binary in windows:

Error: C:\Users\me\AppData\Local\Temp\pkg\f7cfc5f151ddc2ef2a7b0fc56cbb56ccbdb035475df82ccd886e41d19050fb6a\iconv\build\Release\iconv.node is not a valid Win32 application.

I have taken out any pkg.assets keys from my package.json - perhaps that points to the issue? pkg has always added the .node file without me having to do anything. --debug output includes

> [debug] Stat info of /Users/me/mymodule/node_modules/iconv/build/Release/iconv.node is added to queue.
...
> [debug] The file was included as asset content
  /Users/my/mymodule/node_modules/iconv/build/Release/iconv.node
> [debug] The file was included as asset content
  /Users/me/mymodule/node_modules/iconv/build/Release/iconv.node

your flag doesn't seem to be preventing this? Even If I change to false this in line 255 of lib/index.ts

    default: { bytecode: true, 'native-build': false },

and rebuild, I still get the same issue. Also the compile is bombing out still - my binary is still 35 meg when a working one is 75, and if i add log messages further downstream they don't get printed.

What i'd expect to see in the console on compile is the old behaviour. It used to print:

> pkg@4.4.9
> Warning Cannot include addon %1 into executable.
  The addon must be distributed with executable as %2.
  %1: /Users/twoode/CODE/Scripts/quickPlayNode/node_modules/iconv/build/Release/iconv.node
  %2: path-to-executable/iconv.node

@robertsLando
Copy link
Contributor

@tonywoode check your package.json and be sure there is no pkg assets set, the .node files are not automatically added to assets except if you specify them in package.json

@tonywoode
Copy link
Author

I have taken out any pkg.assets keys from my package.json

Note that i've tried to show several times that the iconv.node file has always been automatically added on pkg@5. There is no way to stop it being added. That's why I posted this issue

Those debug statements I just logged are from a build run with no pkg assets key

From my initial post:

how am I configuring pkg? Just with cmd parameters in my package.json
What happened? pkg packaged up the macos version of iconv's module

@robertsLando
Copy link
Contributor

Sorry, lot of issues to answer everyday. BTW my PR prevents just the build to happen, I didn't see where in the code them are automatically added, When I have .node files pkg always tell me the warning 'Warning Cannot include addon %1 into executable' that you ref in your previous message

@tonywoode
Copy link
Author

no problems! Note that both the native modules I use in my module are transitively linked from my require of xml-stream. Maybe your behaviour is different because your .node files could be from directly-required modules?

@robertsLando
Copy link
Contributor

Could be that, BTW could you try this?

In https://github.com/vercel/pkg/blob/main/lib/walker.ts#L887

inside the if statement could you simply return when the file is a dotNode? This may prevent your file to be added to the executable automatically

@tonywoode
Copy link
Author

yes that compiles successfully if I:

 if (store === STORE_BLOB) {
      if (isDotNODE(record.file)){
        return;
      }
      if (unlikelyJavascript(record.file) || isDotNODE(record.file)) {
        this.appendBlobOrContent({
...

strange thing though: the exe size (34,234KB) is still half the size of the exe compiled under pkg@4 (75,427KB). The binary definitely works - is that expected?

@robertsLando
Copy link
Contributor

robertsLando commented Oct 13, 2021

I think @jesec has reduce nodejs binary size by removing some locale stuff or else that I don't remember in latest releases so yes it's possible

BTW ok now we have found the last problem, unfortunally I'm not sure If it makes sense to use my flag or another one to specify when adding dot node files to binary and when not

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants