fix(postinstall): avoid potential infinite loop

Resolves appium/appium#17196 If the postinstall script (`install-npm.js`) is unable to load the install script, the script will now fail instead of infinitely retrying to load the module. The script was rather complicated, so I simplified it (as well as the build workflow). The `install-npm.js` script will spawn an `npm run build` if the project doesn't yet exist (which is only applicable in a dev environment). Fixed a couple broken things in `package.json` as well.
appium · Jul 20, 2022 · 7ae5830 · 7ae5830
1 parent 0227367
commit 7ae5830
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 79 deletions.
diff --git a/.github/workflows/node.js.yml b/.github/workflows/node.js.yml
@@ -19,8 +19,8 @@ jobs:
       uses: actions/setup-node@v1
       with:
         node-version: ${{ env.NODE_VERSION }}
-    - run: npm run clean
-    - run: npm run test
+    - run: npm install
+    - run: npm test
 
   e2e_tests:
     needs: [unit_tests]
@@ -32,7 +32,6 @@ jobs:
       uses: actions/setup-node@v1
       with:
         node-version: ${{ env.NODE_VERSION }}
-    - run: npm run clean
     - run: |
         export CHROME_VERSION=$(google-chrome --version | python -c "import sys, re; print(re.search(r'[0-9.]+', sys.stdin.read()).group(0))")
         echo "Version number of the installed Chrome browser: $CHROME_VERSION"
@@ -42,7 +41,7 @@ jobs:
           export CHROMEDRIVER_VERSION=$(grep -m 1 -n "$MAJOR_CHROME_VERSION" config/mapping.json | cut -d' ' -f4 | tr -d ',"')
           echo "Matching Chromedriver version: $CHROMEDRIVER_VERSION"
         fi
-        npm run chromedriver
+    - run: npm install
     - run: |
         export DISPLAY=:99
         export _FORCE_LOGS=1
@@ -59,6 +58,4 @@ jobs:
       uses: actions/setup-node@v1
       with:
         node-version: ${{ env.NODE_VERSION }}
-    - run: npm run clean
-    # Just to check that CD download is not broken on Windows machines
-    - run: npm run chromedriver --chromedriver_version=${{ env.WIN_CD_VERSION }}
+    - run: npm install --chromedriver_version=${{ env.WIN_CD_VERSION }}
diff --git a/install-npm.js b/install-npm.js
@@ -1,82 +1,66 @@
 #!/usr/bin/env node
-/* eslint-disable promise/prefer-await-to-callbacks */
-
-const fs = require('fs');
-const path = require('path');
-const log = require('fancy-log');
-const _ = require('lodash');
-
 
 /**
- * Because of the way npm lifecycle scripts work, on a local install, when the
- * code has not been tranpiled yet (i.e., the first time, or after the 'build'
- * directory has been deleted) the download **will** fail, and 'npm run chromedriver'
- * will need to be run.
+ * This is the `postinstall` script which:
+ * 1. Builds the project if it isn't yet built (only happens in a dev environment), and
+ * 2. Downloads Chromedriver (which can be disabled);
+ *
+ * Because `prepare` is run _after_ `postinstall`, we cannot just use `prepare` to build the project
+ * because this script depends on the project being built!
  */
 
-const BUILD_RETRIES = 200;
-const BUILD_RETRY_INTERVAL = 1000;
+const B = require('bluebird');
+
+// this is here because we're using async/await, and this must be set _before_ we use async/await,
+// given that bluebird is used elsewhere via `doInstall()`.
+B.config({
+  cancellation: true,
+});
+
+const {fs} = require('fs/promises');
+const path = require('path');
+const log = require('fancy-log');
+const _ = require('lodash');
+const {exec} = require('teen_process');
 
 const BUILD_PATH = path.join(__dirname, 'build', 'lib', 'install.js');
 
-function waitForDeps (cb) {
-  // see if we can import the necessary code
-  // try it a ridiculous (but finite) number of times
-  let i = 0;
-  function check () {
-    i++;
-    try {
-      require(BUILD_PATH);
-      cb();
-    } catch (err) {
-      if (err.message.includes(`Cannot find module '${BUILD_PATH}'`)) {
-        log.warn(`Project does not appear to be built yet. Please run 'npm run chromedriver' first.`);
-        return cb(new Error(`Could not install module: ${err.message}`));
-      }
-      log.warn(`Error trying to install Chromedriver binary. Waiting ${BUILD_RETRY_INTERVAL}ms and trying again: ${err.message}`);
-      if (i <= BUILD_RETRIES) {
-        setTimeout(check, BUILD_RETRY_INTERVAL);
-      } else {
-        cb(new Error(`Could not import installation module: ${err.message}`));
-      }
-    }
+async function main() {
+  // always build if not yet built.
+  // this should only happen in a working copy / dev environment.
+  try {
+    await fs.stat(BUILD_PATH);
+  } catch {
+    log.info('Project not yet built; building...');
+    const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm';
+    await exec(npmCommand, ['run', 'build'], {logger: log});
   }
-  check();
-}
 
-function main () {
   // check if we should skip install
-  if (!_.isEmpty(process.env.APPIUM_SKIP_CHROMEDRIVER_INSTALL) || !_.isEmpty(process.env.npm_config_chromedriver_skip_install)) {
-    log.warn(`'APPIUM_SKIP_CHROMEDRIVER_INSTALL' environment variable set, or '--chromedriver-skip-install' flag set.`);
-    log.warn(`Skipping Chromedriver installation. Android web/hybrid testing will not be possible`);
+  if (
+    !_.isEmpty(process.env.APPIUM_SKIP_CHROMEDRIVER_INSTALL) ||
+    !_.isEmpty(process.env.npm_config_chromedriver_skip_install)
+  ) {
+    log.warn(
+      `'APPIUM_SKIP_CHROMEDRIVER_INSTALL' environment variable is set; skipping Chromedriver installation.`,
+    );
+    log.warn(
+      `Android web/hybrid testing will not be possible without Chromedriver.`,
+    );
     return;
   }
 
-  // check if the code has been transpiled
-  waitForDeps(function wait (err) {
-    if (err) {
-      // this should only happen on local install (i.e., npm install in this directory)
-      log.warn(`Unable to import install script: ${err.message}`);
-      log.warn(`Re-run 'npm run chromedriver' manually.`);
-      return;
-    }
-    fs.stat(BUILD_PATH, function installScriptExists (err) {
-      if (err) {
-        // this should only happen on local install
-        log.warn(`NOTE: Run 'npx gulp transpile' before using`);
-        return;
-      }
-      require(BUILD_PATH).doInstall().catch(function installError (err) {
-        log.error(`Error installing Chromedriver: ${err.message}`);
-        log.error(err.stack ? err.stack : err);
-        log.error(`Downloading Chromedriver can be skipped by using the ` +
-                  `'--chromedriver-skip-install' flag or ` +
-                  `setting the 'APPIUM_SKIP_CHROMEDRIVER_INSTALL' environment ` +
-                  `variable.`);
-        process.exit(1);
-      });
-    });
-  });
+  try {
+    await require(BUILD_PATH).doInstall();
+  } catch (err) {
+    log.error(`Error installing Chromedriver: ${err.message}`);
+    log.error(err.stack ? err.stack : err);
+    log.error(
+      `Downloading Chromedriver can be skipped by setting the` +
+        `'APPIUM_SKIP_CHROMEDRIVER_INSTALL' environment variable.`,
+    );
+    process.exit(1);
+  }
 }
 
 if (require.main === module) {

diff --git a/package.json b/package.json
@@ -14,13 +14,13 @@
     "url": "https://github.com/appium/appium-chromedriver.git"
   },
   "bugs": {
-    "url": "https://github.com/appium/appium-chromedriver/issues"
+    "url": "https://github.com/appium/appium/issues"
+  },
+  "engines": {
+    "node": ">=14",
+    "npm": ">=6"
   },
-  "engines": [
-    "node"
-  ],
   "main": "./build/index.js",
-  "bin": {},
   "directories": {
     "lib": "lib"
   },
@@ -30,7 +30,6 @@
     "install-npm.js",
     "lib",
     "build/index.js",
-    "build/install-npm.js",
     "build/lib",
     "config/mapping.json"
   ],
@@ -54,8 +53,8 @@
   ],
   "scripts": {
     "clean": "rm -rf node_modules && rm -f package-lock.json && npm install",
-    "prepare": "gulp prepublish",
     "postinstall": "node install-npm.js",
+    "prepublishOnly": "gulp prepublish",
     "test": "gulp once",
     "watch": "gulp watch",
     "build": "gulp transpile",