From b1d05369e51387eabf6305522698110e35794881 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Dec 2019 16:25:22 -0800 Subject: [PATCH 1/4] add a test for index file --- test/index.js | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 test/index.js diff --git a/test/index.js b/test/index.js new file mode 100644 index 0000000..b5d9d79 --- /dev/null +++ b/test/index.js @@ -0,0 +1,9 @@ +const t = require('tap') +const index = require('../') +t.match(index, { + rm: Function, + link: Function, + linkIfExists: Function, + mkdir: Function, + binLink: Function +}, 'exports all the functions') From 16f9d41b16789d3d1a700426d8ab451562367f6d Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Dec 2019 16:25:34 -0800 Subject: [PATCH 2/4] add cmd-shim dep --- package-lock.json | 9 +++++++++ package.json | 5 +++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 00cd3f3..4dd1965 100644 --- a/package-lock.json +++ b/package-lock.json @@ -370,6 +370,15 @@ } } }, + "cmd-shim": { + "version": "3.0.3", + "resolved": "https://registry.npmjs.org/cmd-shim/-/cmd-shim-3.0.3.tgz", + "integrity": "sha512-DtGg+0xiFhQIntSBRzL2fRQBnmtAVwXIDo4Qq46HPpObYquxMaZS4sb82U9nH91qJrlosC1wa9gwr0QyL/HypA==", + "requires": { + "graceful-fs": "^4.1.2", + "mkdirp": "~0.5.0" + } + }, "co": { "version": "4.6.0", "resolved": "https://registry.npmjs.org/co/-/co-4.6.0.tgz", diff --git a/package.json b/package.json index fc1b0a7..b3ac047 100644 --- a/package.json +++ b/package.json @@ -6,9 +6,9 @@ "scripts": { "prerelease": "npm t", "postrelease": "npm publish && git push --follow-tags", - "pretest": "standard", + "posttest": "standard", "release": "standard-version -s", - "test": "tap -J --nyc-arg=--all --coverage test/*.js test/**/*.js", + "test": "tap -J --nyc-arg=--all --coverage test", "update-coc": "weallbehave -o . && git add CODE_OF_CONDUCT.md && git commit -m 'docs(coc): updated CODE_OF_CONDUCT.md'", "update-contrib": "weallcontribute -o . && git add CONTRIBUTING.md && git commit -m 'docs(contributing): updated CONTRIBUTING.md'" }, @@ -30,6 +30,7 @@ "dependencies": { "aproba": "^1.1.2", "chownr": "^1.1.2", + "cmd-shim": "^3.0.3", "fs-vacuum": "^1.2.10", "graceful-fs": "^4.1.11", "iferr": "^0.1.5", From a929196c8f7968cc15a4997aa384ccd854d0ec73 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Dec 2019 16:26:16 -0800 Subject: [PATCH 3/4] feat: add option to gently create bin links/shims This adds the top level `binLink(from, to, opts, cb)` method. If `opts.gently` is a string, and `opts.clobberLinkGently` is set to true, and `opts.force` is not set to true, then it will only create the link or cmd shim if the current target either does not exist, or is a link/shim into the path specified by `opts.gently`. This will be used by the `bin-links` package to prevent top-level global packages from overwriting bins that are not related to the package being installed. Paired with @mikemimik --- index.js | 4 +- lib/bin-link.js | 96 +++++++++++++ lib/link.js | 54 ++++++- test/lib/bin-link.js | 246 ++++++++++++++++++++++++++++++++ test/lib/link-clobber-gently.js | 240 +++++++++++++++++++++++++++++++ test/lib/link.js | 7 + 6 files changed, 645 insertions(+), 2 deletions(-) create mode 100644 lib/bin-link.js create mode 100644 test/lib/bin-link.js create mode 100644 test/lib/link-clobber-gently.js diff --git a/index.js b/index.js index 9807ed9..4aeda1c 100644 --- a/index.js +++ b/index.js @@ -3,10 +3,12 @@ const rm = require('./lib/rm.js') const link = require('./lib/link.js') const mkdir = require('./lib/mkdir.js') +const binLink = require('./lib/bin-link.js') exports = module.exports = { rm: rm, link: link.link, linkIfExists: link.linkIfExists, - mkdir: mkdir + mkdir: mkdir, + binLink: binLink } diff --git a/lib/bin-link.js b/lib/bin-link.js new file mode 100644 index 0000000..104c5b6 --- /dev/null +++ b/lib/bin-link.js @@ -0,0 +1,96 @@ +'use strict' +// calls linkIfExists on unix, or cmdShimIfExists on Windows +// reads the cmd shim to ensure it's where we need it to be in the case of +// top level global packages + +const readCmdShim = require('read-cmd-shim') +const cmdShim = require('cmd-shim') +const {linkIfExists} = require('./link.js') + +const binLink = (from, to, opts, cb) => { + // just for testing + const platform = opts._FAKE_PLATFORM_ || process.platform + if (platform !== 'win32') { + return linkIfExists(from, to, opts, cb) + } + + if (!opts.clobberLinkGently || + opts.force === true || + !opts.gently || + typeof opts.gently !== 'string') { + // easy, just go ahead and delete anything in the way + return cmdShim.ifExists(from, to, cb) + } + + // read all three shim targets + // if any exist, and are not a shim to our gently folder, then + // exit with a simulated EEXIST error. + + const shimFiles = [ + to, + to + '.cmd', + to + '.ps1' + ] + + // call this once we've checked all three, if we're good + const done = () => cmdShim.ifExists(from, to, cb) + const then = times(3, done, cb) + shimFiles.forEach(to => isClobberable(from, to, opts, then)) +} + +const times = (n, ok, cb) => { + let errState = null + return er => { + if (!errState) { + if (er) { + cb(errState = er) + } else if (--n === 0) { + ok() + } + } + } +} + +const isClobberable = (from, to, opts, cb) => { + readCmdShim(to, (er, target) => { + // either going to get an error, or the target of where this + // cmd shim points. + // shim, not in opts.gently: simulate EEXIST + // not a shim: simulate EEXIST + // ENOENT: fine, move forward + // shim in opts.gently: fine + if (er) { + switch (er.code) { + case 'ENOENT': + // totally fine, nothing there to clobber + return cb() + case 'ENOTASHIM': + // something is there, and it's not one of ours + return cb(simulateEEXIST(from, to)) + default: + // would probably fail this way later anyway + // can't read the file, likely can't write it either + return cb(er) + } + } + // no error, check the target + if (target.indexOf(opts.gently) !== 0) { + return cb(simulateEEXIST(from, to)) + } + // ok! it's one of ours. + return cb() + }) +} + +const simulateEEXIST = (from, to) => { + // simulate the EEXIST we'd get from fs.symlink to the file + const err = new Error('EEXIST: file already exists, cmd shim \'' + + from + '\' -> \'' + to + '\'') + + err.code = 'EEXIST' + err.path = from + err.dest = to + return err +} + +module.exports = binLink diff --git a/lib/link.js b/lib/link.js index 4623e7e..7cdfef4 100644 --- a/lib/link.js +++ b/lib/link.js @@ -14,15 +14,22 @@ exports = module.exports = { } function linkIfExists (from, to, opts, cb) { + opts.currentIsLink = false + opts.currentExists = false fs.stat(from, function (er) { if (er) return cb() fs.readlink(to, function (er, fromOnDisk) { + if (!er || er.code !== 'ENOENT') { + opts.currentExists = true + } // if the link already exists and matches what we would do, // we don't need to do anything if (!er) { + opts.currentIsLink = true var toDir = path.dirname(to) var absoluteFrom = path.resolve(toDir, from) var absoluteFromOnDisk = path.resolve(toDir, fromOnDisk) + opts.currentTarget = absoluteFromOnDisk if (absoluteFrom === absoluteFromOnDisk) return cb() } link(from, to, opts, cb) @@ -58,7 +65,7 @@ function link (from, to, opts, cb) { const tasks = [ [ensureFromIsNotSource, absTarget, to], [fs, 'stat', absTarget], - [rm, to, opts], + [clobberLinkGently, from, to, opts], [mkdir, path.dirname(to)], [fs, 'symlink', target, to, 'junction'] ] @@ -72,3 +79,48 @@ function link (from, to, opts, cb) { }) } } + +exports._clobberLinkGently = clobberLinkGently +function clobberLinkGently (from, to, opts, cb) { + if (opts.currentExists === false) { + // nothing to clobber! + opts.log.silly('gently link', 'link does not already exist', { + link: to, + target: from + }) + return cb() + } + + if (!opts.clobberLinkGently || + opts.force === true || + !opts.gently || + typeof opts.gently !== 'string') { + opts.log.silly('gently link', 'deleting existing link forcefully', { + link: to, + target: from, + force: opts.force, + gently: opts.gently, + clobberLinkGently: opts.clobberLinkGently + }) + return rm(to, opts, cb) + } + + if (!opts.currentIsLink) { + opts.log.verbose('gently link', 'cannot remove, not a link', to) + // don't delete. it'll fail with EEXIST when it tries to symlink. + return cb() + } + + if (opts.currentTarget.indexOf(opts.gently) === 0) { + opts.log.silly('gently link', 'delete existing link', to) + return rm(to, opts, cb) + } else { + opts.log.verbose('gently link', 'refusing to delete existing link', { + link: to, + currentTarget: opts.currentTarget, + newTarget: from, + gently: opts.gently + }) + return cb() + } +} diff --git a/test/lib/bin-link.js b/test/lib/bin-link.js new file mode 100644 index 0000000..433a711 --- /dev/null +++ b/test/lib/bin-link.js @@ -0,0 +1,246 @@ +const t = require('tap') + +const requireInject = require('require-inject') + +const linkIfExistsCalled = [] +const cmdShimIfExistsCalled = [] +const enoent = Object.assign(new Error('not found'), { code: 'ENOENT' }) +const weirdError = Object.assign(new Error('so weird'), { code: 'EWEIRD' }) +const notashim = Object.assign(new Error('not a shim'), { code: 'ENOTASHIM' }) + +const readCmdShimResults = { + '/not-found': [enoent], + '/not-found.cmd': [enoent], + '/not-found.ps1': [enoent], + '/one-in-pkg': [null, '/path/to/package'], + '/one-in-pkg.cmd': [enoent], + '/one-in-pkg.ps1': [enoent], + '/two-in-pkg': [null, '/path/to/package'], + '/two-in-pkg.cmd': [null, '/path/to/package'], + '/two-in-pkg.ps1': [enoent], + '/all-in-pkg': [null, '/path/to/package'], + '/all-in-pkg.cmd': [null, '/path/to/package'], + '/all-in-pkg.ps1': [null, '/path/to/package'], + '/weird-error': [null, '/path/to/package/bin/foo'], + '/weird-error.cmd': [weirdError], + '/weird-error.ps1': [null, '/path/to/package/what/why/is/it/different/idk'], + '/not/a/shim': [notashim], + '/not/a/shim.cmd': [notashim], + '/not/a/shim.ps1': [notashim], + '/other/package': [null, '/some/other/package'], + '/other/package.cmd': [null, '/some/other/package'], + '/other/package.ps1': [null, '/some/other/package'] +} + +const binLink = requireInject('../../lib/bin-link.js', { + '../../lib/link.js': { + linkIfExists: (from, to, opts, cb) => { + linkIfExistsCalled.push(from, to, opts) + process.nextTick(cb) + } + }, + 'cmd-shim': { + ifExists: (from, to, cb) => { + cmdShimIfExistsCalled.push(from, to) + process.nextTick(cb) + } + }, + 'read-cmd-shim': (path, cb) => { + if (readCmdShimResults[path]) { + return cb(readCmdShimResults[path][0], readCmdShimResults[path][1]) + } + + console.error('READ CMD SHIM', path) + return cb(Object.assign(new Error('not found'), { + code: 'ENOENT', + path + })) + } +}) + +t.test('on unix, just call linkIfExists', t => { + const opts = {} + if (process.platform === 'win32') { + opts._FAKE_PLATFORM_ = 'unix, I swear' + } + const from = { from: true } + const to = { to: true } + binLink(from, to, opts, () => { + t.strictSame(linkIfExistsCalled, [ from, to, opts ]) + linkIfExistsCalled.length = 0 + t.end() + }) +}) + +t.test('on windows, create the shim', t => { + const _FAKE_PLATFORM_ = process.platform === 'win32' ? null : 'win32' + + t.test('no clobberLinkGently', t => { + binLink('/path/to/package/bin/from', '/path/to', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: false + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + '/path/to' + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + }) + + t.test('force = true', t => { + binLink('/path/to/package/bin/from', '/path/to', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: true + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + '/path/to' + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + }) + + t.test('gently not a string', t => { + binLink('/path/to/package/bin/from', '/path/to', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + '/path/to' + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + }) + + t.test('gently is empty string', t => { + binLink('/path/to/package/bin/from', '/path/to', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '' + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + '/path/to' + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + }) + + t.test('shims not found', t => { + binLink('/path/to/package/bin/from', '/not-found', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '/path/to/package' + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + '/not-found' + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + }) + + t.test('shim(s) found, in pkg', t => { + const targets = ['/one-in-pkg', '/two-in-pkg', '/all-in-pkg'] + t.plan(targets.length) + targets.forEach(target => t.test(target, t => { + binLink('/path/to/package/bin/from', target, { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '/path/to/package' + }, (er) => { + if (er) { + throw er + } + t.strictSame(cmdShimIfExistsCalled, [ + '/path/to/package/bin/from', + target + ]) + cmdShimIfExistsCalled.length = 0 + t.end() + }) + })) + }) + + t.end() +}) + +t.test('windows, do not create the shim', t => { + const _FAKE_PLATFORM_ = process.platform === 'win32' ? null : 'win32' + + t.test('weird error', t => { + binLink('/path/to/package/bin/from', '/weird-error', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '/path/to/package' + }, (er) => { + t.equal(er, weirdError, 'got weird error') + t.equal(cmdShimIfExistsCalled.length, 0, 'did not try to create shim') + t.end() + }) + }) + + t.test('found, but not a shim', t => { + binLink('/path/to/package/bin/from', '/not/a/shim', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '/path/to/package' + }, (er) => { + t.match(er, { + code: 'EEXIST', + path: '/path/to/package/bin/from', + dest: '/not/a/shim' + }, 'got simulated EEXIST error') + t.equal(cmdShimIfExistsCalled.length, 0, 'did not try to create shim') + t.end() + }) + }) + + t.test('found, but not our package', t => { + binLink('/path/to/package/bin/from', '/other/package', { + _FAKE_PLATFORM_: _FAKE_PLATFORM_, + clobberLinkGently: true, + force: false, + gently: '/path/to/package' + }, (er) => { + t.match(er, { + code: 'EEXIST', + path: '/path/to/package/bin/from', + dest: '/other/package' + }, 'got simulated EEXIST error') + t.equal(cmdShimIfExistsCalled.length, 0, 'did not try to create shim') + t.end() + }) + }) + + t.end() +}) diff --git a/test/lib/link-clobber-gently.js b/test/lib/link-clobber-gently.js new file mode 100644 index 0000000..008e03f --- /dev/null +++ b/test/lib/link-clobber-gently.js @@ -0,0 +1,240 @@ +const t = require('tap') +const requireInject = require('require-inject') +const sillyLogs = [] +const verboseLogs = [] +const log = { + silly: function () { sillyLogs.push.apply(sillyLogs, arguments) }, + verbose: function () { verboseLogs.push.apply(verboseLogs, arguments) } +} + +const rmCalled = [] +const clobberLinkGently = requireInject('../../lib/link.js', { + '../../lib/rm.js': (path, opts, cb) => { + rmCalled.push(path) + return cb() + } +})._clobberLinkGently + +t.test('current link does not exist, allow it', t => { + clobberLinkGently('/a/b', '/c/d', { + currentExists: false, + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + t.strictSame(sillyLogs, [ + 'gently link', + 'link does not already exist', + { + link: '/c/d', + target: '/a/b' + } + ]) + sillyLogs.length = 0 + t.strictSame(rmCalled, [], 'rm not called') + t.end() + }) +}) + +t.test('use force', t => { + t.test('no clobber gently', t => { + clobberLinkGently('/a/b', '/c/d', { + currentExists: true, + clobberLinkGently: false, + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + t.strictSame(sillyLogs, [ + 'gently link', + 'deleting existing link forcefully', + { + link: '/c/d', + target: '/a/b', + force: undefined, + gently: undefined, + clobberLinkGently: false + } + ]) + t.strictSame(rmCalled, [ '/c/d' ]) + rmCalled.length = 0 + sillyLogs.length = 0 + t.end() + }) + }) + + t.test('--force', t => { + clobberLinkGently('/a/b', '/c/d', { + currentExists: true, + clobberLinkGently: true, + force: true, + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + t.strictSame(sillyLogs, [ + 'gently link', + 'deleting existing link forcefully', + { + link: '/c/d', + target: '/a/b', + force: true, + gently: undefined, + clobberLinkGently: true + } + ]) + t.strictSame(rmCalled, [ '/c/d' ]) + rmCalled.length = 0 + sillyLogs.length = 0 + t.end() + }) + }) + + t.test('falsey gently option', t => { + clobberLinkGently('/a/b', '/c/d', { + currentExists: true, + clobberLinkGently: true, + force: false, + gently: '', + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + t.strictSame(sillyLogs, [ + 'gently link', + 'deleting existing link forcefully', + { + link: '/c/d', + target: '/a/b', + force: false, + gently: '', + clobberLinkGently: true + } + ]) + sillyLogs.length = 0 + t.strictSame(rmCalled, [ '/c/d' ]) + rmCalled.length = 0 + t.end() + }) + }) + + t.test('no string gently path', t => { + clobberLinkGently('/a/b', '/c/d', { + currentExists: true, + clobberLinkGently: true, + force: false, + gently: true, + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + t.strictSame(sillyLogs, [ + 'gently link', + 'deleting existing link forcefully', + { + link: '/c/d', + target: '/a/b', + force: false, + gently: true, + clobberLinkGently: true + } + ]) + sillyLogs.length = 0 + t.strictSame(rmCalled, [ '/c/d' ]) + rmCalled.length = 0 + t.end() + }) + }) + t.end() +}) + +t.test('current is not a link, do not remove', t => { + clobberLinkGently('/path/to/package/bin/foo', '/a/b', { + currentExists: true, + currentIsLink: false, + clobberLinkGently: true, + force: false, + gently: '/path/to/package', + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [ + 'gently link', + 'cannot remove, not a link', + '/a/b' + ], 'expected verbose logs') + verboseLogs.length = 0 + t.strictSame(sillyLogs, [], 'no sillylogs') + t.strictEqual(rmCalled.length, 0, 'rm not called') + t.strictSame(rmCalled, [], 'rm not called') + t.end() + }) +}) + +t.test('current is link to another place', t => { + clobberLinkGently('/path/to/package/bin/foo', '/a/b', { + currentExists: true, + currentIsLink: true, + currentTarget: '/some/other/place', + clobberLinkGently: true, + force: false, + gently: '/path/to/package', + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [ + 'gently link', + 'refusing to delete existing link', + { + link: '/a/b', + currentTarget: '/some/other/place', + newTarget: '/path/to/package/bin/foo', + gently: '/path/to/package' + } + ], 'expected verbose logs') + verboseLogs.length = 0 + t.strictSame(sillyLogs, [], 'no sillylogs') + t.strictEqual(rmCalled.length, 0, 'rm not called') + t.strictSame(rmCalled, [], 'rm not called') + t.end() + }) +}) + +t.test('link is ours, delete it!', t => { + clobberLinkGently('/path/to/package/bin/foo', '/a/b', { + currentExists: true, + currentIsLink: true, + currentTarget: '/path/to/package/bin/bar', + clobberLinkGently: true, + force: false, + gently: '/path/to/package', + log: log + }, (er) => { + if (er) { + throw er + } + t.strictSame(verboseLogs, [], 'no verbose logs') + verboseLogs.length = 0 + t.strictSame(sillyLogs, [ + 'gently link', + 'delete existing link', + '/a/b' + ], 'sillylogs') + t.strictSame(rmCalled, ['/a/b'], 'rm called') + t.end() + }) +}) diff --git a/test/lib/link.js b/test/lib/link.js index fc7b943..c50fbf3 100644 --- a/test/lib/link.js +++ b/test/lib/link.js @@ -6,6 +6,11 @@ var dezalgo = require('dezalgo') var mkdirp = require('mkdirp') var path = require('path') +const log = { + silly: () => {}, + verbose: () => {} +} + test('gently/force', function (t) { t.plan(5) @@ -168,12 +173,14 @@ test('abs, noabs', function (t) { }) function linkOk (t, msg, opts) { + opts.log = log testLink(opts, function (err) { t.ifError(err, msg) }) } function linkNotOk (t, msg, opts) { + opts.log = log testLink(opts, function (err) { t.ok(err, msg) }) From e6fe75e8ce8bb9b7ecb59124ced90be55c84970c Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 10 Dec 2019 18:08:16 -0800 Subject: [PATCH 4/4] update travis config --- .travis.yml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index f713c5b..72e58f2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,8 @@ language: node_js -sudo: false + node_js: - - "8" - - "6" - - "4" + - node + - 12 + - 10 + - 8 + - 6