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

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

Merged
merged 27 commits into from Apr 2, 2024

Conversation

toyobayashi
Copy link
Contributor

Co-authored-by: Christian Clauss <cclauss@me.com>
@toyobayashi
Copy link
Contributor Author

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
Copy link
Contributor Author

toyobayashi commented Jan 23, 2024

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
Copy link
Contributor Author

toyobayashi commented Jan 25, 2024

@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
Copy link
Contributor Author

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

I have found solution for this question.

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

@toyobayashi
Copy link
Contributor Author

Any other suggestions or change request?

@toyobayashi
Copy link
Contributor Author

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
Copy link
Member

Overall LGTM.

@cclauss cclauss requested a review from legendecas April 1, 2024 13:06
pylib/gyp/common.py Outdated Show resolved Hide resolved
pylib/gyp/common.py Outdated Show resolved Hide resolved
pylib/gyp/common.py Outdated Show resolved Hide resolved
pylib/gyp/common.py Outdated Show resolved Hide resolved
pylib/gyp/common.py Outdated Show resolved Hide resolved
@toyobayashi toyobayashi requested a review from cclauss April 1, 2024 13:59
pylib/gyp/common.py Outdated Show resolved Hide resolved
@toyobayashi
Copy link
Contributor Author

@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?

def GetCrossCompilerPredefines(): # -> dict
CC = os.environ.get("CC_target") or os.environ.get("CC")
CFLAGS = os.environ.get("CFLAGS")
CXX = os.environ.get("CXX_target") or os.environ.get("CXX")
CXXFLAGS = os.environ.get("CXXFLAGS")
cmd = []
defines = {}
if CC:
cmd += CC.split(" ")
if CFLAGS:
cmd += CFLAGS.split(" ")
elif CXX:
cmd += CXX.split(" ")
if CXXFLAGS:
cmd += CXXFLAGS.split(" ")
else:
return defines
if sys.platform == "win32":
fd, input = tempfile.mkstemp(suffix=".c")
try:
os.close(fd)
out = subprocess.Popen(
[*cmd, "-dM", "-E", "-x", "c", input],
shell=True,
stdout=subprocess.PIPE, stderr=subprocess.STDOUT
)
stdout = out.communicate()[0]
finally:
os.unlink(input)
else:
input = "/dev/null"
out = subprocess.Popen(
[*cmd, "-dM", "-E", "-x", "c", input],
shell=False,
stdout=subprocess.PIPE, stderr=subprocess.STDOUT
)
stdout = out.communicate()[0]
lines = stdout.decode("utf-8").replace("\r\n", "\n").split("\n")
for line in lines:
if not line:
continue
define_directive, key, *value = line.split(" ")
assert define_directive == "#define"
defines[key] = " ".join(value)
return defines

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

@toyobayashi toyobayashi requested a review from cclauss April 1, 2024 16:10
pylib/gyp/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Sorry that I misread before.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice work! Tests and all.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thanks!

@cclauss cclauss merged commit de0e1c9 into nodejs:main Apr 2, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants