Skip to content
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.

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

feat: add support for yarn installations #560

Merged
merged 28 commits into from
Nov 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a68825d
feat: add support for yarn installations
SimenB Apr 15, 2018
6e901eb
chore: install yarn on Travis CI
SimenB Apr 16, 2018
2714b8a
PR feedback
SimenB Oct 23, 2018
126ce60
explicitly add yarn to `$PATH`
SimenB Oct 23, 2018
6a7013e
fix spawning of yarn process
SimenB Oct 24, 2018
8904db1
add tests
SimenB Oct 24, 2018
c5dbdaa
skip failing tests
SimenB Oct 24, 2018
9ced83d
Update lib/package-manager/test.js
targos Oct 24, 2018
5cb839f
Revert "skip failing tests"
SimenB Oct 24, 2018
190877f
remove invalid test for yarn
SimenB Oct 24, 2018
938fec5
install yarn from npm
SimenB Oct 24, 2018
9352ad6
extract package manager finder to separate module
SimenB Oct 24, 2018
84c73cf
don't install yarn in .travis.yml
SimenB Oct 24, 2018
3becd4d
name function [skip ci]
SimenB Oct 24, 2018
b6f99c1
fix find when bin is not in PATH
targos Oct 24, 2018
61f3b3d
run custom test script and try to add yarn to path
SimenB Oct 24, 2018
fb3ea5a
remove custom test script
SimenB Oct 24, 2018
ede6a95
fix tests
SimenB Oct 24, 2018
99df018
remove uknown flag from yarn call
SimenB Oct 24, 2018
4e4cfa6
stringify buffers in test
SimenB Oct 25, 2018
6829ba8
tweak assertion
SimenB Oct 25, 2018
013b093
chore: rename file
SimenB Nov 18, 2018
8f7795b
rename exported function
SimenB Nov 21, 2018
261d246
simplify calling pkg manager install and test
SimenB Nov 21, 2018
9bb22dc
cache package manager binary lookup
SimenB Nov 21, 2018
792e673
fix failing test
SimenB Nov 21, 2018
669492e
PR feedback
SimenB Nov 21, 2018
04964a3
lint
SimenB Nov 21, 2018
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
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ This is for adding a module to be included in the default `citgm-all` runs.
* Module source code must be on Github.
* Published versions must include a tag on Github
* The test process must be executable with only the commands
`npm install && npm test` using the tarball downloaded from the Github tag
`npm install && npm test` or (`yarn install && yarn test`) using the tarball downloaded from the Github tag
mentioned above
* The tests pass on supported major release lines
* The maintainers of the module remain responsive when there are problems
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ Options:
--includeTags tag1 tag2 Only test modules from the lookup that contain a matching tag field
--excludeTags tag1 tag2 Specify which tags to skip from the lookup (takes priority over includeTags)
Module names are automatically added as tags.
-y, --yarn Install and test the project using yarn instead of npm
```

When using a JSON config file, the properties need to be the same as the
Expand Down Expand Up @@ -140,6 +141,7 @@ For syntax, see [lookup.json](./lib/lookup.json), the available attributes are:
"maintainers": ["user1", "user2"] - List of module maintainers to be contacted with issues
"tags": ["tag1", "tag2"] Specify which tags apply to the module
"ignoreGitHead": Ignore the gitHead field if it exists and fallback to using github tags
"yarn": Install and test the project using yarn instead of npm
```

If you want to pass options to npm, eg `--registry`, you can usually define an
Expand Down
1 change: 1 addition & 0 deletions bin/citgm-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const options = {
timeoutLength: app.timeout,
tmpDir: app.tmpDir,
customTest: app.customTest,
yarn: app.yarn,
includeTags: app.includeTags || [],
excludeTags: app.excludeTags || []
};
Expand Down
3 changes: 2 additions & 1 deletion bin/citgm.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ const options = {
timeoutLength: app.timeout,
sha: app.sha,
tmpDir: app.tmpDir,
customTest: app.customTest
customTest: app.customTest,
yarn: app.yarn
};

if (!citgm.windows) {
Expand Down
42 changes: 28 additions & 14 deletions lib/citgm.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ let which = require('which'); // Mocked in tests
const grabModuleData = require('./grab-module-data');
const grabProject = require('./grab-project');
const lookup = require('./lookup');
const npm = require('./npm');
const packageManager = require('./package-manager');
const tempDirectory = require('./temp-directory');
const unpack = require('./unpack');

Expand All @@ -29,14 +29,28 @@ exports.windows = windows;
* 5. Output the results.
**/

function find(app, context, next) {
which(app, function(err, resolved) {
function findNode(context, next) {
which('node', function(err, resolved) {
if (err) {
next(Error(app + ' not found in path!'));
next(err);
return;
}
context.emit('data', 'verbose', context.module.name + ' using-' + app,
resolved);
context.emit('data', 'verbose', context.module.name + ' using-node',
resolved);
next(null, context);
});
}

function findPackageManagers(context, next) {
packageManager.getPackageManagers((err, res) => {
if (err) {
next(err);
return;
}

context.npmPath = res.npm;
context.yarnPath = res.yarn;

next(null, context);
});
}
Expand All @@ -46,13 +60,13 @@ function init(context, next) {
if (!windows) {
if (context.options.uid)
context.emit('data', 'verbose', context.module.name + ' using-uid',
context.options.uid);
context.options.uid);
if (context.options.gid)
context.emit('data', 'verbose', context.module.name + ' using-gid',
context.options.gid);
context.options.gid);
}
context.emit('data', 'silly', context.module.name + ' init-detail',
context.module);
context.module);
next(null, context); // Inject the context
}

Expand Down Expand Up @@ -98,15 +112,15 @@ Tester.prototype.run = function() {

async.waterfall([
init.bind(null, this),
find.bind(null, 'node'),
find.bind(null, 'npm'),
findNode,
findPackageManagers,
tempDirectory.create,
grabModuleData,
lookup,
grabProject,
unpack,
npm.install,
npm.test
packageManager.install,
packageManager.test
], (err) => {
if (!this.cleanexit) {
const payload = {
Expand Down Expand Up @@ -136,7 +150,7 @@ Tester.prototype.run = function() {
});
};

Tester.prototype.cleanup = function () {
Tester.prototype.cleanup = function() {
this.cleanexit = true;
const payload = {
name: this.module.name || this.module.raw,
Expand Down
6 changes: 6 additions & 0 deletions lib/common-args.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ module.exports = function commonArgs (app) {
description: 'Set timeout for npm install',
default: 1000 * 60 * 10
})
.option('yarn', {
alias: 'y',
type: 'boolean',
description: 'Install and test the project using yarn instead of npm',
default: false
})
.example('citgm-all --customTest /path/to/customTest.js',
'Runs a custom node test script instead of "npm test"');

Expand Down
3 changes: 3 additions & 0 deletions lib/lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ function resolve(context, next) {
if (rep.tags) {
context.module.tags = rep.tags;
}
if (rep.yarn) {
context.module.useYarn = true;
}
context.module.flaky = context.options.failFlaky ?
false : isMatch(rep.flaky);
context.module.expectFail = context.options.expectFail ?
Expand Down
8 changes: 0 additions & 8 deletions lib/npm/index.js

This file was deleted.

30 changes: 30 additions & 0 deletions lib/package-manager/get-executable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict';

const path = require('path');
const which = require('which');
const npmWhich = require('npm-which')(__dirname);

const windows = (process.platform === 'win32');

module.exports = function getExecutable(binaryName, next) {
if (binaryName === 'yarn') {
// Use `npm-which` for yarn to get the locally version
npmWhich(binaryName, next);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we can't use npmWhich for both npm and yarn?. If that's the case and it is pass through could we not remove this script altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

is there a reason we can't use npmWhich for both npm and yarn?

npm-which is just for binaries installed in node_modules. Not sure if it finds globally installed stuff?

Could potentially add npm as a dep as well, then it won't rely on any globals beyond node. Maybe not worth it?

If that's the case and it is pass through could we not remove this script altogether?

IDK about the windows thing below. Would that just work ™️? I don't have access to a windows machine to test

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it just how it is for now, this makes sense. I was more confused by the usage above


return;
}

which(binaryName, (err, packageManagerBin) => {
if (err) {
next(err);
return;
}

if (windows) {
packageManagerBin = path.join(path.dirname(packageManagerBin),
'node_modules', 'npm', 'bin', 'npm-cli.js');
}

next(null, packageManagerBin);
});
};
43 changes: 43 additions & 0 deletions lib/package-manager/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

SimenB marked this conversation as resolved.
Show resolved Hide resolved
const async = require('async');

const install = require('./install');
const test = require('./test');
const getExecutable = require('./get-executable');

function pkgInstall(context, next) {
if (context.options.yarn || context.module.useYarn) {
install('yarn', context, next);
} else {
install('npm', context, next);
}
}

function pkgTest(context, next) {
if (context.options.yarn || context.module.useYarn) {
test('yarn', context, next);
} else {
test('npm', context, next);
}
}

function getPackageManagers(next) {
async.parallel([
getExecutable.bind(null, 'npm'),
getExecutable.bind(null, 'yarn')
], (err, res) => {
if (err) {
next(err);
return;
}

next(null, { npm: res[0], yarn: res[1] });
});
}

module.exports = {
install: pkgInstall,
test: pkgTest,
getPackageManagers: getPackageManagers
};
30 changes: 21 additions & 9 deletions lib/npm/install.js → lib/package-manager/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,31 @@ const createOptions = require('../create-options');
const spawn = require('../spawn');
const timeout = require('../timeout');

function install(context, next) {
function install(packageManager, context, next) {
const options =
createOptions(
path.join(context.path, context.module.name), context);
let args = ['install'];
context.emit('data', 'info', context.module.name + ' npm:',
'npm install started');
context.emit('data', 'info', context.module.name + ' ' + packageManager +
SimenB marked this conversation as resolved.
Show resolved Hide resolved
':',
packageManager + ' install started');

context.emit('data', 'verbose', context.module.name + ' npm:',
context.emit('data', 'verbose', context.module.name + ' ' + packageManager +
':',
'Using temp directory: "' + options.env['npm_config_tmp'] + '"');

if (context.module.install) {
args = args.concat(context.module.install);
}

const proc = spawn('npm', args, options);
const packageManagerBin = packageManager === 'npm'
? context.npmPath
: context.yarnPath;

const binDirectory = path.dirname(packageManagerBin);
options.env.PATH = binDirectory + ':' + process.env.PATH;

const proc = spawn(packageManagerBin, args, options);
const finish = timeout(context, proc, next, 'Install');

proc.stderr.on('data', function (chunk) {
Expand All @@ -31,7 +40,8 @@ function install(context, next) {
chunk = stripAnsi(chunk.toString());
chunk = chunk.replace(/\r/g, '\n');
}
context.emit('data', 'warn', context.module.name + ' npm-install:',
context.emit('data', 'warn', context.module.name +
' ' + packageManager + '-install:',
chunk.toString());
});

Expand All @@ -41,7 +51,8 @@ function install(context, next) {
chunk = stripAnsi(chunk.toString());
chunk = chunk.replace(/\r/g, '\n');
}
context.emit('data', 'verbose', context.module.name + ' npm-install:',
context.emit('data', 'verbose', context.module.name +
' ' + packageManager + '-install:',
chunk.toString());
});

Expand All @@ -53,8 +64,9 @@ function install(context, next) {
if (code > 0) {
return finish(Error('Install Failed'));
}
context.emit('data', 'info', context.module.name + ' npm:',
'npm install successfully completed');
context.emit('data', 'info', context.module.name + ' ' +
packageManager + ':',
packageManager + ' install successfully completed');
return finish(null, context);
});
}
Expand Down
29 changes: 14 additions & 15 deletions lib/npm/test.js → lib/package-manager/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ const createOptions = require('../create-options');
const spawn = require('../spawn');
const timeout = require('../timeout');

const windows = (process.platform === 'win32');
let nodeBinName = 'node'; // Mocked in tests
const npmBinName = 'npm';

function authorName(author) {
if (typeof author === 'string') return author;
Expand All @@ -22,9 +20,9 @@ function authorName(author) {
return parts.join(' ');
}

function test(context, next) {
function test(packageManager, context, next) {
const wd = path.join(context.path, context.module.name);
context.emit('data', 'info', context.module.name + ' npm:',
context.emit('data', 'info', context.module.name + ' ' + packageManager + ':',
SimenB marked this conversation as resolved.
Show resolved Hide resolved
'test suite started');
readPackage(path.join(wd, 'package.json'), false, function(err, data) {
if (err) {
Expand All @@ -35,10 +33,10 @@ function test(context, next) {
data.scripts.test === undefined) {
if (data.author) {
context.emit('data', 'warn', context.module.name + ' notice',
'Please contact the module developer to request adding npm' +
' test support: ' + authorName(data.author));
'Please contact the module developer to request adding ' +
packageManager + '' + ' test support: ' + authorName(data.author));
}
next(new Error('Module does not support npm-test!'));
next(new Error('Module does not support ' + packageManager + '-test!'));
return;
}

Expand All @@ -49,17 +47,18 @@ function test(context, next) {
nodeBin = which(nodeBinName, {path: options.env.PATH
|| process.env.PATH});
}
let npmBin = which(npmBinName, {path: options.env.PATH
|| process.env.PATH});
if (windows) {
npmBin = path.join(path.dirname(npmBin), 'node_modules', 'npm', 'bin',
'npm-cli.js');
}

/* Run `npm test`, or `/path/to/customTest.js` if the customTest option
const packageManagerBin = packageManager === 'npm'
? context.npmPath
: context.yarnPath;

const binDirectory = path.dirname(packageManagerBin);
options.env.PATH = binDirectory + ':' + process.env.PATH;

/* Run `npm/yarn test`, or `/path/to/customTest.js` if the customTest option
was passed */
const args = context.options.customTest ?
[context.options.customTest] : [npmBin, 'test'];
[context.options.customTest] : [packageManagerBin, 'test'];

const proc = spawn(nodeBin, args, options);
const finish = timeout(context, proc, next, 'Test');
Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"mkdirp": "^0.5.1",
"normalize-git-url": "^3.0.2",
"npm-package-arg": "^6.1.0",
"npm-which": "^3.0.1",
"osenv": "^0.1.5",
"read-package-json": "^2.0.13",
"request": "^2.88.0",
Expand All @@ -55,7 +56,8 @@
"winston": "^2.4.4",
"xml-sanitizer": "^1.1.6",
"xmlbuilder": "^10.1.1",
"yargs": "^12.0.2"
"yargs": "^12.0.2",
"yarn": "^1.12.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

devDependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

dependency on purpose as mentioned. See #560 (comment)

},
"devDependencies": {
"eslint": "^5.7.0",
Expand Down
Loading