Test for hard-to-detect recursive CLI calls (#996)

Yarn Berry uses an even different scheme for running bin scripts: it unpacks the script into a temporary directory and adds that temporary directory into the shell's environment. This mitigate is by far the most expensive, so it's used dead-last to avoid the overhead. Looks like on yarn 4.1.1, the call that succeeds takes an additional 140ms. Updated the call order and caching strategy to cache successful runs of the CLI (if it differs from the current cached value) to speed this up on multiple runs from newer Yarn invocations.
edgedb · May 3, 2024 · 9f5431f · 9f5431f
1 parent e888e7d
commit 9f5431f
Show file tree

Hide file tree

Showing 2 changed files with 149 additions and 7 deletions.
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -147,3 +147,87 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - run: echo OK
+
+  test-cli-wrapper:
+    name: "Test CLI Wrapper"
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 1
+
+      - name: Set up Node 20
+        uses: actions/setup-node@v4
+        with:
+          node-version: 20
+
+      - name: Set up Deno
+        uses: denoland/setup-deno@v1
+        with:
+          deno-version: v1.x
+
+      - name: Install EdgeDB
+        uses: edgedb/setup-edgedb@6763b6de72782d9c2e5ecc1095986a1c707da68f
+        with:
+          cli-version: stable
+          server-version: none
+
+      - name: Install dev deps
+        run: |
+          yarn --frozen-lockfile
+
+      - name: Build and pack CLI wrapper
+        run: |
+          yarn workspace edgedb run build
+          yarn workspace edgedb pack --filename=${{ github.workspace }}/edgedb-cli.tar.gz
+
+      - name: Test CLI wrapper with npm
+        run: |
+          mkdir temp-npm
+          cd temp-npm
+          npm init -y
+          npm install ${{ github.workspace }}/edgedb-cli.tar.gz
+          npm exec edgedb -- --version
+
+      - name: Test CLI wrapper with yarn
+        run: |
+          mkdir temp-yarn
+          cd temp-yarn
+          yarn init -y
+          yarn add ${{ github.workspace}}/edgedb-cli.tar.gz
+          yarn edgedb --version
+
+      - uses: threeal/setup-yarn-action@ec8c075e62bc497968de40011c2b766f5e8f1ac5
+        with:
+          version: latest
+          cache: false
+      - name: Test CLI wrapper with yarn-berry
+        run: |
+          mkdir temp-yarn-berry
+          cd temp-yarn-berry
+          yarn set version berry
+          yarn init -y
+          touch yarn.lock
+          yarn add ${{ github.workspace }}/edgedb-cli.tar.gz
+          yarn edgedb --version
+
+      - uses: pnpm/action-setup@a3252b78c470c02df07e9d59298aecedc3ccdd6d
+        with:
+          version: latest
+          run_install: false
+      - name: Test CLI wrapper with pnpm
+        run: |
+          mkdir temp-pnpm
+          cd temp-pnpm
+          pnpm init
+          pnpm add ${{ github.workspace }}/edgedb-cli.tar.gz
+          pnpm exec edgedb --version
+
+      - uses: oven-sh/setup-bun@8f24390df009a496891208e5e36b8a1de1f45135
+      - name: Test CLI wrapper with bun
+        run: |
+          mkdir temp-bun
+          cd temp-bun
+          bun init
+          bun add ${{ github.workspace }}/edgedb-cli.tar.gz
+          bun edgedb --version
diff --git a/packages/driver/src/cli.mts b/packages/driver/src/cli.mts
@@ -39,9 +39,22 @@ await main(args);
 async function main(args: string[]) {
   debug(`Running CLI wrapper from: ${fileURLToPath(import.meta.url)}`);
   debug("Starting main function with args:", args);
+  debug(`  - IS_TTY: ${IS_TTY}`);
+  debug(`  - SCRIPT_LOCATION: ${SCRIPT_LOCATION}`);
+  debug(`  - EDGEDB_PKG_ROOT: ${EDGEDB_PKG_ROOT}`);
+  debug(`  - CACHE_DIR: ${CACHE_DIR}`);
+  debug(`  - TEMPORARY_CLI_PATH: ${TEMPORARY_CLI_PATH}`);
+  debug(`  - CLI_LOCATION_CACHE_FILE_PATH: ${CLI_LOCATION_CACHE_FILE_PATH}`);
+
+  // check to see if we are being tested as a CLI binary wrapper
+  if (args.length === 1 && args[0] === "--succeed-if-cli-bin-wrapper") {
+    process.exit(0);
+  }
+
+  const maybeCachedCliLocation = await getCliLocationFromCache();
   const cliLocation =
+    maybeCachedCliLocation ??
     (await whichEdgeDbCli()) ??
-    (await getCliLocationFromCache()) ??
     (await getCliLocationFromTempCli()) ??
     (await selfInstallFromTempCli()) ??
     null;
@@ -52,6 +65,14 @@ async function main(args: string[]) {
 
   try {
     runEdgeDbCli(args, cliLocation);
+    if (cliLocation !== maybeCachedCliLocation) {
+      debug("CLI location not cached.");
+      debug(`  - Cached location: ${maybeCachedCliLocation}`);
+      debug(`  - CLI location: ${cliLocation}`);
+      debug(`Updating cache with new CLI location: ${cliLocation}`);
+      await writeCliLocationToCache(cliLocation);
+      debug("Cache updated.");
+    }
   } catch (err) {
     if (
       typeof err === "object" &&
@@ -102,6 +123,16 @@ async function whichEdgeDbCli() {
       continue;
     }
 
+    try {
+      runEdgeDbCli(["--succeed-if-cli-bin-wrapper"], actualLocation, {
+        stdio: "ignore",
+      });
+      debug("  - CLI found in PATH is wrapper script. Ignoring.");
+      continue;
+    } catch (err) {
+      debug("  - CLI found in PATH is not a wrapper script. Using.");
+    }
+
     return location;
   }
   debug("  - No CLI found in PATH.");
@@ -111,9 +142,29 @@ async function whichEdgeDbCli() {
 async function getCliLocationFromCache(): Promise<string | null> {
   debug("Checking CLI cache...");
   try {
-    const cachedBinaryPath = (
-      await fs.readFile(CLI_LOCATION_CACHE_FILE_PATH, { encoding: "utf8" })
-    ).trim();
+    let cachedBinaryPath: string | null = null;
+    try {
+      cachedBinaryPath = (
+        await fs.readFile(CLI_LOCATION_CACHE_FILE_PATH, { encoding: "utf8" })
+      ).trim();
+    } catch (err: unknown) {
+      if (
+        typeof err === "object" &&
+        err !== null &&
+        "code" in err &&
+        err.code === "ENOENT"
+      ) {
+        debug("  - Cache file does not exist.");
+      } else if (
+        typeof err === "object" &&
+        err !== null &&
+        "code" in err &&
+        err.code === "EACCES"
+      ) {
+        debug("  - No permission to read cache file.");
+      }
+      return null;
+    }
     debug("  - CLI path in cache at:", cachedBinaryPath);
 
     try {
@@ -136,9 +187,7 @@ async function getCliLocationFromTempCli(): Promise<string | null> {
 
   const installDir = getInstallDir(TEMPORARY_CLI_PATH);
   const binaryPath = path.join(installDir, "edgedb");
-  await fs.writeFile(CLI_LOCATION_CACHE_FILE_PATH, binaryPath, {
-    encoding: "utf8",
-  });
+  await writeCliLocationToCache(binaryPath);
   debug("  - CLI installed at:", binaryPath);
 
   try {
@@ -151,6 +200,15 @@ async function getCliLocationFromTempCli(): Promise<string | null> {
   }
 }
 
+async function writeCliLocationToCache(cliLocation: string) {
+  debug("Writing CLI location to cache:", cliLocation);
+  const dir = path.dirname(CLI_LOCATION_CACHE_FILE_PATH);
+  await fs.mkdir(dir, { recursive: true });
+  await fs.writeFile(CLI_LOCATION_CACHE_FILE_PATH, cliLocation, {
+    encoding: "utf8",
+  });
+}
+
 async function selfInstallFromTempCli(): Promise<string | null> {
   debug("Self-installing EdgeDB CLI...");
   runEdgeDbCli(["_self_install"], TEMPORARY_CLI_PATH);