nodejs / gyp-next

A fork of the GYP build system for use in the Node.js projects
BSD 3-Clause "New" or "Revised" License
125 stars 69 forks source link

fix: support cross compiling for wasm with make generator #222

Closed toyobayashi closed 3 months ago

toyobayashi commented 5 months ago

I'm not sure if it's reasonable to do such changes, but it works!

toyobayashi commented 5 months ago

In fact the generated file path in makefile is still incorrect on Windows, users need to use a additional script to make it work

const path = require('path')
const fs = require('fs')

const buildDir = path.join(__dirname, '../build')

fs.readdirSync(buildDir)
  .filter(p => p.endsWith('.target.mk'))
  .map((p) => path.join(buildDir, p))
  .forEach((p) => {
    const content = fs.readFileSync(p, 'utf8').replace(/\\|\\\\/g, '/').replace(/\/(\r?\n)/g, '\\$1')
    fs.writeFileSync(p, content, 'utf8')
  })

I'm not familiar the gyp code base, it will be nice if you could help make node-gyp more friendly for building wasm🙏

toyobayashi commented 5 months ago

In fact the generated file path in makefile is still incorrect on Windows, users need to use a additional script to make it work

with changes in 882c1f9, the additional script is no longer required in my minimum test project

not sure if self.WriteLn or somewhere also need to apply the ReplaceSep

toyobayashi commented 5 months ago

@cclauss want to ask a question that may be not related to this PR, how can I get the absolute path of .gypi in itself?

The emcc --js-library flag can receive an absolute or relative path (relative to cwd) of a JavaScript library file to link against. If I set a relative path in common.gypi,

# ${projectRoot}/node_modules/emnapi/common.gypi
{
  'target_defaults': {
    'target_conditions': [
      ['_type=="executable"', {
        'conditions': [
          ['OS == "emscripten"', {
            'libraries': [
               # expect "${projectRoot}/node_modules/emnapi/dist/library_napi.js"
              '--js-library=./dist/library_napi.js',
            ],
          }],
        ],
      }],
    ],
  },
}

it doesn't work when running node-gyp build because the relative path is not handled by gyp, the cwd is ${projectRoot}/build so emscripten will find the path in ${projectRoot}/build/dist/library_napi.js, that is incorrect.

I tried defining a variable that invoke Node.js's require to locate the correct absolute path

'variables': {
  'emnapi_js_library%': '<!(node -p "(()=>{try{return require(\'emnapi\').js_library}catch(e){return \'\'}})()")'
},

But this is not reliable, The package found by require('emnapi') is not necessarily the package where common.gypi is currently located, also may enter the error path if emnapi is not used via npm.

So I wonder if I can set the absolute path depend on common.gypi itself. So that I can write something like

{
  'libraries': [
    # like CMAKE_CURRENT_SOURCE_DIR
    '--js-library=<(SOURCE_DIR_OF_COMMONGYPI)/dist/library_napi.js',
  ],
}

in emnapi's common.gypi file.

toyobayashi commented 5 months ago

how can I get the absolute path of .gypi in itself?

I have found solution for this question.

I opened another PR https://github.com/nodejs/node-gyp/pull/2974 in node-gyp repo to support rebuild and build commands on Windows. That and this one both are ready for review.

toyobayashi commented 4 months ago

Any other suggestions or change request?

toyobayashi commented 4 months ago

@cclauss Could you take a look? It's large improvement of development experience for building Node.js addon to wasm.

cclauss commented 4 months ago

@node-gyp Are there any wasm-knowledgeable folks who would be willing to review these proposed changes?

toyobayashi commented 3 months ago

Maybe @legendecas can review?

Tests: https://github.com/toyobayashi/emnapi-node-gyp-test/actions

cclauss commented 3 months ago

Python's https://docs.python.org/3/library/os.html#os.sep should tell you which way the slashes should lean on Windows vs. WSL.

toyobayashi commented 3 months ago

It seems that the replace_sep is to replace path seperators on Windows for WSL. Does this mean that the patch no longer supports Windows builds that are not on WSL?

On WSL the sys.platform is not win32, but Cygwin / MSYS is.

I don't think the make generator (pylib/gyp/generator/make.py) originally supports non-POSIX-like environment on Windows since it generates sh scripts and originally generates \, but make require / instead of \.

When building WebAssembly on Windows, the MSVC generator doesn't support Emscripten / wasi-sdk toolchain so makefile (-f make-linux) ➕ bash is the only choice.

This PR not only fix the Windows path problem, but also fix building static_library type of target in cross-compiling. If there is still any concern about breaking something, I'm ok to add gyp.common.CrossCompileRequested() into replace_sep.

toyobayashi commented 3 months ago

To be honest, Windows is nightmare, all these changes are the result of countless rebuild trials and errors. If you like, you can temporary revert any of these changes and then build the test project to see what will happen.

legendecas commented 3 months ago

Overall LGTM.

toyobayashi commented 3 months ago

@cclauss I have added unit test. I believe you have read diff wrongly just now somehow. The subprocess.Popen() was inside GetCrossCompilerPredefines so I defined defines and return it. The whole function looks like this now, could you please take another look?

https://github.com/nodejs/gyp-next/blob/4bf22c4b70a385a506d566430db59cf1c72a4602/pylib/gyp/common.py#L425-L472

All tests in my project passed. https://github.com/toyobayashi/emnapi-node-gyp-test/actions/runs/8510349552/job/23307768601