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.

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

Remove hardcoded node_modules in gulpfile. #8890

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Remove hardcoded node_modules in gulpfile. #8890

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
48 changes: 15 additions & 33 deletions gulpfile.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,13 +272,11 @@ gulp.task("clean", function (done) {

function cloc() {
var cmdLine;
var clocPath = path.join("node_modules", "cloc", "lib", "cloc");

//Run cloc on primary Source files only
var source = new Promise(function (resolve, reject) {
cmdLine =
"perl " +
clocPath +
"npx cloc" +
Copy link
Member

@kring kring Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess npx is ok, because it should run the version in node_modules/.bin if it exists, and only install the package if it doesn't already exist. This is important because installing the package from npm could end up installing the wrong version.

But it really shouldn't be necessary to use npx in any case. Previously we were using node_modules/cloc/lib/cloc, which is not guaranteed to work. The cloc package can be hoisted somewhere else for various reasons. This is not true of node_modules/.bin, though. The .bins should always be present, as long as the package has actually been installed. So node_modules/.bin/cloc should be equivalent to npx cloc except that the former won't ever install cloc or execute some random thing with that name from your path.

And if we do need to run node_modules/clock/lib/cloc directly (rather than the thing in .bin), you can just ask node to resolve it first. Something like this (not tested):

const clocPath = path.resolve(path.dirname(require.resolve("cloc/package.json")), "cloc", "lib", "cloc");

Copy link
Contributor

Choose a reason for hiding this comment

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

Kevin, the original problem that prompted this change was a literal node_modules/.bin/something in the gulpfile, which is only correct if you're not on Windows. This PR seems like a good opportunity to pick one consistent means of path construction and stick to it.

I never realized that npx will install missing packages -- kind of neat I guess? -- but if that's any kind of concern, there's a --no-install flag that you could use on all these invocations.

Copy link
Member

Choose a reason for hiding this comment

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

@thw0rted Windows stores the binaries in node_modules/.bin, too. One potential gotcha, though, is that, on Windows, the programs in .bin are actually cmd scripts with a .cmd extension. So doing child_process.exec("node_modules/.bin/cloc", ...) won't work because there's no file called cloc and possibly also because cloc.cmd is not an executable. Long story short, add shell: true as an option to the exec call and you'll be good to go.

Is all this better than just using npx? Especially with --no-install? I guess not. After reading up on it some more, npx is better suited to this than I remembered. Carry on. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to the path separator. node_modules/.bin/cloc does not work but I believe in my testing node_modules\\.bin\\.cloc worked. Anyway, I like how npx shows intent, handles nesting, global installs, etc. Ever since I discovered it a few years ago I've been trying to spread the word 😁

Copy link
Member

Choose a reason for hiding this comment

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

Weird, Windows is usually ok with forward slashes. 🤷

" --quiet --progress-rate=0" +
" Source/ --exclude-dir=Assets,ThirdParty,Workers --not-match-f=copyrightHeader.js";

Expand All @@ -297,8 +295,7 @@ function cloc() {
return source.then(function () {
return new Promise(function (resolve, reject) {
cmdLine =
"perl " +
clocPath +
"npx cloc" +
" --quiet --progress-rate=0" +
" Specs/ --exclude-dir=Data";
child_process.exec(cmdLine, function (error, stdout, stderr) {
Expand Down Expand Up @@ -341,30 +338,16 @@ gulp.task("combineRelease", gulp.series("build", combineRelease));

//Builds the documentation
function generateDocumentation() {
var envPathSeperator = os.platform() === "win32" ? ";" : ":";

return new Promise(function (resolve, reject) {
child_process.exec(
"jsdoc --configure Tools/jsdoc/conf.json",
{
env: {
PATH: process.env.PATH + envPathSeperator + "node_modules/.bin",
CESIUM_VERSION: version,
},
},
function (error, stdout, stderr) {
if (error) {
console.log(stderr);
return reject(error);
}
console.log(stdout);
var stream = gulp
.src("Documentation/Images/**")
.pipe(gulp.dest("Build/Documentation/Images"));
return streamToPromise(stream).then(resolve);
}
);
child_process.execSync("npx jsdoc --configure Tools/jsdoc/conf.json", {
stdio: "inherit",
env: Object.assign({}, process.env, { CESIUM_VERSION: version }),
});

var stream = gulp
.src("Documentation/Images/**")
.pipe(gulp.dest("Build/Documentation/Images"));

return streamToPromise(stream);
}
gulp.task("generateDocumentation", generateDocumentation);

Expand Down Expand Up @@ -1481,10 +1464,9 @@ function createCesiumJs() {

function createTypeScriptDefinitions() {
// Run jsdoc with tsd-jsdoc to generate an initial Cesium.d.ts file.
child_process.execSync(
"node_modules/.bin/jsdoc --configure Tools/jsdoc/ts-conf.json",
{ stdio: "inherit" }
);
child_process.execSync("npx jsdoc --configure Tools/jsdoc/ts-conf.json", {
stdio: "inherit",
});

var source = fs.readFileSync("Source/Cesium.d.ts").toString();

Expand Down Expand Up @@ -1608,7 +1590,7 @@ ${source}
fs.writeFileSync("Source/Cesium.d.ts", source);

// Use tsc to compile it and make sure it is valid
child_process.execSync("node_modules/.bin/tsc -p Tools/jsdoc/tsconfig.json", {
child_process.execSync("npx tsc -p Tools/jsdoc/tsconfig.json", {
stdio: "inherit",
});

Expand Down