From 56cff1254ea7a0ba1c760e0f9a35f4a1e15e91f3 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 5 Sep 2025 19:04:45 +0800 Subject: [PATCH 1/2] fix: Try locating packages using importlib --- README.md | 2 + packages/pyright-scip/{AGENT.md => AGENTS.md} | 4 + .../src/virtualenv/environment.ts | 163 +++++++++++++++-- packages/pyright-scip/test/python-cli-test.ts | 168 ++++++++++++++++++ packages/pyright-scip/test/test-main.ts | 6 +- 5 files changed, 330 insertions(+), 13 deletions(-) rename packages/pyright-scip/{AGENT.md => AGENTS.md} (80%) create mode 100644 packages/pyright-scip/test/python-cli-test.ts diff --git a/README.md b/README.md index eaea187d0..c413b7613 100644 --- a/README.md +++ b/README.md @@ -6,6 +6,8 @@ Project is primarily an addition to Pyright. At this time, there are no substant ## Pre-requisites +scip-python only supports Python 3.10+. + ``` $ # Install scip-python $ npm install -g @sourcegraph/scip-python diff --git a/packages/pyright-scip/AGENT.md b/packages/pyright-scip/AGENTS.md similarity index 80% rename from packages/pyright-scip/AGENT.md rename to packages/pyright-scip/AGENTS.md index b45b8c7fd..584923fef 100644 --- a/packages/pyright-scip/AGENT.md +++ b/packages/pyright-scip/AGENTS.md @@ -8,6 +8,10 @@ - `npm run check-snapshots` - Check snapshot tests - `npm run update-snapshots` - Update snapshot tests +After making changes to the codebase, run tests with: +1. `npm run build-agent` - Build the development version +2. `npm run check-snapshots` - Run all tests including unit tests + ### Building - `npm run webpack` - Development build diff --git a/packages/pyright-scip/src/virtualenv/environment.ts b/packages/pyright-scip/src/virtualenv/environment.ts index 60931214e..8843c8bc4 100644 --- a/packages/pyright-scip/src/virtualenv/environment.ts +++ b/packages/pyright-scip/src/virtualenv/environment.ts @@ -1,6 +1,7 @@ import * as fs from 'fs'; import * as child_process from 'child_process'; import * as os from 'os'; +import * as path from 'path'; import PythonPackage from './PythonPackage'; import PythonEnvironment from './PythonEnvironment'; import { withStatus } from 'src/status'; @@ -14,6 +15,11 @@ interface PipInformation { version: string; } +type PipBulkShowResult = + | { success: true; data: string[] } + | { success: false; error: 'timeout'; message: string } + | { success: false; error: 'other'; message: string; code?: number }; + let pipCommand: string | undefined; let getPipCommand = () => { if (pipCommand === undefined) { @@ -22,14 +28,29 @@ let getPipCommand = () => { } else if (commandExistsSync('pip')) { pipCommand = 'pip'; } else { - throw new Error('Could not find valid pip command'); + throw new Error(`Could not find valid pip command. Searched PATH: ${process.env.PATH}`); } } return pipCommand; }; -function spawnSyncWithRetry(command: string, args: string[]): child_process.SpawnSyncReturns { +let pythonCommand: string | undefined; +let getPythonCommand = () => { + if (pythonCommand === undefined) { + if (commandExistsSync('python3')) { + pythonCommand = 'python3'; + } else if (commandExistsSync('python')) { + pythonCommand = 'python'; + } else { + throw new Error(`Could not find valid python command. Searched PATH: ${process.env.PATH}`); + } + } + + return pythonCommand; +}; + +function spawnSyncWithRetry(command: string, args: string[], timeout?: number): child_process.SpawnSyncReturns { let maxBuffer = 1 * 1024 * 1024; // Start with 1MB (original default) const maxMemory = os.totalmem() * 0.1; // Don't use more than 10% of total system memory @@ -37,6 +58,7 @@ function spawnSyncWithRetry(command: string, args: string[]): child_process.Spaw const result = child_process.spawnSync(command, args, { encoding: 'utf8', maxBuffer: maxBuffer, + timeout: timeout, // Will be undefined if not provided, which is fine }); const error = result.error as NodeJS.ErrnoException | null; @@ -57,6 +79,67 @@ function spawnSyncWithRetry(command: string, args: string[]): child_process.Spaw } } +// Utility function for temporary directory cleanup +function cleanupTempDirectory(tempDir: string): void { + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Warning: Failed to cleanup temp directory ${tempDir}: ${error}`); + } +} + +// Helper function to validate and warn about missing packages +function validatePackageResults(results: PythonPackage[], requestedNames: string[]): PythonPackage[] { + if (results.length !== requestedNames.length) { + const foundNames = new Set(results.map((pkg) => pkg.name)); + const missingNames = requestedNames.filter((name) => !foundNames.has(name)); + console.warn(`Warning: Could not find package information for: ${missingNames.join(', ')}`); + } + return results; +} + +function generatePackageInfoScript(): string { + return `#!/usr/bin/env python3 +import sys +import json +import importlib.metadata + +def get_package_info(package_names): + results = [] + package_set = set(package_names) # Use set for faster lookup + + for dist in importlib.metadata.distributions(): + if dist.name in package_set: + files = [] + + # Get files for this package + if dist.files: + for file_path in dist.files: + file_str = str(file_path) + + # Skip cached or out-of-project files + if file_str.startswith('..') or '__pycache__' in file_str: + continue + + # Only include .py and .pyi files + if file_str.endswith(('.py', '.pyi')): + files.append(file_str) + + results.append({ + 'name': dist.name, + 'version': dist.version, + 'files': files + }) + + return results + +if __name__ == '__main__': + package_names = set(sys.argv[1:]) + package_info = get_package_info(package_names) + json.dump(package_info, sys.stdout) +`; +} + function pipList(): PipInformation[] { const result = spawnSyncWithRetry(getPipCommand(), ['list', '--format=json']); @@ -70,19 +153,75 @@ function pipList(): PipInformation[] { // pipBulkShow returns the results of 'pip show', one for each package. // // It doesn't cross-check if the length of the output matches that of the input. -function pipBulkShow(names: string[]): string[] { +function pipBulkShow(names: string[]): PipBulkShowResult { // FIXME: The performance of this scales with the number of packages that // are installed in the Python distribution, not just the number of packages // that are requested. If 10K packages are installed, this can take several // minutes. However, it's not super obvious if there is a more performant // way to do this without hand-rolling the functionality ourselves. - const result = spawnSyncWithRetry(getPipCommand(), ['show', '-f', ...names]); + const result = spawnSyncWithRetry(getPipCommand(), ['show', '-f', ...names], 60000); // 1 minute timeout if (result.status !== 0) { - throw new Error(`pip show failed with code ${result.status}: ${result.stderr}`); + const error = result.error as NodeJS.ErrnoException | null; + if (result.signal === 'SIGTERM' || (error && error.code === 'ETIMEDOUT')) { + return { + success: false, + error: 'timeout', + message: 'pip show timed out after 1 minute.', + }; + } + return { + success: false, + error: 'other', + message: `pip show failed: ${result.stderr}`, + code: result.status ?? undefined, + }; } - return result.stdout.split('\n---').filter((pkg) => pkg.trim()); + return { + success: true, + data: result.stdout.split('\n---').filter((pkg) => pkg.trim()), + }; +} + +// Get package information by running a short Python script. +// If we fail to run that, attempt to use `pip show`. +function gatherPackageData(packageNames: string[]): PythonPackage[] { + // First try the new importlib.metadata approach + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'scip-python-')); + try { + const scriptPath = path.join(tempDir, 'get_packages.py'); + const scriptContent = generatePackageInfoScript(); + + fs.writeFileSync(scriptPath, scriptContent, { mode: 0o755 }); + + const result = spawnSyncWithRetry(getPythonCommand(), [scriptPath, ...packageNames]); + + if (result.status === 0) { + const packageData = JSON.parse(result.stdout); + const packages = packageData.map((pkg: any) => new PythonPackage(pkg.name, pkg.version, pkg.files)); + return validatePackageResults(packages, packageNames); + } else { + console.warn(`Python script failed with code ${result.status}: ${result.stderr}`); + console.warn('Falling back to pip show approach'); + } + } catch (error) { + console.warn(`Failed to use importlib.metadata approach: ${error}`); + console.warn('Falling back to pip show approach'); + } finally { + cleanupTempDirectory(tempDir); + } + + // Fallback to original pip show approach + const bulkResult = pipBulkShow(packageNames); + if (!bulkResult.success) { + console.warn(`Warning: Package discovery failed - ${bulkResult.message}`); + console.warn('Navigation to external packages may not work correctly.'); + return []; + } + + const pipResults = bulkResult.data.map((shown) => PythonPackage.fromPipShow(shown)); + return validatePackageResults(pipResults, packageNames); } export default function getEnvironment( @@ -101,13 +240,13 @@ export default function getEnvironment( return withStatus('Evaluating python environment dependencies', (progress) => { const listed = pipList(); - progress.message('Gathering environment information from `pip`'); - const bulk = pipBulkShow(listed.map((item) => item.name)); + progress.message('Gathering environment information'); + const packageNames = listed.map((item) => item.name); + const info = gatherPackageData(packageNames); - progress.message('Analyzing dependencies'); - const info = bulk.map((shown) => { - return PythonPackage.fromPipShow(shown); - }); return new PythonEnvironment(projectFiles, projectVersion, info); }); } + +// Export for testing purposes +export { gatherPackageData }; diff --git a/packages/pyright-scip/test/python-cli-test.ts b/packages/pyright-scip/test/python-cli-test.ts new file mode 100644 index 000000000..afc469ddc --- /dev/null +++ b/packages/pyright-scip/test/python-cli-test.ts @@ -0,0 +1,168 @@ +import * as path from 'path'; +import * as fs from 'fs'; +import * as os from 'os'; +import assert from 'assert'; +import { execSync } from 'child_process'; +import { gatherPackageData } from '../src/virtualenv/environment'; + +interface FileSpec { + relativePath: string; + content: string; +} + +/** + * Utility function to create files in a declarative way + */ +function writeFS(rootPath: string, files: FileSpec[]): void { + for (const file of files) { + const fullPath = path.join(rootPath, file.relativePath); + const dir = path.dirname(fullPath); + + // Create directory if it doesn't exist + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true }); + } + + fs.writeFileSync(fullPath, file.content); + } +} + +export function pythonCLITests(): void { + console.log('Running Python CLI tests...'); + + // Override console.warn to suppress warnings during tests + const originalWarn = console.warn; + console.warn = () => {}; + + let packageTempDir: string | undefined; + let venvRootDir: string | undefined; + + try { + // Create temporary directory for fake Python package + packageTempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'scip-python-test-pkg-')); + + // Create fake package structure using writeFS + const packageName = 'test-scip-package'; + const packageFiles: FileSpec[] = [ + { + relativePath: 'setup.py', + content: ` +from setuptools import setup, find_packages + +setup( + name="${packageName}", + version="1.0.0", + packages=find_packages(), + python_requires=">=3.6", +) +`, + }, + { + relativePath: `${packageName}/__init__.py`, + content: '# Test package\n', + }, + { + relativePath: `${packageName}/module.py`, + content: 'def test_function():\n pass\n', + }, + ]; + + writeFS(packageTempDir, packageFiles); + + venvRootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'scip-python-test-venv-')); + + // Set up virtual environment + // We modify PATH to simulate activating the venv. The venv's bin/ (or Scripts/ on Windows) + // directory contains Python and pip executables that are configured to use the venv context: + // - venv/bin/python: has sys.path configured to include venv's site-packages first + // - venv/bin/pip: script that uses venv's Python, so it sees venv + system packages + // By prepending venvBinDir to PATH, any Python/pip calls will use these venv-aware versions + // instead of system ones, ensuring gatherPackageData can discover our installed fake package. + const venvDir = path.join(venvRootDir, 'venv'); + execSync(`python3 -m venv "${venvDir}"`, { stdio: 'pipe' }); + const venvBinDir = path.join(venvDir, process.platform === 'win32' ? 'Scripts' : 'bin'); + const pipPath = path.join(venvBinDir, 'pip'); + // Install the fake package in the virtual environment + execSync(`"${pipPath}" install -e "${packageTempDir}"`, { stdio: 'pipe' }); + // Simulate `source venv/bin/activate` by modifying PATH so that it propagates + // down to calls inside gatherPackageData. + const originalPath = process.env.PATH; + process.env.PATH = `${venvBinDir}${path.delimiter}${originalPath}`; + + try { + // Test gatherPackageData with mix of existing and nonexistent packages + const packages = gatherPackageData([packageName, 'pip', 'nonexistent-package-12345']); + + assert.ok(packages.length > 0, 'Expected to find at least one package'); + + // Check that we found the existing packages but not the nonexistent one + const packageNames = packages.map((p) => p.name); + + assert.ok( + packageNames.includes(packageName), + `Expected to find fake package '${packageName}', got: ${packageNames.join(', ')}` + ); + + assert.ok(packageNames.includes('pip'), `Expected to find 'pip' package, got: ${packageNames.join(', ')}`); + + assert.ok( + !packageNames.includes('nonexistent-package-12345'), + 'Should not include nonexistent package in results' + ); + + // Find our fake package specifically + const fakePackage = packages.find((p) => p.name === packageName); + assert.ok(fakePackage, `Could not find fake package '${packageName}' in results`); + + // Verify that our fake package contains the files we created + const expectedFiles = ['__init__.py', 'module.py']; + for (const expectedFile of expectedFiles) { + const foundFile = fakePackage.files.some((file) => file.endsWith(expectedFile)); + assert.ok( + foundFile, + `Expected to find '${expectedFile}' in fake package files: ${fakePackage.files.join(', ')}` + ); + } + + // Verify structure of returned packages + for (const pkg of packages) { + assert.ok(pkg.name && pkg.version, `Package missing name or version: ${JSON.stringify(pkg)}`); + assert.ok(Array.isArray(pkg.files), `Package files should be an array: ${JSON.stringify(pkg)}`); + + // Verify all files are .py or .pyi + for (const file of pkg.files) { + assert.ok( + file.endsWith('.py') || file.endsWith('.pyi'), + `Unexpected file extension in ${pkg.name}: ${file}` + ); + + // Verify no __pycache__ files + assert.ok(!file.includes('__pycache__'), `Should not include __pycache__ files: ${file}`); + } + } + + console.log(`✓ Package discovery test passed`); + } finally { + // Restore original PATH + if (originalPath) { + process.env.PATH = originalPath; + } + } + } catch (error) { + console.error(`✗ Python CLI test failed: ${error}`); + throw error; + } finally { + // Restore console.warn + console.warn = originalWarn; + + // Clean up temporary directories + if (packageTempDir && fs.existsSync(packageTempDir)) { + fs.rmSync(packageTempDir, { recursive: true, force: true }); + } + if (venvRootDir && fs.existsSync(venvRootDir)) { + fs.rmSync(venvRootDir, { recursive: true, force: true }); + } + } + + console.log('Python CLI tests passed!'); +} diff --git a/packages/pyright-scip/test/test-main.ts b/packages/pyright-scip/test/test-main.ts index d0c808819..5dbc97021 100644 --- a/packages/pyright-scip/test/test-main.ts +++ b/packages/pyright-scip/test/test-main.ts @@ -7,11 +7,14 @@ import { SnapshotOptions } from '../src/MainCommand'; import { join } from 'path'; import * as path from 'path'; import * as fs from 'fs'; +import * as os from 'os'; import { Indexer } from '../src/indexer'; import { setGlobalAssertionFlags, setGlobalContext, checkSometimesAssertions, SeenCondition } from '../src/assertions'; import { normalizePathCase, isFileSystemCaseSensitive } from 'pyright-internal/common/pathUtils'; import { PyrightFileSystem } from 'pyright-internal/pyrightFileSystem'; import { createFromRealFileSystem } from 'pyright-internal/common/realFileSystem'; +// Import Python CLI tests +import { pythonCLITests } from './python-cli-test'; function createTempDirectory(outputDirectory: string, testName: string): string { const tempPrefix = path.join(path.dirname(outputDirectory), `.tmp-${testName}-`); @@ -271,6 +274,7 @@ version = "16.05"`, function unitTests(): void { testPyprojectParsing(); + pythonCLITests(); } function snapshotTests(mode: 'check' | 'update', failFast: boolean, quiet: boolean, filterTests?: string[]): void { @@ -344,7 +348,7 @@ function parseFilterTests(): string[] | undefined { } const filterTests = parseFilterTests(); -const failFast = process.argv.indexOf('--fail-fast') !== -1 ?? false; +const failFast = process.argv.indexOf('--fail-fast') !== -1; const quiet = process.argv.indexOf('--verbose') === -1; if (process.argv.indexOf('--check') !== -1) { From 812cdd1e3dd5cedd60c98902861fc7f27e6a8b13 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 5 Sep 2025 20:25:20 +0800 Subject: [PATCH 2/2] chore: Prepare for v0.6.6 release --- packages/pyright-scip/CHANGELOG.md | 7 +++++++ packages/pyright-scip/package-lock.json | 4 ++-- packages/pyright-scip/package.json | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/pyright-scip/CHANGELOG.md b/packages/pyright-scip/CHANGELOG.md index 93e2bcb7a..0ffd0ed47 100644 --- a/packages/pyright-scip/CHANGELOG.md +++ b/packages/pyright-scip/CHANGELOG.md @@ -1,5 +1,12 @@ # scip-python CHANGELOG +## v0.6.6 + +- Changes package listing to use Python's `importlib` instead of + `pip show` by default. On versions of pip 24.0 and older, `pip show` + is much slower. This also avoids parsing of unstructured data + returned by pip in favor of JSON. + ## v0.6.5 - Fixes a crash when `pip show` returns more than 1MB of data. (#151) diff --git a/packages/pyright-scip/package-lock.json b/packages/pyright-scip/package-lock.json index d1d4878e9..4fd0517be 100644 --- a/packages/pyright-scip/package-lock.json +++ b/packages/pyright-scip/package-lock.json @@ -1,12 +1,12 @@ { "name": "@sourcegraph/scip-python", - "version": "0.6.5", + "version": "0.6.6", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@sourcegraph/scip-python", - "version": "0.6.5", + "version": "0.6.6", "license": "MIT", "dependencies": { "@iarna/toml": "^2.2.5", diff --git a/packages/pyright-scip/package.json b/packages/pyright-scip/package.json index 18171a8fc..441e1cc92 100644 --- a/packages/pyright-scip/package.json +++ b/packages/pyright-scip/package.json @@ -1,6 +1,6 @@ { "name": "@sourcegraph/scip-python", - "version": "0.6.5", + "version": "0.6.6", "description": "SCIP indexer for Python", "main": "index.js", "scripts": {