Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(manager/pip-compile): Support -r dependencies between files #28052

Merged
merged 10 commits into from
Mar 29, 2024
Merged

feat(manager/pip-compile): Support -r dependencies between files #28052

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a95fb4c
feat(manager/pip-compile): Support -r dependencies between files
mbudnek Mar 20, 2024
d4a0501
Pre-compute a Map of lockFile->source to simplify lookup
mbudnek Mar 20, 2024
3f421ac
Refactor to unify warning between paths
mbudnek Mar 20, 2024
e2e7c23
Remove redundant if not has then set
mbudnek Mar 20, 2024
f6126bf
Push extra lock files into existing array instead of building a new one
mbudnek Mar 20, 2024
a449732
Address review comments and fix a but with transitive dependencies
mbudnek Mar 21, 2024
9c63dfd
Use the spread operator instead of Array.splice()
mbudnek Mar 21, 2024
c93ff1e
Remove unnecessary !
mbudnek Mar 24, 2024
b138577
Remove unnecessary !
mbudnek Mar 24, 2024
d8d1950
Update tests based on review feedback
mbudnek Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 138 additions & 15 deletions lib/modules/manager/pip-compile/extract.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,26 +246,33 @@ describe('modules/manager/pip-compile/extract', () => {
});

it('return sorted package files', async () => {
fs.readLocalFile.mockResolvedValueOnce(
getSimpleRequirementsFile('pip-compile --output-file=4.txt 3.in', [
'foo==1.0.1',
]),
);
fs.readLocalFile.mockResolvedValueOnce('-r 2.txt\nfoo');
fs.readLocalFile.mockResolvedValueOnce(
getSimpleRequirementsFile('pip-compile --output-file=2.txt 1.in', [
'foo==1.0.1',
]),
);
fs.readLocalFile.mockResolvedValueOnce('foo');
// eslint-disable-next-line require-await,@typescript-eslint/require-await
mbudnek marked this conversation as resolved.
Show resolved Hide resolved
fs.readLocalFile.mockImplementation(async (name): Promise<any> => {
if (name === '1.in') {
return 'foo';
} else if (name === '2.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=2.txt 1.in',
['foo==1.0.1'],
);
} else if (name === '3.in') {
return '-r 2.txt\nfoo';
} else if (name === '4.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=4.txt 3.in',
['foo==1.0.1'],
);
}
return null;
});

const lockFiles = ['4.txt', '2.txt'];
const packageFiles = await extractAllPackageFiles({}, lockFiles);
expect(packageFiles).toBeDefined();
expect(packageFiles?.map((p) => p.packageFile)).toEqual(['1.in', '3.in']);
expect(packageFiles?.map((p) => p.lockFiles!.pop())).toEqual([
'2.txt',
'4.txt',
expect(packageFiles?.map((p) => p.lockFiles!)).toEqual([
mbudnek marked this conversation as resolved.
Show resolved Hide resolved
['2.txt', '4.txt'],
['4.txt'],
]);
});

Expand Down Expand Up @@ -359,4 +366,120 @@ describe('modules/manager/pip-compile/extract', () => {
'pip-compile: dependency not found in lock file',
);
});

it('handles -r reference to another input file', async () => {
// eslint-disable-next-line require-await,@typescript-eslint/require-await
mbudnek marked this conversation as resolved.
Show resolved Hide resolved
fs.readLocalFile.mockImplementation(async (name): Promise<any> => {
if (name === '1.in') {
return 'foo';
} else if (name === '2.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=2.txt 1.in',
['foo==1.0.1'],
);
} else if (name === '3.in') {
return '-r 1.in\nfoo';
} else if (name === '4.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=4.txt 3.in',
['foo==1.0.1'],
);
}
return null;
});

const lockFiles = ['4.txt', '2.txt'];
const packageFiles = await extractAllPackageFiles({}, lockFiles);
expect(packageFiles).toBeDefined();
mbudnek marked this conversation as resolved.
Show resolved Hide resolved
expect(packageFiles?.map((p) => p.lockFiles!)).toEqual([
mbudnek marked this conversation as resolved.
Show resolved Hide resolved
['2.txt', '4.txt'],
['4.txt'],
]);
});

it('handles transitive -r references', async () => {
// eslint-disable-next-line require-await,@typescript-eslint/require-await
fs.readLocalFile.mockImplementation(async (name): Promise<any> => {
if (name === '1.in') {
return 'foo';
} else if (name === '2.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=2.txt 1.in',
['foo==1.0.1'],
);
} else if (name === '3.in') {
return '-r 1.in\nfoo';
} else if (name === '4.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=4.txt 3.in',
['foo==1.0.1'],
);
} else if (name === '5.in') {
return '-r 4.txt\nfoo';
} else if (name === '6.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=6.txt 5.in',
['foo==1.0.1'],
);
}
return null;
});

const lockFiles = ['4.txt', '2.txt', '6.txt'];
const packageFiles = await extractAllPackageFiles({}, lockFiles);
expect(packageFiles).toBeDefined();
expect(packageFiles?.map((p) => p.lockFiles!)).toEqual([
['2.txt', '4.txt', '6.txt'],
['4.txt', '6.txt'],
['6.txt'],
]);
});

it('warns on -r reference to failed file', async () => {
// eslint-disable-next-line require-await,@typescript-eslint/require-await
fs.readLocalFile.mockImplementation(async (name): Promise<any> => {
if (name === 'reqs-no-headers.txt') {
return Fixtures.get('requirementsNoHeaders.txt');
} else if (name === '1.in') {
return '-r reqs-no-headers.txt\nfoo';
} else if (name === '2.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=2.txt 1.in',
['foo==1.0.1'],
);
}
return null;
});

const lockFiles = ['reqs-no-headers.txt', '2.txt'];
const packageFiles = await extractAllPackageFiles({}, lockFiles);
expect(packageFiles).toBeDefined();
expect(packageFiles?.map((p) => p.lockFiles!)).toEqual([['2.txt']]);
expect(logger.warn).toHaveBeenCalledWith(
'pip-compile: 1.in references reqs-no-headers.txt which does not appear to be a requirements file managed by pip-compile',
);
});

it('warns on -r reference to requirements file not managed by pip-compile', async () => {
// eslint-disable-next-line require-await,@typescript-eslint/require-await
fs.readLocalFile.mockImplementation(async (name): Promise<any> => {
if (name === '1.in') {
return '-r unmanaged-file.txt\nfoo';
} else if (name === '2.txt') {
return getSimpleRequirementsFile(
'pip-compile --output-file=2.txt 1.in',
['foo==1.0.1'],
);
}
return null;
});

const lockFiles = ['2.txt'];
const packageFiles = await extractAllPackageFiles({}, lockFiles);
expect(packageFiles).toBeDefined();
expect(packageFiles?.map((p) => p.lockFiles!)).toEqual([['2.txt']]);
expect(logger.warn).toHaveBeenCalledWith(
'pip-compile: 1.in references unmanaged-file.txt which does not appear to be a requirements file managed by pip-compile',
);
});
});
30 changes: 27 additions & 3 deletions lib/modules/manager/pip-compile/extract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export async function extractAllPackageFiles(
const lockFileArgs = new Map<string, PipCompileArgs>();
const depsBetweenFiles: DependencyBetweenFiles[] = [];
const packageFiles = new Map<string, PackageFile>();
const lockFileSources = new Map<string, PackageFile>();
for (const fileMatch of fileMatches) {
const fileContent = await readLocalFile(fileMatch, 'utf8');
if (!fileContent) {
Expand Down Expand Up @@ -132,7 +133,9 @@ export async function extractAllPackageFiles(
logger.debug(
`pip-compile: ${packageFile} used in multiple output files`,
);
packageFiles.get(packageFile)!.lockFiles!.push(fileMatch);
const existingPackageFile = packageFiles.get(packageFile)!;
existingPackageFile.lockFiles!.push(fileMatch);
lockFileSources.set(fileMatch, existingPackageFile);
continue;
}
const content = await readLocalFile(packageFile, 'utf8');
Expand Down Expand Up @@ -180,11 +183,13 @@ export async function extractAllPackageFiles(
);
}
}
packageFiles.set(packageFile, {
const newPackageFile: PackageFile = {
...packageFileContent,
lockFiles: [fileMatch],
packageFile,
});
};
packageFiles.set(packageFile, newPackageFile);
lockFileSources.set(fileMatch, newPackageFile);
} else {
logger.warn(
{ packageFile },
Expand All @@ -200,6 +205,25 @@ export async function extractAllPackageFiles(
depsBetweenFiles,
packageFiles,
);

// This needs to go in reverse order to handle transitive dependencies
for (const packageFile of [...result].reverse()) {
for (const reqFile of packageFile.managerData?.requirementsFiles ?? []) {
let sourceFile: PackageFile | undefined = undefined;
if (fileMatches.includes(reqFile)) {
sourceFile = lockFileSources.get(reqFile);
} else if (packageFiles.has(reqFile)) {
sourceFile = packageFiles.get(reqFile);
}
if (!sourceFile) {
logger.warn(
`pip-compile: ${packageFile.packageFile} references ${reqFile} which does not appear to be a requirements file managed by pip-compile`,
);
continue;
}
sourceFile.lockFiles!.push(...packageFile.lockFiles!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a lock file is duplicated across package files before this operation?

Copy link
Contributor Author

@mbudnek mbudnek Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I suppose that could cause a problem with something like this

graph LR
    a.in --> b.in
    a.in --> c.in
    b.in --> d.in
    c.in --> d.in
Loading

In that case a.in's lockFiles would be ["a.txt", "b.txt", "d.txt", "c.txt", "d.txt"] and Renovate would end up running

pip-compile a.in
pip-compile b.in
pip-compile d.in
pip-compile c.in
pip-compile d.in

Where the first instance of pip-compile d.in would likely fail since b.txt and c.txt could ask for different versions of a newly-updated package.

I'll look into fixing that on Monday.

}
}
logger.debug(
'pip-compile: dependency graph:\n' +
generateMermaidGraph(depsBetweenFiles, lockFileArgs),
Expand Down
Loading