Skip to content

Add support for nodejs-mobile #704

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.

Sign up for GitHub

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

Open
jaimecbernardo wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for nodejs-mobile #704

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions lib/util/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,25 @@ const cp = require('child_process');

// try to build up the complete path to node-gyp
/* priority:
- node-gyp on ENV:NODEJS_MOBILE_GYP
- node-gyp on ENV:npm_config_node_gyp (https://github.com/npm/npm/pull/4887)
- node-gyp on NODE_PATH
- node-gyp inside npm on NODE_PATH (ignore on iojs)
- node-gyp inside npm beside node exe
*/
function which_node_gyp() {
let node_gyp_bin;
if (process.env.NODEJS_MOBILE_GYP) {
Copy link
Collaborator

@cclauss cclauss Apr 23, 2025

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

@jaimecbernardo jaimecbernardo Apr 23, 2025

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

@jaimecbernardo jaimecbernardo Apr 23, 2025

Choose a reason for hiding this comment

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

Here's a direct link that opens the diff showing the code next to it opened as well: https://github.com/mapbox/node-pre-gyp/pull/704/files/cd5f1b4c0ca0b2b14a7d650441c3a9bff4150a6e#diff-b5f34a62c1cecd1c9964561345faa126b26e2bd642a1f67ee6bbe6a9931bb972R32-R41

// nodejs-mobile was using npm_config_node_gyp, but that's currently being overwritten by npm 7+
try {
node_gyp_bin = process.env.NODEJS_MOBILE_GYP;
if (existsSync(node_gyp_bin)) {
return node_gyp_bin;
}
} catch (err) {
// do nothing
}
}
if (process.env.npm_config_node_gyp) {
try {
node_gyp_bin = process.env.npm_config_node_gyp;
Expand Down