dsherret / dax

Cross-platform shell tools for Deno and Node.js inspired by zx.
MIT License
967 stars 33 forks source link

`resolveCommand` does not always resolve correctly (error in `deno_which`) #133

Open xe54ck opened 1 year ago

xe54ck commented 1 year ago

It appears that resolveCommand fails to correctly resolve the location of commands that are available on the system path. This causes scripts to fail with the error thrown here.

The following simple example will crash:

import $ from "https://deno.land/x/dax@0.30.1/mod.ts"
const winget = await $`winget --version`.text();
// console.log(await $.which("winget"));
// console.log(await $.commandExists("winget"));

The issue lies in deno_which. Here is a failing test case for deno_which:

import {
  assertEquals,
  assertRejects,
} from "https://deno.land/std@0.147.0/testing/asserts.ts";
import { Environment, which, whichSync } from "./mod.ts";

const expectedWingetLocation = await getLocation("winget");

Deno.test("should get path", async () => {
  await runTest(async (which) => {
    const result = await which("winget");
    checkMatches(result, expectedWingetLocation);
  });
});

Deno.test("should get path when using exe on windows", {
  ignore: Deno.build.os !== "windows",
}, async () => {
  await runTest(async (which) => {
    const result = await which("winget");
    checkMatches(result, expectedWingetLocation);
  });
});

async function runTest(
  action: (
    whichFunction: (
      cmd: string,
      environment?: Environment,
    ) => Promise<string | undefined>,
  ) => Promise<void>,
) {
  await action(which);
  await action((cmd, environment) => {
    try {
      return Promise.resolve(whichSync(cmd, environment));
    } catch (err) {
      return Promise.reject(err);
    }
  });
}

function checkMatches(a: string | undefined, b: string | undefined) {
  if (Deno.build.os === "windows") {
    if (a != null) {
      a = a.toLowerCase();
    }
    if (b != null) {
      b = b.toLowerCase();
    }
  }
  assertEquals(a, b);
}

async function getLocation(command: string) {
  const cmd = Deno.build.os === "windows"
    ? ["cmd", "/c", "where", command]
    : ["which", command];
  const p = await Deno.run({
    cmd,
    stdout: "piped",
  });
  try {
    return new TextDecoder().decode(await p.output()).split(/\r?\n/)[0];
  } finally {
    p.close();
  }
}
dsherret commented 1 year ago

Do you see the directory in Deno.env.get("PATH")? What’s the extension of winget? Do you see that extension in Deno.env.get("PATHEXT")?

xe54ck commented 1 year ago

The directory does appear. The extension is exe.

PS C:\> cmd /c where winget
C:\Users\user\AppData\Local\Microsoft\WindowsApps\winget.exe

PS C:\> which winget
/c/Users/user/AppData/Local/Microsoft/WindowsApps/winget

C:\Users\user\AppData\Local\Microsoft\WindowsApps is on the path.

After inspecting the WindowsApps directory, it appears winget.exe is a symlink into another directory. This seems to be the common for applications installed by Windows Apps (winget is installed this way).

MINGW64 ~/AppData/Local/Microsoft/WindowsApps
$ ls -al
total 24
...
lrwxrwxrwx 1 user 197609 101 Mar 28 15:55 winget.exe -> '/c/Program Files/WindowsApps/Microsoft.DesktopAppInstaller_1.19.10173.0_x64__8wekyb3d8bbwe/winget.exe'*
lrwxrwxrwx 1 user 197609  93 Feb 15 10:37 wt.exe -> '/c/Program Files/WindowsApps/Microsoft.WindowsTerminal_1.16.10262.0_x64__8wekyb3d8bbwe/wt.exe'*
...

If I add the symlinked path directly to %PATH%, it does work. The issue is maybe the symlink?

dsherret commented 1 year ago

Thanks for investigating! Yeah it appears deno_which needs to handle symlinks.

xe54ck commented 1 year ago

Looks like updating deno_which's fileExists and fileExistsSync functions to use Deno.lstat resolves the issue.

Updated deno_which mod.ts:

  async fileExists(path: string): Promise<boolean> {
    try {
      const result = await Deno.lstat(path);
      return result.isFile;
    } catch (err) {
      if (err instanceof Deno.errors.PermissionDenied) {
        throw err;
      }
      return false;
    }
  }

  fileExistsSync(path: string): boolean {
    try {
      const result = Deno.lstatSync(path);
      return result.isFile;
    } catch (err) {
      if (err instanceof Deno.errors.PermissionDenied) {
        throw err;
      }
      return false;
    }
  }

The Deno docs state that Deno.stat Will always follow symlinks but that does not appear to be the case. For example, this simple code errors:

 try {
    const stat = await Deno.stat(`C:/Users/user/AppData/Local/Microsoft/WindowsApps/winget.EXE`)
    console.log(stat)
} catch (e) {
    console.log(e)
}
Error: The file cannot be accessed by the system. (os error 1920): stat 'C:/Users/user/AppData/Local/Microsoft/WindowsApps/winget.EXE'
    at async Object.stat (ext:deno_fs/30_fs.js:323:15)
    at async file:///C:/Development/deno_which/test.ts:4:14

Is this a bug in Deno.stat or weird issue with Windows symlinks?

dsherret commented 1 year ago

I investigated and opened https://github.com/denoland/deno/issues/18598

This is not a deno_which issue.