Skip to content

Commit 9cb63a4

Browse files
Copiloteleanorjboyd
andcommitted
Fix pyproject.toml CRLF parsing failure and add comprehensive tests
Co-authored-by: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com>
1 parent 30b9660 commit 9cb63a4

2 files changed

Lines changed: 189 additions & 1 deletion

File tree

src/managers/builtin/pipUtils.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,10 @@ export function validatePyprojectToml(toml: PyprojectToml): string | undefined {
7373
async function tomlParse(fsPath: string, log?: LogOutputChannel): Promise<tomljs.JsonMap> {
7474
try {
7575
const content = await fse.readFile(fsPath, 'utf-8');
76-
return tomljs.parse(content);
76+
// Normalize CRLF to LF before parsing to handle Windows-style line endings.
77+
// The TOML spec disallows control characters (including \r) in comments,
78+
// so files with CRLF line endings would fail to parse otherwise.
79+
return tomljs.parse(content.replace(/\r\n/g, '\n'));
7780
} catch (err) {
7881
log?.error('Failed to parse `pyproject.toml`:', err);
7982
}

src/test/managers/builtin/pipUtils.unit.test.ts

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import assert from 'assert';
2+
import * as fse from 'fs-extra';
3+
import * as os from 'os';
24
import * as path from 'path';
35
import * as sinon from 'sinon';
46
import { CancellationToken, Progress, ProgressOptions, Uri } from 'vscode';
@@ -200,3 +202,186 @@ suite('Pip Utils - getProjectInstallable', () => {
200202
assert.ok(firstResult.uri.fsPath.startsWith(workspacePath), 'Should be in workspace directory');
201203
});
202204
});
205+
206+
suite('Pip Utils - getProjectInstallable (pyproject.toml parsing)', () => {
207+
let findFilesStub: sinon.SinonStub;
208+
let withProgressStub: sinon.SinonStub;
209+
let mockApi: { getPythonProject: (uri: Uri) => PythonProject | undefined };
210+
let tmpDir: string;
211+
212+
setup(async () => {
213+
// Create a temporary directory with a fake workspace and pyproject.toml
214+
tmpDir = await fse.mkdtemp(path.join(os.tmpdir(), 'pyproject-toml-test-'));
215+
216+
findFilesStub = sinon.stub(wapi, 'findFiles');
217+
withProgressStub = sinon.stub(winapi, 'withProgress');
218+
withProgressStub.callsFake(
219+
async (
220+
_options: ProgressOptions,
221+
callback: (
222+
progress: Progress<{ message?: string; increment?: number }>,
223+
token: CancellationToken,
224+
) => Thenable<unknown>,
225+
) => {
226+
return await callback(
227+
{} as Progress<{ message?: string; increment?: number }>,
228+
{ isCancellationRequested: false } as CancellationToken,
229+
);
230+
},
231+
);
232+
233+
mockApi = {
234+
getPythonProject: (uri: Uri) => {
235+
if (uri.fsPath.startsWith(tmpDir)) {
236+
return { name: 'workspace', uri: Uri.file(tmpDir) };
237+
}
238+
return undefined;
239+
},
240+
};
241+
});
242+
243+
teardown(async () => {
244+
sinon.restore();
245+
await fse.remove(tmpDir);
246+
});
247+
248+
/**
249+
* Writes a pyproject.toml with the given content to the temp dir,
250+
* stubs findFiles so only that file is returned, and calls getProjectInstallable.
251+
*/
252+
async function runWithTomlContent(
253+
content: string,
254+
): Promise<Awaited<ReturnType<typeof getProjectInstallable>>> {
255+
const tomlPath = path.join(tmpDir, 'pyproject.toml');
256+
await fse.writeFile(tomlPath, content, 'utf-8');
257+
258+
findFilesStub.callsFake((pattern: string) => {
259+
if (pattern === '**/pyproject.toml') {
260+
return Promise.resolve([Uri.file(tomlPath)]);
261+
}
262+
return Promise.resolve([]);
263+
});
264+
265+
const projects = [{ name: 'workspace', uri: Uri.file(tmpDir) }];
266+
return getProjectInstallable(mockApi as PythonEnvironmentApi, projects);
267+
}
268+
269+
test('should parse a valid pyproject.toml with LF line endings', async () => {
270+
const content =
271+
'[build-system]\nrequires = ["setuptools"]\nbuild-backend = "setuptools.build_meta"\n\n[project]\nname = "mypackage"\nversion = "1.0.0"\n';
272+
273+
const result = await runWithTomlContent(content);
274+
275+
assert.strictEqual(result.installables.length, 1, 'Should find one TOML installable');
276+
assert.strictEqual(result.installables[0].group, 'TOML', 'Should be in TOML group');
277+
assert.strictEqual(result.validationError, undefined, 'Should not have validation error');
278+
});
279+
280+
test('should parse a valid pyproject.toml with CRLF line endings (Windows-style)', async () => {
281+
// This is the core regression test for issue #25809.
282+
// The @iarna/toml parser rejects \r as a control character in comments,
283+
// so we must normalize CRLF → LF before parsing.
284+
const content =
285+
'[build-system]\r\nrequires = ["setuptools"]\r\nbuild-backend = "setuptools.build_meta"\r\n\r\n[project]\r\nname = "mypackage"\r\nversion = "1.0.0"\r\n';
286+
287+
const result = await runWithTomlContent(content);
288+
289+
assert.strictEqual(result.installables.length, 1, 'Should find one TOML installable with CRLF');
290+
assert.strictEqual(result.installables[0].group, 'TOML', 'Should be in TOML group');
291+
assert.strictEqual(result.validationError, undefined, 'Should not have validation error');
292+
});
293+
294+
test('should parse a pyproject.toml with CRLF line endings and comments', async () => {
295+
// Comments with \r caused "Control characters not allowed" errors in the TOML parser
296+
const content =
297+
'# This is a comment\r\n[build-system]\r\n# Another comment\r\nrequires = ["hatchling"]\r\nbuild-backend = "hatchling.build"\r\n\r\n# Project metadata\r\n[project]\r\nname = "my-package"\r\n';
298+
299+
const result = await runWithTomlContent(content);
300+
301+
assert.strictEqual(result.installables.length, 1, 'Should parse CRLF+comments successfully');
302+
assert.strictEqual(result.validationError, undefined, 'Should not have validation error');
303+
});
304+
305+
test('should return validation error for invalid package name in pyproject.toml', async () => {
306+
// Spaces are not allowed in package names per PEP 508
307+
const content =
308+
'[build-system]\nrequires = ["setuptools"]\nbuild-backend = "setuptools.build_meta"\n\n[project]\nname = "my package"\n';
309+
310+
const result = await runWithTomlContent(content);
311+
312+
assert.strictEqual(result.installables.length, 1, 'Installable is still added even with validation error');
313+
assert.ok(result.validationError, 'Should have validation error for invalid name');
314+
assert.ok(result.validationError.message.includes('"my package"'), 'Error should mention the invalid name');
315+
});
316+
317+
test('should not add installable when pyproject.toml has no [build-system] section', async () => {
318+
// Without [build-system], the package cannot be pip-installed in editable mode
319+
const content = '[project]\nname = "mypackage"\nversion = "1.0.0"\n';
320+
321+
const result = await runWithTomlContent(content);
322+
323+
assert.strictEqual(result.installables.length, 0, 'No installable without [build-system]');
324+
assert.strictEqual(result.validationError, undefined, 'Should not have validation error');
325+
});
326+
327+
test('should not add installable when pyproject.toml has no [project] section', async () => {
328+
// Without [project], the package is not a PEP 621 project
329+
const content = '[build-system]\nrequires = ["setuptools"]\nbuild-backend = "setuptools.build_meta"\n';
330+
331+
const result = await runWithTomlContent(content);
332+
333+
assert.strictEqual(result.installables.length, 0, 'No installable without [project]');
334+
assert.strictEqual(result.validationError, undefined, 'Should not have validation error');
335+
});
336+
337+
test('should add optional-dependency extras as separate installables', async () => {
338+
const content =
339+
'[build-system]\nrequires = ["setuptools"]\nbuild-backend = "setuptools.build_meta"\n\n[project]\nname = "mypackage"\n\n[project.optional-dependencies]\ndev = ["pytest", "black"]\ndocs = ["sphinx"]\n';
340+
341+
const result = await runWithTomlContent(content);
342+
343+
// Expect: base installable + 2 extras (dev, docs)
344+
assert.strictEqual(result.installables.length, 3, 'Should have base + 2 optional dependency groups');
345+
const names = result.installables.map((i) => i.name).sort();
346+
assert.ok(names.includes('dev'), 'Should include dev extras');
347+
assert.ok(names.includes('docs'), 'Should include docs extras');
348+
assert.ok(
349+
result.installables.every((i) => i.group === 'TOML'),
350+
'All installables should be in TOML group',
351+
);
352+
});
353+
354+
test('should handle unparseable pyproject.toml gracefully (no crash)', async () => {
355+
// Completely invalid TOML content should be silently ignored
356+
const content = 'this is not valid toml @@@ ###';
357+
358+
const result = await runWithTomlContent(content);
359+
360+
assert.strictEqual(result.installables.length, 0, 'Should return empty on parse failure');
361+
assert.strictEqual(result.validationError, undefined, 'Should not report validation error on parse failure');
362+
});
363+
364+
test('should return validation error for missing build-system requires field', async () => {
365+
// PEP 518 requires the 'requires' key in [build-system]
366+
const content =
367+
'[build-system]\nbuild-backend = "setuptools.build_meta"\n\n[project]\nname = "mypackage"\n';
368+
369+
const result = await runWithTomlContent(content);
370+
371+
assert.ok(result.validationError, 'Should have validation error for missing requires');
372+
assert.ok(result.validationError.message.includes('"requires"'), 'Error should mention the requires field');
373+
});
374+
375+
test('should use editable install args (-e) for TOML installable', async () => {
376+
const content =
377+
'[build-system]\nrequires = ["setuptools"]\nbuild-backend = "setuptools.build_meta"\n\n[project]\nname = "mypackage"\n';
378+
379+
const result = await runWithTomlContent(content);
380+
381+
const installable = result.installables[0];
382+
assert.ok(installable, 'Should have an installable');
383+
assert.ok(installable.args, 'Should have args');
384+
assert.strictEqual(installable.args?.[0], '-e', 'First arg should be -e for editable install');
385+
assert.strictEqual(installable.args?.[1], tmpDir, 'Second arg should be the project directory');
386+
});
387+
});

0 commit comments

Comments
 (0)