Use binary.remote_path in package.json so host can be overridden

[thom-nic]

So this is an oddity in how node-pre-gyp works and furthermore my use case: I'm trying to publish pre-built ARM binaries so I don't have to do native build on-target.

To accomplish this I can (mostly) just override flags during publish and install scripts. Publish looks like this:

export node_pre_gyp_accessKeyId=xxx
export node_pre_gyp_secretAccessKey=xyz
export node_pre_gyp_bucket=mybucket

$(npm bin)/node-pre-gyp package publish

which publishes to https://mybucket.s3.amazonaws.com/EmergingTechnologyAdvisors/node-serialport/releases/download/4.0.7/serialport-v4.0.7-node-v48-linux-arm.tar.gz - great!

However when doing npm install the node-pre-gyp mirror location is looked up differently - it's specified as opts.module_name + '_binary_host_mirror' (command-line option) or package_json.binary.host and then concatenates package_json.binary.remote_path.

Because serialport defines the path as part of the host in package.json, my install needs to look like this:

npm i --production --serialport_binary_host_mirror=https://mybucket.s3.amazonaws.com/EmergingTechnologyAdvisors/node-serialport/releases/download/4.0.7/

...which is less than ideal versus just passing --serialport_binary_host_mirror=https://mybucket.s3.amazonaws.com

We can accomplish that if you split the host line in package.json into host and remote_path. It's then possible to only override the host and keep a consistent path between publish and install steps.

Also because node-pre-gyp will interpolate those strings you can use a variable for the version portion of the path:

"binary: {
  "host": "https://github.com",
  "remote_path": "EmergingTechnologyAdvisors/node-serialport/releases/download/{version}"
}

this is similar to how sqlite3 defines their node-pre-gyp settings and I've confirmed what I'm trying to accomplish works for that module: https://github.com/mapbox/node-sqlite3/blob/master/package.json#L13-L14

I believe making this change for serialport should not have any adverse affect - future releases should look exactly the same otherwise.

[reconbot]

I'm for it. I'll see if I can get to it in the next few days (concentrating on the 5x releases). Happy to take a PR for both master and the 4x branches.

Can I ask some details about your arm build? We've not supported it because we haven't been able to build it on our CI (yet) and node-pre-gyp doesn't support detecting arm versions.

[thom-nic]

Great I already started a feature branch. I wonder if gyp can be used with a cross toolchain? If so it might not be so difficult to install a prebuilt cross ARM toolchain in CI. I've never built a native module myself so I wouldn't know where to begin...

[reconbot]

Well this totally broke node-pre-gyp-github's publishing.

[reconbot]

In this build https://travis-ci.org/EmergingTechnologyAdvisors/node-serialport/jobs/190853220

$ node-pre-gyp-github publish --release
/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/node-pre-gyp-github/index.js:56
		throw new Error('binary.host in package.json should begin with: "' + hostPrefix + '"');
		^
Error: binary.host in package.json should begin with: "https://github.com/EmergingTechnologyAdvisors/node-serialport/releases/download/"
    at NodePreGypGithub.init (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/node-pre-gyp-github/index.js:56:9)
    at NodePreGypGithub.publish (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/node-pre-gyp-github/index.js:137:7)
    at Command.<anonymous> (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/node-pre-gyp-github/bin/node-pre-gyp-github.js:16:5)
    at Command.listener (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/commander/index.js:301:8)
    at emitTwo (events.js:87:13)
    at Command.emit (events.js:172:7)
    at Command.parseArgs (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/commander/index.js:615:12)
    at Command.parse (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/commander/index.js:458:21)
    at Object.<anonymous> (/home/travis/build/EmergingTechnologyAdvisors/node-serialport/node_modules/node-pre-gyp-github/bin/node-pre-gyp-github.js:28:9)
    at Module._compile (module.js:409:26)

I think we should open an issue on node-pre-gyp-github.

[thom-nic]

Interesting - as soon as I read the docs it's clear why this is a problem - it wants to to use remote_path to auto-tag the release. This also suggests it may be difficult for node-pre-gyp-github to accommodate my use case.

If we try to work the way node-pre-gyp-github wants, the binary section of package.json should look like this:

binary: {
  host: "https://github.com/EmergingTechnologyAdvisors/node-serialport/releases/download/",
  remote_path: "{version}"
}

which, while it's a little annoying for me, is still an improvement....

So ideally I would have this:

npm i --production --no-optional \
  --node_sqlite3_binary_host_mirror=$node_pre_gyp_mirror \
  --serialport_binary_host_mirror=$node_pre_gyp_mirror \
  --bcrypt_lib_binary_host_mirror=$node_pre_gyp_mirror

(actually ideal would be specifying a single switch that node-pre-gyp used for all mirrors, instead of one for each module. But sadly node-pre-gyp doesn't support that!)

Instead I'll have to do this:

npm i --production --no-optional \
  --node_sqlite3_binary_host_mirror=$node_pre_gyp_mirror \
  --serialport_binary_host_mirror=$node_pre_gyp_mirror/EmergingTechnologyAdvisors/node-serialport/releases/download/ \
  --bcrypt_lib_binary_host_mirror=$node_pre_gyp_mirror

which isn't ideal but at least I won't have to hard-code the version and worry about whether it's in sync with package.json & shrinkwrap files.

I'll create new PRs, I noticed you didn't revert the PR for the 4x branch but you'll run into the same issue if you try to publish a new v4x release I think.

[thom-nic]

FYI this is on hold until we work out bchr02/node-pre-gyp-github#18 - after that gets resolved I'll fix this.

[reconbot]

Any progress on this?

[reconbot]

we switched to prebuild