From 31f58a1f046c7df4c31c98b56ba2d50a6338dd9d Mon Sep 17 00:00:00 2001 From: cornmander Date: Mon, 20 Oct 2025 19:17:44 -0400 Subject: [PATCH] Fix Windows ripgrep detection (#11492) --- .gitignore | 1 + packages/core/src/tools/ripGrep.test.ts | 195 ++++++++++++------ packages/core/src/tools/ripGrep.ts | 47 ++++- .../get-ripgrep/src/downloadRipGrep.js | 6 +- 4 files changed, 174 insertions(+), 75 deletions(-) diff --git a/.gitignore b/.gitignore index c703777ac4..ac5609d580 100644 --- a/.gitignore +++ b/.gitignore @@ -47,6 +47,7 @@ packages/cli/src/generated/ packages/core/src/generated/ .integration-tests/ packages/vscode-ide-companion/*.vsix +packages/cli/download-ripgrep*/ # GHA credentials gha-creds-*.json diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 7c47275b49..3173a1479c 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -10,8 +10,8 @@ import { expect, beforeEach, afterEach, + afterAll, vi, - type Mock, } from 'vitest'; import type { RipGrepToolParams } from './ripGrep.js'; import { canUseRipgrep, RipGrepTool, ensureRgPath } from './ripGrep.js'; @@ -19,28 +19,15 @@ import path from 'node:path'; import fs from 'node:fs/promises'; import os, { EOL } from 'node:os'; import type { Config } from '../config/config.js'; +import { Storage } from '../config/storage.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; import { downloadRipGrep } from '@joshua.litt/get-ripgrep'; -import { fileExists } from '../utils/fileUtils.js'; - // Mock dependencies for canUseRipgrep vi.mock('@joshua.litt/get-ripgrep', () => ({ downloadRipGrep: vi.fn(), })); -vi.mock('../utils/fileUtils.js', async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - fileExists: vi.fn(), - }; -}); -vi.mock('../config/storage.js', () => ({ - Storage: { - getGlobalBinDir: vi.fn().mockReturnValue('/mock/bin/dir'), - }, -})); // Mock child_process for ripgrep calls vi.mock('child_process', () => ({ @@ -48,96 +35,167 @@ vi.mock('child_process', () => ({ })); const mockSpawn = vi.mocked(spawn); +const downloadRipGrepMock = vi.mocked(downloadRipGrep); +const originalGetGlobalBinDir = Storage.getGlobalBinDir.bind(Storage); +const storageSpy = vi.spyOn(Storage, 'getGlobalBinDir'); describe('canUseRipgrep', () => { - beforeEach(() => { - vi.clearAllMocks(); + let tempRootDir: string; + let binDir: string; + + beforeEach(async () => { + downloadRipGrepMock.mockReset(); + downloadRipGrepMock.mockResolvedValue(undefined); + tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'ripgrep-bin-')); + binDir = path.join(tempRootDir, 'bin'); + await fs.mkdir(binDir, { recursive: true }); + storageSpy.mockImplementation(() => binDir); + }); + + afterEach(async () => { + storageSpy.mockImplementation(() => originalGetGlobalBinDir()); + await fs.rm(tempRootDir, { recursive: true, force: true }); }); it('should return true if ripgrep already exists', async () => { - (fileExists as Mock).mockResolvedValue(true); + const candidateNames = + process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; + const existingPath = path.join(binDir, candidateNames[0]); + await fs.writeFile(existingPath, ''); + const result = await canUseRipgrep(); expect(result).toBe(true); - expect(fileExists).toHaveBeenCalledWith(path.join('/mock/bin/dir', 'rg')); - expect(downloadRipGrep).not.toHaveBeenCalled(); + expect(downloadRipGrepMock).not.toHaveBeenCalled(); }); it('should download ripgrep and return true if it does not exist initially', async () => { - (fileExists as Mock) - .mockResolvedValueOnce(false) - .mockResolvedValueOnce(true); - (downloadRipGrep as Mock).mockResolvedValue(undefined); + const candidateNames = + process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; + const expectedPath = path.join(binDir, candidateNames[0]); + + downloadRipGrepMock.mockImplementation(async () => { + await fs.writeFile(expectedPath, ''); + }); const result = await canUseRipgrep(); expect(result).toBe(true); - expect(fileExists).toHaveBeenCalledTimes(2); - expect(downloadRipGrep).toHaveBeenCalledWith('/mock/bin/dir'); + expect(downloadRipGrep).toHaveBeenCalledWith(binDir); + await expect(fs.access(expectedPath)).resolves.toBeUndefined(); }); it('should return false if download fails and file does not exist', async () => { - (fileExists as Mock).mockResolvedValue(false); - (downloadRipGrep as Mock).mockResolvedValue(undefined); - const result = await canUseRipgrep(); expect(result).toBe(false); - expect(fileExists).toHaveBeenCalledTimes(2); - expect(downloadRipGrep).toHaveBeenCalledWith('/mock/bin/dir'); + expect(downloadRipGrep).toHaveBeenCalledWith(binDir); }); it('should propagate errors from downloadRipGrep', async () => { const error = new Error('Download failed'); - (fileExists as Mock).mockResolvedValue(false); - (downloadRipGrep as Mock).mockRejectedValue(error); + downloadRipGrepMock.mockRejectedValue(error); await expect(canUseRipgrep()).rejects.toThrow(error); - expect(fileExists).toHaveBeenCalledTimes(1); - expect(downloadRipGrep).toHaveBeenCalledWith('/mock/bin/dir'); + expect(downloadRipGrep).toHaveBeenCalledWith(binDir); + }); + + it('should only download once when called concurrently', async () => { + const candidateNames = + process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; + const expectedPath = path.join(binDir, candidateNames[0]); + + downloadRipGrepMock.mockImplementation( + () => + new Promise((resolve, reject) => { + setTimeout(() => { + fs.writeFile(expectedPath, '') + .then(() => resolve()) + .catch(reject); + }, 0); + }), + ); + + const firstCall = ensureRgPath(); + const secondCall = ensureRgPath(); + + const [pathOne, pathTwo] = await Promise.all([firstCall, secondCall]); + + expect(pathOne).toBe(expectedPath); + expect(pathTwo).toBe(expectedPath); + expect(downloadRipGrepMock).toHaveBeenCalledTimes(1); + await expect(fs.access(expectedPath)).resolves.toBeUndefined(); }); }); describe('ensureRgPath', () => { - beforeEach(() => { - vi.clearAllMocks(); + let tempRootDir: string; + let binDir: string; + + beforeEach(async () => { + downloadRipGrepMock.mockReset(); + downloadRipGrepMock.mockResolvedValue(undefined); + tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'ripgrep-bin-')); + binDir = path.join(tempRootDir, 'bin'); + await fs.mkdir(binDir, { recursive: true }); + storageSpy.mockImplementation(() => binDir); + }); + + afterEach(async () => { + storageSpy.mockImplementation(() => originalGetGlobalBinDir()); + await fs.rm(tempRootDir, { recursive: true, force: true }); }); it('should return rg path if ripgrep already exists', async () => { - (fileExists as Mock).mockResolvedValue(true); + const candidateNames = + process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; + const existingPath = path.join(binDir, candidateNames[0]); + await fs.writeFile(existingPath, ''); + const rgPath = await ensureRgPath(); - expect(rgPath).toBe(path.join('/mock/bin/dir', 'rg')); - expect(fileExists).toHaveBeenCalledOnce(); + expect(rgPath).toBe(existingPath); expect(downloadRipGrep).not.toHaveBeenCalled(); }); it('should return rg path if ripgrep is downloaded successfully', async () => { - (fileExists as Mock) - .mockResolvedValueOnce(false) - .mockResolvedValueOnce(true); - (downloadRipGrep as Mock).mockResolvedValue(undefined); + const candidateNames = + process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; + const expectedPath = path.join(binDir, candidateNames[0]); + + downloadRipGrepMock.mockImplementation(async () => { + await fs.writeFile(expectedPath, ''); + }); + const rgPath = await ensureRgPath(); - expect(rgPath).toBe(path.join('/mock/bin/dir', 'rg')); - expect(downloadRipGrep).toHaveBeenCalledOnce(); - expect(fileExists).toHaveBeenCalledTimes(2); + expect(rgPath).toBe(expectedPath); + expect(downloadRipGrep).toHaveBeenCalledTimes(1); + await expect(fs.access(expectedPath)).resolves.toBeUndefined(); }); it('should throw an error if ripgrep cannot be used after download attempt', async () => { - (fileExists as Mock).mockResolvedValue(false); - (downloadRipGrep as Mock).mockResolvedValue(undefined); await expect(ensureRgPath()).rejects.toThrow('Cannot use ripgrep.'); - expect(downloadRipGrep).toHaveBeenCalledOnce(); - expect(fileExists).toHaveBeenCalledTimes(2); + expect(downloadRipGrep).toHaveBeenCalledTimes(1); }); it('should propagate errors from downloadRipGrep', async () => { const error = new Error('Download failed'); - (fileExists as Mock).mockResolvedValue(false); - (downloadRipGrep as Mock).mockRejectedValue(error); + downloadRipGrepMock.mockRejectedValue(error); await expect(ensureRgPath()).rejects.toThrow(error); - expect(fileExists).toHaveBeenCalledTimes(1); - expect(downloadRipGrep).toHaveBeenCalledWith('/mock/bin/dir'); + expect(downloadRipGrep).toHaveBeenCalledWith(binDir); }); + + it.runIf(process.platform === 'win32')( + 'should detect ripgrep when only rg.exe exists on Windows', + async () => { + const expectedRgExePath = path.join(binDir, 'rg.exe'); + await fs.writeFile(expectedRgExePath, ''); + + const rgPath = await ensureRgPath(); + expect(rgPath).toBe(expectedRgExePath); + expect(downloadRipGrep).not.toHaveBeenCalled(); + await expect(fs.access(expectedRgExePath)).resolves.toBeUndefined(); + }, + ); }); // Helper function to create mock spawn implementations @@ -190,6 +248,9 @@ function createMockSpawn( describe('RipGrepTool', () => { let tempRootDir: string; + let tempBinRoot: string; + let binDir: string; + let ripgrepBinaryPath: string; let grepTool: RipGrepTool; const abortSignal = new AbortController().signal; @@ -200,10 +261,16 @@ describe('RipGrepTool', () => { } as unknown as Config; beforeEach(async () => { - vi.clearAllMocks(); - (downloadRipGrep as Mock).mockResolvedValue(undefined); - (fileExists as Mock).mockResolvedValue(true); - mockSpawn.mockClear(); + downloadRipGrepMock.mockReset(); + downloadRipGrepMock.mockResolvedValue(undefined); + mockSpawn.mockReset(); + tempBinRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'ripgrep-bin-')); + binDir = path.join(tempBinRoot, 'bin'); + await fs.mkdir(binDir, { recursive: true }); + const binaryName = process.platform === 'win32' ? 'rg.exe' : 'rg'; + ripgrepBinaryPath = path.join(binDir, binaryName); + await fs.writeFile(ripgrepBinaryPath, ''); + storageSpy.mockImplementation(() => binDir); tempRootDir = await fs.mkdtemp(path.join(os.tmpdir(), 'grep-tool-root-')); grepTool = new RipGrepTool(mockConfig); @@ -228,7 +295,9 @@ describe('RipGrepTool', () => { }); afterEach(async () => { + storageSpy.mockImplementation(() => originalGetGlobalBinDir()); await fs.rm(tempRootDir, { recursive: true, force: true }); + await fs.rm(tempBinRoot, { recursive: true, force: true }); }); describe('validateToolParams', () => { @@ -551,9 +620,8 @@ describe('RipGrepTool', () => { }); it('should throw an error if ripgrep is not available', async () => { - // Make ensureRgPath throw - (fileExists as Mock).mockResolvedValue(false); - (downloadRipGrep as Mock).mockResolvedValue(undefined); + await fs.rm(ripgrepBinaryPath, { force: true }); + downloadRipGrepMock.mockResolvedValue(undefined); const params: RipGrepToolParams = { pattern: 'world' }; const invocation = grepTool.build(params); @@ -1353,3 +1421,6 @@ describe('RipGrepTool', () => { }); }); }); +afterAll(() => { + storageSpy.mockRestore(); +}); diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 1e5e732c30..f9d5bc8b6f 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -21,28 +21,55 @@ import { GREP_TOOL_NAME } from './tool-names.js'; const DEFAULT_TOTAL_MAX_MATCHES = 20000; -function getRgPath(): string { - return path.join(Storage.getGlobalBinDir(), 'rg'); +function getRgCandidateFilenames(): readonly string[] { + return process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; +} + +async function resolveExistingRgPath(): Promise { + const binDir = Storage.getGlobalBinDir(); + for (const fileName of getRgCandidateFilenames()) { + const candidatePath = path.join(binDir, fileName); + if (await fileExists(candidatePath)) { + return candidatePath; + } + } + return null; +} + +let ripgrepAcquisitionPromise: Promise | null = null; + +async function ensureRipgrepAvailable(): Promise { + const existingPath = await resolveExistingRgPath(); + if (existingPath) { + return existingPath; + } + if (!ripgrepAcquisitionPromise) { + ripgrepAcquisitionPromise = (async () => { + try { + await downloadRipGrep(Storage.getGlobalBinDir()); + return await resolveExistingRgPath(); + } finally { + ripgrepAcquisitionPromise = null; + } + })(); + } + return ripgrepAcquisitionPromise; } /** * Checks if `rg` exists, if not then attempt to download it. */ export async function canUseRipgrep(): Promise { - if (await fileExists(getRgPath())) { - return true; - } - - await downloadRipGrep(Storage.getGlobalBinDir()); - return await fileExists(getRgPath()); + return (await ensureRipgrepAvailable()) !== null; } /** * Ensures `rg` is downloaded, or throws. */ export async function ensureRgPath(): Promise { - if (await canUseRipgrep()) { - return getRgPath(); + const downloadedPath = await ensureRipgrepAvailable(); + if (downloadedPath) { + return downloadedPath; } throw new Error('Cannot use ripgrep.'); } diff --git a/third_party/get-ripgrep/src/downloadRipGrep.js b/third_party/get-ripgrep/src/downloadRipGrep.js index 906b2a9e2e..3fd9073128 100644 --- a/third_party/get-ripgrep/src/downloadRipGrep.js +++ b/third_party/get-ripgrep/src/downloadRipGrep.js @@ -104,7 +104,7 @@ const untarGz = async (inFile, outDir) => { } } -export const downloadRipGrep = async () => { +export const downloadRipGrep = async (binPath = BIN_PATH) => { const target = getTarget() const url = `https://github.com/${REPOSITORY}/releases/download/${VERSION}/ripgrep-${VERSION}-${target}` const downloadPath = `${xdgCache}/vscode-ripgrep/ripgrep-${VERSION}-${target}` @@ -114,9 +114,9 @@ export const downloadRipGrep = async () => { console.info(`File ${downloadPath} has been cached`) } if (downloadPath.endsWith('.tar.gz')) { - await untarGz(downloadPath, BIN_PATH) + await untarGz(downloadPath, binPath) } else if (downloadPath.endsWith('.zip')) { - await unzip(downloadPath, BIN_PATH) + await unzip(downloadPath, binPath) } else { throw new VError(`Invalid downloadPath ${downloadPath}`) }