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.

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

(build-tools): base command with build project context #23257

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions build-tools/feeds/internal-build.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@fluid-tools/version-tools
@fluidframework/bundle-size-tools
@fluidframework/build-tools
@fluid-tools/build-infrastructure
@fluid-tools/build-cli
1 change: 1 addition & 0 deletions build-tools/feeds/internal-test.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@fluid-tools/version-tools
@fluidframework/bundle-size-tools
@fluidframework/build-tools
@fluid-tools/build-infrastructure
@fluid-tools/build-cli
1 change: 1 addition & 0 deletions build-tools/feeds/public.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@fluid-tools/version-tools
@fluidframework/bundle-size-tools
@fluidframework/build-tools
@fluid-tools/build-infrastructure
@fluid-tools/build-cli
1 change: 1 addition & 0 deletions build-tools/packages/build-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
"@andrewbranch/untar.js": "^1.0.3",
"@fluid-tools/version-tools": "workspace:~",
"@fluidframework/build-tools": "workspace:~",
"@fluid-tools/build-infrastructure": "workspace:~",
"@fluidframework/bundle-size-tools": "workspace:~",
"@inquirer/prompts": "^7.0.1",
"@microsoft/api-extractor": "^7.47.11",
Expand Down
187 changes: 187 additions & 0 deletions build-tools/packages/build-cli/src/BasePackageCommandWithContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import { strict as assert } from "node:assert";
import { Package } from "@fluidframework/build-tools";
import { Flags, ux, type Command } from "@oclif/core";
import async from "async";
import {
PackageFilterOptions,
PackageKind,
PackageSelectionCriteria,
PackageWithKind,
parsePackageFilterFlags,
parsePackageSelectionFlags,
selectAndFilterPackages,
} from "./filter.js";
import { type PackageSelectionDefault, filterFlags, selectionFlags } from "./flags.js";
import { BaseCommandWithBuildProject } from "./library/commands/baseCommand.js";

/**
* Commands that run operations per project.
*/
export abstract class PackageCommandWithBuildProject<
T extends typeof Command & { flags: typeof PackageCommandWithBuildProject.flags },
> extends BaseCommandWithBuildProject<T> {
static readonly flags = {
concurrency: Flags.integer({
description: "The number of tasks to execute concurrently.",
default: 25,
}),
...selectionFlags,
...filterFlags,
...BaseCommandWithBuildProject.flags,
};

/**
* The default to use as selection criteria when none is explicitly provided by the user. This enables commands
* without flags to operate on a collection of packages by default that make sense based on the command.
*/
protected abstract get defaultSelection(): PackageSelectionDefault;
protected abstract set defaultSelection(value: PackageSelectionDefault);

protected filterOptions: PackageFilterOptions | undefined;
protected selectionOptions: PackageSelectionCriteria | undefined;

/**
* An array of packages selected based on the selection criteria.
*
* @remarks
*
* Note that these packages are not necessarily the ones that are acted on. Packages are selected, then that list is
* further narrowed by filtering criteria, so this array may contain packages that are not acted on.
*/
protected selectedPackages: PackageWithKind[] | undefined;

/**
* The list of packages after all filters are applied to the selected packages.
*/
protected filteredPackages: PackageWithKind[] | undefined;

/**
* Called for each package that is selected/filtered based on the filter flags passed in to the command.
*
* @param pkg - The package being processed.
* @param kind - The kind of the package.
* @typeparam TPkg - Type of the package-like object being processed.
*/
protected abstract processPackage<TPkg extends Package>(
pkg: TPkg,
kind: PackageKind,
): Promise<void>;

protected parseFlags(): void {
this.selectionOptions = parsePackageSelectionFlags(this.flags, this.defaultSelection);
this.filterOptions = parsePackageFilterFlags(this.flags);
}

protected async selectAndFilterPackages(): Promise<void> {
if (this.selectionOptions === undefined) {
throw new Error(`No packages selected.`);
}

const ctx = await this.getContext();
const { selected, filtered } = await selectAndFilterPackages(
ctx,
this.selectionOptions,
this.filterOptions,
);

[this.selectedPackages, this.filteredPackages] = [selected, filtered];
}

public async run(): Promise<unknown> {
this.parseFlags();

assert(this.selectionOptions !== undefined, "selectionOptions is undefined");
assert(this.filterOptions !== undefined, "filterOptions is undefined");

await this.selectAndFilterPackages();

assert(this.selectedPackages !== undefined, "selectedPackages is undefined");
assert(this.filteredPackages !== undefined, "filteredPackages is undefined");

this.info(
`Filtered ${this.selectedPackages.length} packages to ${listNames(
this.filteredPackages.map((pkg) => pkg.directory),
)}`,
);

const errors = await this.processPackages(this.filteredPackages);
if (errors.length > 0) {
this.errorLog(`Completed with ${errors.length} errors.`);
for (const error of errors) {
this.errorLog(error);
}
this.exit(1);
}

return undefined;
}

/**
* Runs the processPackage method on each package in the provided array.
*
* @returns An array of error strings. If the array is not empty, at least one of the calls to processPackage failed.
*/
protected async processPackages(packages: PackageWithKind[]): Promise<string[]> {
let started = 0;
let finished = 0;
let succeeded = 0;
const errors: string[] = [];

// In verbose mode, we output a log line per package. In non-verbose mode, we want to display an activity
// spinner, so we only start the spinner if verbose is false.
const { verbose } = this.flags;

function updateStatus(): void {
if (!verbose) {
ux.action.start(
"Processing Packages...",
`${finished}/${packages.length}: ${started - finished} pending. Errors: ${
finished - succeeded
}`,
{
stdout: true,
},
);
}
}

try {
await async.mapLimit(packages, this.flags.concurrency, async (pkg: PackageWithKind) => {
started += 1;
updateStatus();
try {
await this.processPackage(pkg, pkg.kind);
succeeded += 1;
} catch (error: unknown) {
const errorString = `Error updating ${pkg.name}: '${error}'\nStack: ${
(error as Error).stack
}`;
errors.push(errorString);
this.verbose(errorString);
} finally {
finished += 1;
updateStatus();
}
});
} finally {
// Stop the spinner if needed.
if (!verbose) {
ux.action.stop(`Done. ${packages.length} Packages. ${finished - succeeded} Errors`);
}
}
return errors;
}
}

function listNames(strings: string[] | undefined): string {
return strings === undefined
? ""
: strings.length > 10
? `${strings.length}`
: `${strings.length} (${strings.join(", ")})`;
}
27 changes: 27 additions & 0 deletions build-tools/packages/build-cli/src/library/commands/baseCommand.ts
zhenmichael marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/

import {
IBuildProject,
findGitRootSync,
loadBuildProject,
} from "@fluid-tools/build-infrastructure";
import { BaseCommand } from "./base.js";
import type { Command } from "@oclif/core";

export abstract class BaseCommandWithBuildProject<
T extends typeof Command
> extends BaseCommand<T> {
private _buildProject: IBuildProject | undefined;

public getBuildProject(repoRoot?: string): IBuildProject {
if (this._buildProject === undefined) {
const root = repoRoot ?? findGitRootSync();
this._buildProject = loadBuildProject(root);
}

return this._buildProject;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

export { BaseCommand } from "./base.js";
export { BaseCommandWithBuildProject } from "./baseCommand.js";
export { unscopedPackageNameString } from "./constants.js";
export {
GenerateEntrypointsCommand,
Expand Down
1 change: 1 addition & 0 deletions build-tools/packages/build-cli/src/library/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export {
export {
unscopedPackageNameString,
BaseCommand,
BaseCommandWithBuildProject,
GenerateEntrypointsCommand,
} from "./commands/index.js";
export { Context, VersionDetails, isMonoRepoKind, MonoRepoKind } from "./context.js";
Expand Down
1 change: 0 additions & 1 deletion build-tools/packages/build-infrastructure/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"name": "@fluid-tools/build-infrastructure",
"version": "0.52.0",
"private": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the motivation or use case for the whole change, if it requires us to make this public and get it published.

Copy link
Member

Choose a reason for hiding this comment

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

Great question. The ultimate goal is to replace all of the release group/package handling code in build-cli commands with the new code from build-infra. But rolling out changes to all the commands at once is just too big a change/task, so the plan is to convert commands one-by-one by creating new versions of the commands that use this new base class. That way we can roll out the changes piecemeal.

Long-term, we may not need a separate package for the infra stuff, but at the moment it's much easier for it to be separate right now, because build-cli and fluid-build need the underlying code. Unfortunately, fluid-build cannot depend on build-cli today because there's already a dep going the other way (that is, build-cli depends on fluid-build).

"description": "Fluid build infrastructure",
"homepage": "https://fluidframework.com",
"repository": {
Expand Down
3 changes: 3 additions & 0 deletions build-tools/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading