Skip to content

Conversation

@jaimecbernardo
Copy link

Hi,
nodejs-mobile is a fork of node used to run Node.js applications on Android and iOS.

It also allows building and running native modules. For this, it tries to use nodejs-mobile-gyp instead of node-gyp. The way it does this is setting the npm_config_node_gyp to point to nodejs-mobile-gyp. However, npm, since v7+, has started overwriting this environment variable before passing it to modules, such as node-pre-gyp.

This PR makes it so we check for the NODEJS_MOBILE_GYP environment variable to try to set node_gyp's location before trying the other methods. This variable is already used in some projects related to nodejs-mobile, so we decided to pick it:
https://github.com/nodejs-mobile/prebuild-for-nodejs-mobile/blob/44687f6cb4c0375e0f4ef063ce3321350f159241/bin.js#L367
https://github.com/nodejs-mobile/nodejs-mobile-react-native/blob/794f395152f25d1c5ccf9cc794eafa8ce58ad541/scripts/ios-build-native-modules.sh#L116
https://github.com/nodejs-mobile/nodejs-mobile-react-native/blob/794f395152f25d1c5ccf9cc794eafa8ce58ad541/android/build.gradle#L433
https://github.com/okhiroyuki/nodejs-mobile-cordova/blob/9d02c76700bba851241922d7632555546b4ee771/src/android/build.gradle#L243

The original motivation for this change was building sqlite3, which uses "@mapbox/node-pre-gyp", for nodejs-mobile. This is currently possible by adding this overrides field to a package.json of a project that's using sqlite3, but in theory it should help improve compatibility with other modules as well:

  "overrides": {
    "sqlite3": {
      "@mapbox/node-pre-gyp": "github:JaneaSystems/mapbox-node-pre-gyp#add-support-for-nodejs-mobile"
    }
  }

We'd consider this change would be safe for current uses, since in theory a process which sets an environment variable called NODEJS_MOBILE_GYP is likely to want to build for nodejs-mobile.

@staltz , FYI, since this information may help unblock building other modules in nodejs-mobile.

*/
function which_node_gyp() {
let node_gyp_bin;
if (process.env.NODEJS_MOBILE_GYP) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trying to understand... Does this enable me to set an environment variable that switches node-gyp to be any executable I choose? Would that be a vast attack vector?

Copy link
Author

Choose a reason for hiding this comment

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

Good question 🤔
I suppose in theory yes, but if you check the code for the whole function, you get this code right after my addition:

  if (process.env.npm_config_node_gyp) {
    try {
      node_gyp_bin = process.env.npm_config_node_gyp;
      if (existsSync(node_gyp_bin)) {
        return node_gyp_bin;
      }
    } catch (err) {
      // do nothing
    }
  }

This addition follows the pattern that exists already in the code, just with a different environment variable, which is what is used in Node.js mobile.

Copy link
Author

Choose a reason for hiding this comment

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

@jaimecbernardo jaimecbernardo force-pushed the add-support-for-nodejs-mobile branch from cd5f1b4 to 6dfd680 Compare April 24, 2025 12:32
@jaimecbernardo jaimecbernardo requested a review from a team as a code owner April 24, 2025 12:32
@jaimecbernardo
Copy link
Author

Please rebase now that #900 has been merged and our GitHub Actions are passing again.

@cclauss , just rebased, thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants