Skip to content

Commit

Permalink
Refactor PR 533 and Fix issue 653
Browse files Browse the repository at this point in the history 
- Moved logic regarding host selection to versioning where all user defined values from package.json are transformed into command options.
- Moved testing of feature from `run.test.js` to `versioning.test.js`.
- Added `development_host` option. Becomes default option for `publish` `unpublish` when present.
- Changed behavior when alternate hosts are defined. Now `production_host` acts as alias to host. Defining `staging_host` or `development_host` is enough to default `publish` and `unpublish` away from production.
- When a chain of commands that includes `publish` or `unpublish`, when host not specifically set via command line or environment variable, ALL commands in the chain default away from production.
- An invalid `s3_host` option does not result in error and is instead silently ignored.
- Change is backwards compatible with previously valid configurations.
  • Loading branch information
@ronilan
ronilan committed Jun 30, 2024
1 parent 09171e5 commit 4e407be
Show file tree
Hide file tree
Showing 6 changed files with 708 additions and 241 deletions.
6 changes: 0 additions & 6 deletions lib/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,6 @@ function run() {
return;
}

// set binary.host when appropriate. host determines the s3 target bucket.
const target = prog.setBinaryHostProperty(command.name);
if (target && ['install', 'publish', 'unpublish', 'info'].indexOf(command.name) >= 0) {
log.info('using binary.host: ' + prog.package_json.binary.host);
}

prog.commands[command.name](command.args, function(err) {
if (err) {
log.error(command.name + ' error');
Expand Down
66 changes: 1 addition & 65 deletions lib/node-pre-gyp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,8 @@ function Run({ package_json_path = './package.json', argv }) {
});

this.parseArgv(argv);

// this is set to true after the binary.host property was set to
// either staging_host or production_host.
this.binaryHostSet = false;
}

inherits(Run, EE);
exports.Run = Run;
const proto = Run.prototype;
Expand Down Expand Up @@ -205,67 +202,6 @@ proto.parseArgv = function parseOpts(argv) {
log.resume();
};

/**
* allow the binary.host property to be set at execution time.
*
* for this to take effect requires all the following to be true.
* - binary is a property in package.json
* - binary.host is falsey
* - binary.staging_host is not empty
* - binary.production_host is not empty
*
* if any of the previous checks fail then the function returns an empty string
* and makes no changes to package.json's binary property.
*
*
* if command is "publish" then the default is set to "binary.staging_host"
* if command is not "publish" the the default is set to "binary.production_host"
*
* if the command-line option '--s3_host' is set to "staging" or "production" then
* "binary.host" is set to the specified "staging_host" or "production_host". if
* '--s3_host' is any other value an exception is thrown.
*
* if '--s3_host' is not present then "binary.host" is set to the default as above.
*
* this strategy was chosen so that any command other than "publish" or "unpublish" uses "production"
* as the default without requiring any command-line options but that "publish" and "unpublish" require
* '--s3_host production_host' to be specified in order to *really* publish (or unpublish). publishing
* to staging can be done freely without worrying about disturbing any production releases.
*/
proto.setBinaryHostProperty = function(command) {
if (this.binaryHostSet) {
return this.package_json.binary.host;
}
const p = this.package_json;
// don't set anything if host is present. it must be left blank to trigger this.
if (!p || !p.binary || p.binary.host) {
return '';
}
// and both staging and production must be present. errors will be reported later.
if (!p.binary.staging_host || !p.binary.production_host) {
return '';
}
let target = 'production_host';
if (command === 'publish' || command === 'unpublish') {
target = 'staging_host';
}
// the environment variable has priority over the default or the command line. if
// either the env var or the command line option are invalid throw an error.
const npg_s3_host = process.env.node_pre_gyp_s3_host;
if (npg_s3_host === 'staging' || npg_s3_host === 'production') {
target = `${npg_s3_host}_host`;
} else if (this.opts['s3_host'] === 'staging' || this.opts['s3_host'] === 'production') {
target = `${this.opts['s3_host']}_host`;
} else if (this.opts['s3_host'] || npg_s3_host) {
throw new Error(`invalid s3_host ${this.opts['s3_host'] || npg_s3_host}`);
}

p.binary.host = p.binary[target];
this.binaryHostSet = true;

return p.binary.host;
};

/**
* Returns the usage instructions for node-pre-gyp.
*/
Expand Down
1 change: 0 additions & 1 deletion lib/pre-binding.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ exports.find = function(package_json_path, opts) {
throw new Error(package_json_path + 'does not exist');
}
const prog = new npg.Run({ package_json_path, argv: process.argv });
prog.setBinaryHostProperty();
const package_json = prog.package_json;

versioning.validate_config(package_json, opts);
Expand Down
83 changes: 60 additions & 23 deletions lib/util/versioning.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,6 @@ function get_runtime_abi(runtime, target_version) {
}
module.exports.get_runtime_abi = get_runtime_abi;

const required_parameters = [
'module_name',
'module_path',
'host'
];

function validate_config(package_json, opts) {
const msg = package_json.name + ' package.json is not node-pre-gyp ready:\n';
const missing = [];
Expand All @@ -207,25 +201,33 @@ function validate_config(package_json, opts) {
if (!package_json.binary) {
missing.push('binary');
}
const o = package_json.binary;
if (o) {
required_parameters.forEach((p) => {
if (!o[p] || typeof o[p] !== 'string') {
missing.push('binary.' + p);
}
});

if (package_json.binary) {
if (!package_json.binary.module_name) {
missing.push('binary.module_name');
}
if (!package_json.binary.module_path) {
missing.push('binary.module_path');
}
if (!package_json.binary.host && !package_json.binary.production_host) {
missing.push('binary.host');
}
}

if (missing.length >= 1) {
throw new Error(msg + 'package.json must declare these properties: \n' + missing.join('\n'));
}
if (o) {
// enforce https over http
const protocol = url.parse(o.host).protocol;
if (protocol === 'http:') {
throw new Error("'host' protocol (" + protocol + ") is invalid - only 'https:' is accepted");
}

if (package_json.binary) {
// for all possible host definitions - verify https usage
['host', 'production_host', 'staging_host', 'development_host'].filter((item) => package_json.binary[item]).forEach((item) => {
const protocol = url.parse(package_json.binary[item]).protocol;
if (protocol === 'http:') {
throw new Error(msg + "'" + item + "' protocol (" + protocol + ") is invalid - only 'https:' is accepted");
}
});
}

napi.validate_package_json(package_json, opts);
}

Expand Down Expand Up @@ -309,11 +311,46 @@ module.exports.evaluate = function(package_json, options, napi_build_version) {
region: package_json.binary.region,
s3ForcePathStyle: package_json.binary.s3ForcePathStyle || false
};
// support host mirror with npm config `--{module_name}_binary_host_mirror`
// e.g.: https://github.com/node-inspector/v8-profiler/blob/master/package.json#L25
// > npm install v8-profiler --profiler_binary_host_mirror=https://npm.taobao.org/mirrors/node-inspector/

// user can define a target host key to use (development_host, staging_host, production_host)
// by setting the name of the host (development, staging, production)
// into an environment variable or via a command line option.
// the environment variable has priority over the the command line.
let targetHost = process.env.node_pre_gyp_s3_host || options.s3_host;

// if value is not one of the allowed or the matching key is not found in package.json
// silently ignore the option
if (['production', 'staging', 'development'].indexOf(targetHost) === -1 || !package_json.binary[`${targetHost}_host`]) {
targetHost = '';
}

// the production host is as specified in 'host' key (default)
// unless there is none and alias production_host is specified (backwards compatibility)
// note: package.json is verified in validate_config to include at least one of the two.
let host = package_json.binary.host || package_json.binary.production_host;

// when a valid target is specified by user, the host is from that target (or 'host')
if (targetHost === 'staging') {
host = package_json.binary.staging_host;
} else if (targetHost === 'development') {
host = package_json.binary.development_host;
} else if (!targetHost && (package_json.binary.development_host || package_json.binary.staging_host)) {
// when host not specifically set via command line or environment variable
// but staging and/or development host are present in package.json
// for any command (or command chain) that includes publish or unpublish
// default to lower host (development, and if not preset, staging).
if (options.argv && options.argv.remain.some((item) => (item === 'publish' || item === 'unpublish'))) {
host = package_json.binary.development_host || package_json.binary.staging_host;
}
}

// support host mirror with npm config `--{module_name}_binary_host_mirror`
// e.g.: https://github.com/node-inspector/v8-profiler/blob/master/package.json#L25
// > npm install v8-profiler --profiler_binary_host_mirror=https://npm.taobao.org/mirrors/node-inspector/
const validModuleName = opts.module_name.replace('-', '_');
const host = process.env['npm_config_' + validModuleName + '_binary_host_mirror'] || package_json.binary.host;
// explicitly set mirror overrides everything set above
host = process.env['npm_config_' + validModuleName + '_binary_host_mirror'] || host;

opts.host = fix_slashes(eval_template(host, opts));
opts.module_path = eval_template(package_json.binary.module_path, opts);
// now we resolve the module_path to ensure it is absolute so that binding.gyp variables work predictably
Expand Down
141 changes: 4 additions & 137 deletions test/run.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ const package_json_template = {
}
};


const all_commands = ['build', 'clean', 'configure', 'info', 'install', 'package', 'publish', 'rebuild',
'reinstall', 'reveal', 'testbinary', 'testpackage', 'unpublish'];

/**
* before testing create a scratch directory to run tests in.
*/
Expand Down Expand Up @@ -67,82 +63,6 @@ test.onFinish(() => {
rimraf(scratch).then(() => undefined, () => undefined);
});

test('should set staging and production hosts', (t) => {
// make sure it's good when specifying host.
const mock_package_json = makePackageJson();

let { prog } = setupTest(dir, mock_package_json);
t.deepEqual(prog.package_json, mock_package_json);
t.equal(prog.binaryHostSet, false, 'binary host should not be flagged as set');

// test with no s3_host option
all_commands.forEach((cmd) => {
const mpj = clone(mock_package_json);
mpj.binary.host = '';
const opts = { argv: [cmd] };
({ prog } = setupTest(dir, mpj, opts));
mpj.binary.host = (cmd === 'publish' || cmd === 'unpublish') ? mpj.binary.staging_host : mpj.binary.production_host;
t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
});

// test with s3_host set to staging
all_commands.forEach((cmd) => {
const mpj = clone(mock_package_json);
mpj.binary.host = '';
const opts = { argv: [cmd, '--s3_host=staging'] };
({ prog } = setupTest(dir, mpj, opts));
mpj.binary.host = mpj.binary.staging_host;
t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
});

// test with s3_host set to production
all_commands.forEach((cmd) => {
const mpj = clone(mock_package_json);
mpj.binary.host = '';
const opts = { argv: [cmd, '--s3_host=production'] };
({ prog } = setupTest(dir, mpj, opts));
mpj.binary.host = mpj.binary.production_host;
t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
});

t.end();
});

test('should execute setBinaryHostProperty() properly', (t) => {
// it only --s3_host only takes effect if host is falsey.
const mock_package_json = makePackageJson({ binary: { host: '' } });

const opts = { argv: ['publish', '--s3_host=staging'] };

let { prog, binaryHost } = setupTest(dir, mock_package_json, opts);
t.equal(binaryHost, mock_package_json.binary.staging_host);

// set it again to verify that it returns the already set value
binaryHost = prog.setBinaryHostProperty('publish');
t.equal(binaryHost, mock_package_json.binary.staging_host);

// now do this again but expect an empty binary host value because
// staging_host is missing.
const mpj = clone(mock_package_json);
delete mpj.binary.staging_host;
({ prog, binaryHost } = setupTest(dir, mpj, opts));
t.equal(binaryHost, '');

// one more time but with an invalid value for s3_host
opts.argv = ['publish', '--s3_host=bad-news'];
try {
({ prog, binaryHost } = setupTest(dir, mock_package_json, opts));
t.fail('should throw with --s3_host=bad-news');
} catch (e) {
t.equal(e.message, 'invalid s3_host bad-news');
}

t.end();
});

test('verify that the --directory option works', (t) => {
const initial = process.cwd();

Expand Down Expand Up @@ -223,6 +143,10 @@ test('verify that a non-existent package.json fails', (t) => {
// test helpers.
//

// helper to clone mock package.json.
// // https://stackoverflow.com/questions/4459928/how-to-deep-clone-in-javascript
const clone = (obj) => JSON.parse(JSON.stringify(obj));

function makePackageJson(options = {}) {
const package_json = clone(package_json_template);
// override binary values if supplied
Expand All @@ -233,60 +157,3 @@ function makePackageJson(options = {}) {
}
return package_json;
}

// helper to write package.json to disk so Run() can be instantiated with it.
function setupTest(directory, package_json, opts) {
opts = opts || {};
let argv = ['node', 'program'];
if (opts.argv) {
argv = argv.concat(opts.argv);
}
const prev_dir = process.cwd();
if (!opts.noChdir) {
try {
fs.mkdirSync(directory);
} catch (e) {
if (e.code !== 'EEXIST') {
throw e;
}
}
process.chdir(directory);
}

try {
fs.writeFileSync('package.json', JSON.stringify(package_json));
const prog = new npg.Run({ package_json_path: './package.json', argv });
const binaryHost = prog.setBinaryHostProperty(prog.todo[0] && prog.todo[0].name);
return { prog, binaryHost };
} finally {
process.chdir(prev_dir);
}
}

// helper to clone mock package.json. it's overkill for existing tests
// but is future-proof.
// https://stackoverflow.com/questions/4459928/how-to-deep-clone-in-javascript
function clone(obj, hash = new WeakMap()) {
if (Object(obj) !== obj) return obj; // primitives
if (hash.has(obj)) return hash.get(obj); // cyclic reference
let result;

if (obj instanceof Set) {
result = new Set(obj); // treat set as a value
} else if (obj instanceof Map) {
result = new Map(Array.from(obj, ([key, val]) => [key, clone(val, hash)]));
} else if (obj instanceof Date) {
result = new Date(obj);
} else if (obj instanceof RegExp) {
result = new RegExp(obj.source, obj.flags);
} else if (obj.constructor) {
result = new obj.constructor();
} else {
result = Object.create(null);
}
hash.set(obj, result);
return Object.assign(result, ...Object.keys(obj).map((key) => {
return { [key]: clone(obj[key], hash) };
}));
}

Loading

0 comments on commit 4e407be

Please sign in to comment.