Skip to content

Commit 8f3149e

Browse files
authored
improvement(build-tools): support 1-deep transitive deps (#23863)
In policy checks transitive dependencies are not deeply considered. But one level deep checks are now added to handle regular tasks that are expanded to be complex (multiple tasks). ### Example Splitting common `build:esnext` into two steps should not require downstream consumers to change dependencies as dependency on `build:esnext` still ensures that all of the build steps are run. The policy checkers know that `build:esnext:main` and/or `build:esnext:experimental` are required, but not that `build:esnext` ensures they are run. ```diff - "build:esnext": "tsc --project ./tsconfig.json", + "build:esnext": "npm run build:esnext:main && npm run build:esnext:experimental", + "build:esnext:experimental": "tsc --project ./tsconfig.json", + "build:esnext:main": "tsc --project ./tsconfig.main.json", ``` ### Infrastructure changes in support: - Add "children" to Task Definitions to understand what other tasks are run when running a task. This enables policy checks to work with a multi-step task as a dependency and not requiring explicit dependency on one or each part. "children" property of Task Definition is computed from script command line explicitly and is not manually configurable. Scripts listed via `npm run` (no other arguments) and `concurrently` are recognized. - Refactors existing logic from taskFactory to support `concurrently` parsing. - Add broad `readonly` attribute to Task Definitions and Configs that shouldn't be changed. Add cloning where mutation may occur. (Note that TypeScript doesn't detect all cases where something readonly is assigned to non-readonly context. See microsoft/TypeScript#18770.) Where mutability is needed in policy fix-up handler, cast away to writeable.
1 parent 3f44d43 commit 8f3149e

File tree

5 files changed

+352
-114
lines changed

5 files changed

+352
-114
lines changed

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts

Lines changed: 98 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,15 @@ function isFluidBuildEnabled(root: string, json: Readonly<PackageJson>): boolean
300300
return getFluidPackageMap(root).get(json.name) !== undefined;
301301
}
302302

303+
function getOrSet<T>(map: Map<string, T>, key: string, defaultValue: T): T {
304+
const value = map.get(key);
305+
if (value !== undefined) {
306+
return value;
307+
}
308+
map.set(key, defaultValue);
309+
return defaultValue;
310+
}
311+
303312
/**
304313
* Check if a task has a specific dependency
305314
* @param root - directory of the Fluid repo root
@@ -316,12 +325,25 @@ function hasTaskDependency(
316325
): boolean {
317326
const rootConfig = getFluidBuildConfig(root);
318327
const globalTaskDefinitions = normalizeGlobalTaskDefinitions(rootConfig?.tasks);
319-
const taskDefinitions = getTaskDefinitions(json, globalTaskDefinitions, false);
328+
const taskDefinitions = getTaskDefinitions(json, globalTaskDefinitions, {
329+
isReleaseGroupRoot: false,
330+
});
320331
// Searched deps that are package specific (e.g. <packageName>#<taskName>)
321332
// It is expected that all packageNames are other packages' names; using
322333
// given package's name (json.name) will alway return false as package is
323334
// not a dependency of itself. Skip "name# prefix for self dependencies.
324335
const packageSpecificSearchDeps = searchDeps.filter((d) => d.includes("#"));
336+
const secondaryPackagesTasksToConsider = new Map<
337+
string,
338+
{ searchDeps: string[]; tasks: Set<string> }
339+
>();
340+
for (const d of packageSpecificSearchDeps) {
341+
const [pkg, task] = d.split("#");
342+
getOrSet(secondaryPackagesTasksToConsider, pkg, {
343+
searchDeps: [],
344+
tasks: new Set<string>(),
345+
}).searchDeps.push(task);
346+
}
325347
/**
326348
* Set of package dependencies
327349
*/
@@ -352,31 +374,73 @@ function hasTaskDependency(
352374
if (dep.startsWith("^")) {
353375
// ^ means "depends on the task of the same name in all package dependencies".
354376
// dep of exactly ^* means "_all_ tasks in all package dependencies".
355-
const regexSearchMatches = new RegExp(dep === "^*" ? "." : `#${dep.slice(1)}$`);
377+
const depPattern = dep.slice(1);
378+
const regexSearchMatches = new RegExp(depPattern === "*" ? "." : `#${depPattern}$`);
379+
// Check for task matches
356380
const possibleSearchMatches = packageSpecificSearchDeps.filter((searchDep) =>
357381
regexSearchMatches.test(searchDep),
358382
);
383+
// Check if there is matching dependency
359384
if (
360385
possibleSearchMatches.some((searchDep) =>
361386
packageDependencies.has(searchDep.split("#")[0]),
362387
)
363388
) {
364389
return true;
365390
}
391+
if (depPattern === "*") {
392+
// No possible match even through transitive dependencies since
393+
// ^* would already consider all tasks in all dependencies.
394+
continue;
395+
}
396+
for (const [packageName, secondaryData] of secondaryPackagesTasksToConsider) {
397+
// If there is a matching dependency package, add this task to
398+
// transitive dependency in secondary package search list.
399+
if (packageDependencies.has(packageName)) {
400+
secondaryData.tasks.add(depPattern);
401+
}
402+
}
366403
continue;
367404
}
368-
if (dep.includes("#")) {
369-
// Current "pending" dependency is from another package and could possibly
370-
// be successor to given queried deps, but we are not doing such a deep or
371-
// exhaustive search at this time.
372-
continue;
405+
const packageDepMatch = dep.match(/^([^#]*)#(.*)$/);
406+
if (packageDepMatch) {
407+
// Consider one level deep of package's tasks to handle multi-task dependencies.
408+
const secondaryPackageSet = secondaryPackagesTasksToConsider.get(packageDepMatch[1]);
409+
if (secondaryPackageSet) {
410+
secondaryPackageSet.tasks.add(packageDepMatch[2]);
411+
}
412+
} else {
413+
// Do expand transitive dependencies and child tasks from local tasks.
414+
const taskDef = taskDefinitions[dep];
415+
if (taskDef !== undefined) {
416+
pending.push(...taskDef.dependsOn, ...taskDef.children);
417+
}
418+
}
419+
}
420+
421+
// Consider secondary package dependencies transitive dependencies
422+
const packageMap = getFluidPackageMap(root);
423+
for (const [packageName, secondaryData] of secondaryPackagesTasksToConsider.entries()) {
424+
const pkgJson = packageMap.get(packageName)?.packageJson;
425+
if (pkgJson === undefined) {
426+
throw new Error(`Dependent package ${packageName} not found in repo`);
373427
}
374-
// Do expand transitive dependencies from local tasks.
375-
// eslint-disable-next-line @typescript-eslint/strict-boolean-expressions
376-
if (taskDefinitions[dep]) {
377-
pending.push(...taskDefinitions[dep].dependsOn);
428+
const secondaryTaskDefinitions = getTaskDefinitions(pkgJson, globalTaskDefinitions, {
429+
isReleaseGroupRoot: false,
430+
});
431+
pending.push(...secondaryData.tasks);
432+
let dep;
433+
while ((dep = pending.pop()) !== undefined) {
434+
if (secondaryData.searchDeps.includes(dep)) {
435+
return true;
436+
}
437+
const taskDef = secondaryTaskDefinitions[dep];
438+
if (taskDef !== undefined) {
439+
pending.push(...taskDef.dependsOn, ...taskDef.children);
440+
}
378441
}
379442
}
443+
380444
return false;
381445
}
382446

@@ -408,6 +472,19 @@ function checkTaskDeps(
408472
: undefined;
409473
}
410474

475+
/**
476+
* Recursive inverse of Readonly
477+
* Makes all properties writeable through entire structure.
478+
*/
479+
type DeeplyMutable<T> = { -readonly [K in keyof T]: DeeplyMutable<T[K]> };
480+
481+
/**
482+
* Reinterprets a readonly object as a mutable object
483+
*/
484+
function asWriteable<T>(onlyReadable: T): DeeplyMutable<T> {
485+
return onlyReadable as DeeplyMutable<T>;
486+
}
487+
411488
/**
412489
* Fix up the actual dependencies of a task against an expected set of dependent tasks
413490
* @param root - directory of the Fluid repo root
@@ -428,30 +505,33 @@ function patchTaskDeps(
428505
);
429506

430507
if (missingTaskDependencies.length > 0) {
431-
const fileDep = json.fluidBuild?.tasks?.[taskName];
432-
if (fileDep === undefined) {
433-
let tasks: Exclude<Exclude<PackageJson["fluidBuild"], undefined>["tasks"], undefined>;
508+
const readonlyFileDep = json.fluidBuild?.tasks?.[taskName];
509+
if (readonlyFileDep === undefined) {
510+
let tasks: DeeplyMutable<
511+
Exclude<Exclude<PackageJson["fluidBuild"], undefined>["tasks"], undefined>
512+
>;
434513
if (json.fluidBuild === undefined) {
435514
tasks = {};
436515
json.fluidBuild = { tasks, version: 1 };
437516
} else if (json.fluidBuild.tasks === undefined) {
438517
tasks = {};
439518
json.fluidBuild.tasks = tasks;
440519
} else {
441-
tasks = json.fluidBuild.tasks;
520+
tasks = asWriteable(json.fluidBuild.tasks);
442521
}
443522

444523
tasks[taskName] = taskDeps.map((dep) => {
445524
if (Array.isArray(dep)) {
446525
throw new TypeError(
447-
`build-tools patchTaskDeps for ${taskName} will not auto select single dependency from choice of ${dep.join(
526+
`build-cli patchTaskDeps for ${taskName} will not auto select single dependency from choice of ${dep.join(
448527
" or ",
449528
)}`,
450529
);
451530
}
452531
return dep;
453532
});
454533
} else {
534+
const fileDep = asWriteable(readonlyFileDep);
455535
let depArray: string[];
456536
if (Array.isArray(fileDep)) {
457537
depArray = fileDep;
@@ -464,12 +544,12 @@ function patchTaskDeps(
464544
for (const missingDep of missingTaskDependencies) {
465545
if (Array.isArray(missingDep)) {
466546
throw new TypeError(
467-
`build-tools patchTaskDeps for ${taskName} will not auto select single dependency from choice of ${missingDep.join(
547+
`build-cli patchTaskDeps for ${taskName} will not auto select single dependency from choice of ${missingDep.join(
468548
" or ",
469549
)}`,
470550
);
471551
}
472-
// Check if already added in previous interation to avoid duplicates.
552+
// Check if already added in previous iteration to avoid duplicates.
473553
if (!depArray.includes(missingDep)) {
474554
depArray.push(missingDep);
475555
}

build-tools/packages/build-tools/src/fluidBuild/buildGraph.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,9 @@ export class BuildPackage {
100100
public readonly pkg: Package,
101101
globalTaskDefinitions: TaskDefinitions,
102102
) {
103-
this._taskDefinitions = getTaskDefinitions(
104-
this.pkg.packageJson,
105-
globalTaskDefinitions,
106-
this.pkg.isReleaseGroupRoot,
107-
);
103+
this._taskDefinitions = getTaskDefinitions(this.pkg.packageJson, globalTaskDefinitions, {
104+
isReleaseGroupRoot: this.pkg.isReleaseGroupRoot,
105+
});
108106
traceTaskDef(
109107
`${pkg.nameColored}: Task def: ${JSON.stringify(this._taskDefinitions, undefined, 2)}`,
110108
);
@@ -159,6 +157,7 @@ export class BuildPackage {
159157
dependsOn: [`^${taskName}`],
160158
script: false,
161159
before: [],
160+
children: [],
162161
after: [],
163162
};
164163
}
@@ -275,7 +274,7 @@ export class BuildPackage {
275274
}
276275

277276
// Create or get the task with names in the `deps` array
278-
private getMatchedTasks(deps: string[], pendingInitDep?: Task[]) {
277+
private getMatchedTasks(deps: readonly string[], pendingInitDep?: Task[]) {
279278
const matchedTasks: Task[] = [];
280279
for (const dep of deps) {
281280
// If pendingInitDep is undefined, that mean we don't expect the task to be found, so pretend that we already found it.
@@ -354,7 +353,7 @@ export class BuildPackage {
354353
};
355354

356355
// Expand the star entry to all scheduled tasks
357-
const expandStar = (deps: string[], getTaskNames: () => string[]) => {
356+
const expandStar = (deps: readonly string[], getTaskNames: () => string[]) => {
358357
const newDeps = deps.filter((dep) => dep !== "*");
359358
if (newDeps.length === deps.length) {
360359
return newDeps;

0 commit comments

Comments
 (0)