🏗 Optimize root-level and task-level package installation while runni…

…ng `amp` tasks (#33452)

amp.js

@@ -18,14 +18,8 @@
 const {
   createTask,
   finalizeRunner,
-  initializeRunner,
 } = require('./build-system/tasks/amp-task-runner');
 
-/**
- * Initialize the task runner before any task is created.
- */
-initializeRunner();
-
 /**
  * All the AMP tasks. Keep this list alphabetized.
  *
@@ -78,7 +72,6 @@ createTask('storybook', 'storybook', 'storybook');
 createTask('sweep-experiments', 'sweepExperiments', 'sweep-experiments');
 createTask('test-report-upload', 'testReportUpload', 'test-report-upload');
 createTask('unit', 'unit', 'unit');
-createTask('update-packages', 'updatePackages', 'update-packages');
 createTask('validator', 'validator', 'validator');
 createTask('validator-cpp', 'validatorCpp', 'validator');
 createTask('validator-webui', 'validatorWebui', 'validator');

build-system/tasks/update-packages.js → build-system/common/update-packages.js

@@ -18,9 +18,12 @@
 const checkDependencies = require('check-dependencies');
 const del = require('del');
 const fs = require('fs-extra');
-const {cyan, green, yellow} = require('kleur/colors');
-const {execOrDie} = require('../common/exec');
-const {log, logLocalDev} = require('../common/logging');
+const path = require('path');
+const {cyan, red} = require('kleur/colors');
+const {execOrDie} = require('./exec');
+const {getOutput} = require('./process');
+const {isCiBuild} = require('./ci');
+const {log, logLocalDev} = require('./logging');
 
 /**
  * Writes the given contents to the patched file if updated
@@ -30,7 +33,7 @@ const {log, logLocalDev} = require('../common/logging');
 function writeIfUpdated(patchedName, file) {
   if (!fs.existsSync(patchedName) || fs.readFileSync(patchedName) != file) {
     fs.writeFileSync(patchedName, file);
-    logLocalDev(green('Patched'), cyan(patchedName));
+    logLocalDev('Patched', cyan(patchedName));
   }
 }
 
@@ -207,7 +210,7 @@ function removeRruleSourcemap() {
   const rruleMapFile = 'node_modules/rrule/dist/es5/rrule.js.map';
   if (fs.existsSync(rruleMapFile)) {
     del.sync(rruleMapFile);
-    logLocalDev(green('Deleted'), cyan(rruleMapFile));
+    logLocalDev('Deleted', cyan(rruleMapFile));
   }
 }
 
@@ -220,22 +223,11 @@ function updateDeps() {
     log: () => {},
     error: console.log,
   });
-  if (!results.depsWereOk) {
-    log(
-      yellow('WARNING:'),
-      'The packages in',
-      cyan('node_modules'),
-      'do not match',
-      cyan('package.json') + '.'
-    );
-    log('Running', cyan('npm install'), 'to update packages...');
-    execOrDie('npm install');
+  if (results.depsWereOk) {
+    logLocalDev('All packages in', cyan('node_modules'), 'are up to date.');
   } else {
-    log(
-      green('All packages in'),
-      cyan('node_modules'),
-      green('are up to date.')
-    );
+    log('Running', cyan('npm install') + '...');
+    execOrDie('npm install');
   }
 }
 
@@ -253,9 +245,34 @@ async function updatePackages() {
   removeRruleSourcemap();
 }
 
+/**
+ * Update packages in a given task directory. Some notes:
+ * - During CI, do a clean install.
+ * - During local development, do an incremental install if necessary.
+ * - Since install scripts can be async, `await` the process object.
+ * - Since script output is noisy, capture and print the stderr if needed.
+ *
+ * @param {string} dir
+ * @return {Promise<void>}
+ */
+async function updateSubpackages(dir) {
+  const results = checkDependencies.sync({packageDir: dir});
+  const relativeDir = path.relative(process.cwd(), dir);
+  if (results.depsWereOk) {
+    const nodeModulesDir = path.join(relativeDir, 'node_modules');
+    logLocalDev('All packages in', cyan(nodeModulesDir), 'are up to date.');
+  } else {
+    const installCmd = isCiBuild() ? 'npm ci' : 'npm install';
+    log('Running', cyan(installCmd), 'in', cyan(relativeDir) + '...');
+    const output = await getOutput(`${installCmd} --prefix ${dir}`);
+    if (output.status !== 0) {
+      log(red('ERROR:'), output.stderr);
+      throw new Error('Installation failed');
+    }
+  }
+}
+
 module.exports = {
   updatePackages,
+  updateSubpackages,
 };
-
-updatePackages.description =
-  'Updates npm packages if node_modules is out of date, and applies custom patches';

build-system/common/utils.js

@@ -18,17 +18,13 @@ const argv = require('minimist')(process.argv.slice(2));
 const experimentsConfig = require('../global-configs/experiments-config.json');
 const fs = require('fs-extra');
 const globby = require('globby');
-const path = require('path');
 const {clean} = require('../tasks/clean');
 const {doBuild} = require('../tasks/build');
 const {doDist} = require('../tasks/dist');
-const {getOutput} = require('./process');
 const {gitDiffNameOnlyMaster} = require('./git');
-const {green, cyan, red, yellow} = require('kleur/colors');
+const {green, cyan, yellow} = require('kleur/colors');
 const {log, logLocalDev} = require('./logging');
 
-const ROOT_DIR = path.resolve(__dirname, '../../');
-
 /**
  * Performs a clean build of the AMP runtime in testing mode.
  * Used by `amp e2e|integration|visual_diff`.
@@ -166,33 +162,12 @@ function usesFilesOrLocalChanges(taskName) {
   return validUsage;
 }
 
-/**
- * Runs 'npm ci' to install packages in a given directory. Some notes:
- * - Since install scripts can be async, we `await` the process object.
- * - Since script output is noisy, we capture and print the stderr if needed.
- *
- * @param {string} dir
- * @return {Promise<void>}
- */
-async function installPackages(dir) {
-  const relativeDir = path.relative(ROOT_DIR, dir);
-  log('Running', cyan('npm ci'), 'in', cyan(relativeDir) + '...');
-  const output = await getOutput(`npm ci --prefix ${dir}`);
-  if (output.status === 0) {
-    log('Done running', cyan('npm ci'), 'in', cyan(relativeDir) + '.');
-  } else {
-    log(red('ERROR:'), output.stderr);
-    throw new Error('Installation failed');
-  }
-}
-
 module.exports = {
   buildRuntime,
   getExperimentConfig,
   getValidExperiments,
   getFilesChanged,
   getFilesFromArgv,
   getFilesToCheck,
-  installPackages,
   usesFilesOrLocalChanges,
 };

build-system/pr-check/ci-job.js

@@ -25,7 +25,7 @@ const {determineBuildTargets} = require('./build-targets');
 const {isPullRequestBuild} = require('../common/ci');
 const {runNpmChecks} = require('./npm-checks');
 const {setLoggingPrefix} = require('../common/logging');
-const {updatePackages} = require('../tasks/update-packages');
+const {updatePackages} = require('../common/update-packages');
 
 /**
  * Helper used by all CI job scripts. Runs the PR / push build workflow.

build-system/tasks/amp-task-runner.js

@@ -21,11 +21,15 @@
 
 const argv = require('minimist')(process.argv.slice(2));
 const commander = require('commander');
+const fs = require('fs-extra');
 const path = require('path');
+const {
+  updatePackages,
+  updateSubpackages,
+} = require('../common/update-packages');
 const {cyan, red, green, magenta, yellow} = require('kleur/colors');
 const {isCiBuild} = require('../common/ci');
 const {log, logWithoutTimestamp} = require('../common/logging');
-const {updatePackages} = require('./update-packages');
 
 /**
  * Special-case constant that indicates if `amp --help` was invoked.
@@ -63,17 +67,31 @@ function startAtRepoRoot() {
 }
 
 /**
- * Runs an AMP task with logging and timing. Always start at the repo root.
+ * Updates task-specific subpackages if there are any.
+ * @param {string} taskSourceFilePath
+ * @return {Promise<void>}
+ */
+async function maybeUpdateSubpackages(taskSourceFilePath) {
+  const packageFile = path.join(taskSourceFilePath, 'package.json');
+  const hasSubpackages = await fs.pathExists(packageFile);
+  if (hasSubpackages) {
+    await updateSubpackages(taskSourceFilePath);
+  }
+}
+
+/**
+ * Runs an AMP task with logging and timing after installing its subpackages.
  * @param {string} taskName
+ * @param {string} taskSourceFilePath
  * @param {Function()} taskFunc
  * @return {Promise<void>}
  */
-async function runTask(taskName, taskFunc) {
-  startAtRepoRoot();
+async function runTask(taskName, taskSourceFilePath, taskFunc) {
   log('Using task file', magenta(path.join('amphtml', 'amp.js')));
   const start = Date.now();
   try {
     log(`Starting '${cyan(taskName)}'...`);
+    await maybeUpdateSubpackages(taskSourceFilePath);
     await taskFunc();
     log('Finished', `'${cyan(taskName)}'`, 'after', magenta(getTime(start)));
   } catch (err) {
@@ -86,7 +104,9 @@ async function runTask(taskName, taskFunc) {
 /**
  * Helper that creates the tasks in AMP's toolchain based on the invocation:
  * - For `amp --help`, load all task descriptions so a list can be printed.
- * - For other tasks, load the entry point, validate usage, and run the task.
+ * - For `amp <task> --help`, load and print just the task description + flags.
+ * - When a task is actually run, update root packages, load the entry point,
+ *   validate usage, update task-specific packages, and run the task.
  * @param {string} taskName
  * @param {string} taskFuncName
  * @param {string} taskSourceFileName
@@ -96,13 +116,19 @@ function createTask(taskName, taskFuncName, taskSourceFileName) {
   const isInvokedTask = argv._.includes(taskName); // `amp <task>`
   const isDefaultTask =
     argv._.length === 0 && taskName == 'default' && !isHelpTask; // `amp`
+  const isTaskLevelHelp =
+    (isInvokedTask || isDefaultTask) && argv.hasOwnProperty('help'); // `amp <task> --help`
 
   if (isHelpTask) {
     const taskFunc = require(taskSourceFilePath)[taskFuncName];
     const task = commander.command(cyan(taskName));
     task.description(taskFunc.description);
   }
   if (isInvokedTask || isDefaultTask) {
+    if (!isTaskLevelHelp && !isCiBuild()) {
+      startAtRepoRoot();
+      updatePackages();
+    }
     const taskFunc = require(taskSourceFilePath)[taskFuncName];
     const task = commander.command(taskName, {isDefault: isDefaultTask});
     task.description(green(taskFunc.description));
@@ -114,7 +140,7 @@ function createTask(taskName, taskFuncName, taskSourceFileName) {
     }
     task.action(async () => {
       validateUsage(task, taskName, taskFunc);
-      await runTask(taskName, taskFunc);
+      await runTask(taskName, taskSourceFilePath, taskFunc);
     });
   }
 }
@@ -139,17 +165,6 @@ function validateUsage(task, taskName, taskFunc) {
   }
 }
 
-/**
- * Initializes the task runner by running the package updater before any task
- * code can be loaded. As an optimization, we skip this during CI because it is
- * already done as a prereqisite step before every CI job.
- */
-function initializeRunner() {
-  if (!isCiBuild()) {
-    updatePackages();
-  }
-}
-
 /**
  * Finalizes the task runner by doing special-case setup for `amp --help`,
  * parsing the invoked command, and printing an error message if an unknown task
@@ -202,6 +217,5 @@ function printGulpDeprecationNotice(withTimestamps) {
 module.exports = {
   createTask,
   finalizeRunner,
-  initializeRunner,
   printGulpDeprecationNotice,
 };

build-system/tasks/coverage-map/index.js

@@ -18,7 +18,6 @@ const fs = require('fs').promises;
 const {buildNewServer} = require('../../server/typescript-compile');
 const {cyan} = require('kleur/colors');
 const {dist} = require('../dist');
-const {installPackages} = require('../../common/utils');
 const {log} = require('../../common/logging');
 const {startServer, stopServer} = require('../serve');
 
@@ -143,7 +142,6 @@ async function generateMap() {
  * @return {Promise<void>}
  */
 async function coverageMap() {
-  await installPackages(__dirname);
   await buildNewServer();
 
   if (!argv.nobuild) {

build-system/tasks/e2e/index.js

@@ -25,15 +25,11 @@ const glob = require('glob');
 const http = require('http');
 const Mocha = require('mocha');
 const path = require('path');
-const {
-  buildRuntime,
-  getFilesFromArgv,
-  installPackages,
-} = require('../../common/utils');
 const {
   createCtrlcHandler,
   exitCtrlcHandler,
 } = require('../../common/ctrlcHandler');
+const {buildRuntime, getFilesFromArgv} = require('../../common/utils');
 const {cyan} = require('kleur/colors');
 const {execOrDie} = require('../../common/exec');
 const {HOST, PORT, startServer, stopServer} = require('../serve');
@@ -55,9 +51,6 @@ const COV_OUTPUT_HTML = path.resolve(COV_OUTPUT_DIR, 'lcov-report/index.html');
  * @return {!Promise}
  */
 async function setUpTesting_() {
-  // install e2e-specific modules
-  await installPackages(__dirname);
-
   require('@babel/register')({caller: {name: 'test'}});
   const {describes} = require('./helper');
   describes.configure({

build-system/tasks/performance/index.js

@@ -22,7 +22,6 @@ const loadConfig = require('./load-config');
 const rewriteAnalyticsTags = require('./rewrite-analytics-tags');
 const rewriteScriptTags = require('./rewrite-script-tags');
 const runTests = require('./run-tests');
-const {installPackages} = require('../../common/utils');
 const {printReport} = require('./print-report');
 
 /**
@@ -34,7 +33,6 @@ async function performance() {
     resolver = resolverIn;
   });
 
-  await installPackages(__dirname);
   const config = new loadConfig();
   const urls = Object.keys(config.urlToHandlers);
   const urlsAndAdsUrls = urls.concat(config.adsUrls || []);

build-system/tasks/release/index.js

@@ -112,7 +112,7 @@ function logSeparator_() {
 }
 
 /**
- * Prepares output and temp directories, and ensures dependencies are installed.
+ * Prepares output and temp directories.
  *
  * @param {string} outputDir full directory path to emplace artifacts in.
  * @param {string} tempDir full directory path to temporary working directory.
@@ -121,9 +121,6 @@ async function prepareEnvironment_(outputDir, tempDir) {
   await fs.emptyDir(outputDir);
   await fs.emptyDir(tempDir);
   logSeparator_();
-
-  execOrDie('amp update-packages');
-  logSeparator_();
 }
 
 /**

build-system/tasks/storybook/index.js

@@ -22,7 +22,6 @@ const {cyan} = require('kleur/colors');
 const {defaultTask: runAmpDevBuildServer} = require('../default-task');
 const {exec, execScriptAsync} = require('../../common/exec');
 const {getBaseUrl} = require('../pr-deploy-bot-utils');
-const {installPackages} = require('../../common/utils');
 const {isCiBuild} = require('../../common/ci');
 const {isPullRequestBuild} = require('../../common/ci');
 const {log} = require('../../common/logging');
@@ -107,7 +106,6 @@ async function storybook() {
   if (!build && envs.includes('amp')) {
     await runAmpDevBuildServer();
   }
-  await installPackages(__dirname);
   if (!build) {
     createCtrlcHandler('storybook');
   }