From 2cb5f59acc557270cb20d16e3cbd0b607219af89 Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Fri, 24 Apr 2020 16:29:53 -0700 Subject: [PATCH] feat(dev-infra): run buildifier formatting and linting via ng-dev (#36842) In an effort to centralize formatting and linting enforcement into one location, buildifier is being added as a formatter for ng-dev's format command. Allowing for format enforcement for all .bzl, .bazel, WORKSPACE and BUILD files. PR Close #36842 --- .dev-infra.json | 42 ++++---- dev-infra/format/BUILD.bazel | 9 +- dev-infra/format/cli.ts | 33 ++++++- dev-infra/format/config.ts | 4 +- dev-infra/format/format.ts | 79 +++------------ dev-infra/format/formatters/base-formatter.ts | 99 +++++++++++++++++++ dev-infra/format/formatters/buildifier.ts | 54 ++++++++++ dev-infra/format/formatters/clang-format.ts | 47 +++++++++ dev-infra/format/formatters/index.ts | 29 ++++++ dev-infra/format/run-commands-parallel.ts | 67 ++++++++++--- dev-infra/tmpl-package.json | 1 + 11 files changed, 354 insertions(+), 110 deletions(-) create mode 100644 dev-infra/format/formatters/base-formatter.ts create mode 100644 dev-infra/format/formatters/buildifier.ts create mode 100644 dev-infra/format/formatters/clang-format.ts create mode 100644 dev-infra/format/formatters/index.ts diff --git a/.dev-infra.json b/.dev-infra.json index a4ef046c90..b3d7b3f2ec 100644 --- a/.dev-infra.json +++ b/.dev-infra.json @@ -45,25 +45,27 @@ ] }, "format": { - "matchers": [ - "dev-infra/**/*.{js,ts}", - "packages/**/*.{js,ts}", - "!packages/zone.js", - "!packages/common/locales/**/*.{js,ts}", - "!packages/common/src/i18n/available_locales.ts", - "!packages/common/src/i18n/currencies.ts", - "!packages/common/src/i18n/locale_en.ts", - "modules/benchmarks/**/*.{js,ts}", - "modules/playground/**/*.{js,ts}", - "tools/**/*.{js,ts}", - "!tools/gulp-tasks/cldr/extract.js", - "!tools/public_api_guard/**/*.d.ts", - "!tools/ts-api-guardian/test/fixtures/**", - "./*.{js,ts}", - "!**/node_modules/**", - "!**/dist/**", - "!**/built/**", - "!shims_for_IE.js" - ] + "matchers": { + "jsTs": [ + "dev-infra/**/*.{js,ts}", + "packages/**/*.{js,ts}", + "!packages/zone.js", + "!packages/common/locales/**/*.{js,ts}", + "!packages/common/src/i18n/available_locales.ts", + "!packages/common/src/i18n/currencies.ts", + "!packages/common/src/i18n/locale_en.ts", + "modules/benchmarks/**/*.{js,ts}", + "modules/playground/**/*.{js,ts}", + "tools/**/*.{js,ts}", + "!tools/gulp-tasks/cldr/extract.js", + "!tools/public_api_guard/**/*.d.ts", + "!tools/ts-api-guardian/test/fixtures/**", + "./*.{js,ts}", + "!**/node_modules/**", + "!**/dist/**", + "!**/built/**", + "!shims_for_IE.js" + ] + } } } diff --git a/dev-infra/format/BUILD.bazel b/dev-infra/format/BUILD.bazel index 00f8a0136a..0d1b4d80a2 100644 --- a/dev-infra/format/BUILD.bazel +++ b/dev-infra/format/BUILD.bazel @@ -2,12 +2,9 @@ load("@npm_bazel_typescript//:index.bzl", "ts_library") ts_library( name = "format", - srcs = [ - "cli.ts", - "config.ts", - "format.ts", - "run-commands-parallel.ts", - ], + srcs = glob([ + "**/*.ts", + ]), module_name = "@angular/dev-infra-private/format", visibility = ["//dev-infra:__subpackages__"], deps = [ diff --git a/dev-infra/format/cli.ts b/dev-infra/format/cli.ts index 2f04f4d466..71221e8ee1 100644 --- a/dev-infra/format/cli.ts +++ b/dev-infra/format/cli.ts @@ -34,9 +34,36 @@ export function buildFormatParser(localYargs: yargs.Argv) { const executionCmd = check ? checkFiles : formatFiles; executionCmd(allChangedFilesSince(sha)); }) - .command('files ', 'Run the formatter on provided files', {}, ({check, files}) => { - const executionCmd = check ? checkFiles : formatFiles; - executionCmd(files); + .command( + 'files ', 'Run the formatter on provided files', {}, + ({check, files}) => { + const executionCmd = check ? checkFiles : formatFiles; + executionCmd(files); + }) + // TODO(josephperrott): remove this hidden command after deprecation period. + .command('deprecation-warning [originalCommand]', false, {}, ({originalCommand}) => { + console.warn(`\`yarn ${ + originalCommand}\` is deprecated in favor of running the formatter via ng-dev`); + console.warn(); + console.warn(`As a replacement of \`yarn ${originalCommand}\`, run:`); + switch (originalCommand) { + case 'bazel:format': + case 'bazel:lint-fix': + console.warn(` yarn ng-dev format all`); + break; + case 'bazel:lint': + console.warn(` yarn ng-dev format all --check`); + break; + default: + console.warn(`Error: Unrecognized previous command.`); + } + console.warn(); + console.warn(`You can find more usage information by running:`); + console.warn(` yarn ng-dev format --help`); + console.warn(); + console.warn(`For more on the rationale and effects of this deprecation visit:`); + // TODO(josephperrott): Update this PR to the correct URL. + console.warn(` https://github.com/angular/angular/pull/36842#issue-410321447`); }); } diff --git a/dev-infra/format/config.ts b/dev-infra/format/config.ts index a61bd1f349..864f26aae2 100644 --- a/dev-infra/format/config.ts +++ b/dev-infra/format/config.ts @@ -7,5 +7,7 @@ */ export interface FormatConfig { - matchers: string[]; + [keyof: string]: boolean|{ + matchers: string[]; + }; } diff --git a/dev-infra/format/format.ts b/dev-infra/format/format.ts index 0f23ecf9b9..03f880134c 100644 --- a/dev-infra/format/format.ts +++ b/dev-infra/format/format.ts @@ -7,46 +7,22 @@ */ import {prompt} from 'inquirer'; -import * as multimatch from 'multimatch'; -import {join} from 'path'; - -import {getAngularDevConfig, getRepoBaseDir} from '../utils/config'; - -import {FormatConfig} from './config'; -import {runInParallel} from './run-commands-parallel'; - -/** By default, run the formatter on all javascript and typescript files. */ -const DEFAULT_MATCHERS = ['**/*.{t,j}s']; +import {runFormatterInParallel} from './run-commands-parallel'; /** * Format provided files in place. */ -export async function formatFiles(unfilteredFiles: string[]) { +export async function formatFiles(files: string[]) { // Whether any files failed to format. - let formatFailed = false; - // All files which formatting should be applied to. - const files = filterFilesByMatchers(unfilteredFiles); + let failures = await runFormatterInParallel(files, 'format'); - console.info(`Formatting ${files.length} file(s)`); - - - // Run the formatter to format the files in place, split across (number of available - // cpu threads - 1) processess. The task is done in multiple processess to speed up - // the overall time of the task, as running across entire repositories takes a large - // amount of time. - // As a data point for illustration, using 8 process rather than 1 cut the execution - // time from 276 seconds to 39 seconds for the same 2700 files - await runInParallel(files, `${getFormatterBinary()} -i -style=file`, (file, code, _, stderr) => { - if (code !== 0) { - formatFailed = true; - console.error(`Error running clang-format on: ${file}`); - console.error(stderr); - console.error(); - } - }); + if (failures === false) { + console.info('No files matched for formatting.'); + process.exit(0); + } // The process should exit as a failure if any of the files failed to format. - if (formatFailed) { + if (failures.length !== 0) { console.error(`Formatting failed, see errors above for more information.`); process.exit(1); } @@ -57,26 +33,14 @@ export async function formatFiles(unfilteredFiles: string[]) { /** * Check provided files for formatting correctness. */ -export async function checkFiles(unfilteredFiles: string[]) { - // All files which formatting should be applied to. - const files = filterFilesByMatchers(unfilteredFiles); +export async function checkFiles(files: string[]) { // Files which are currently not formatted correctly. - const failures: string[] = []; + const failures = await runFormatterInParallel(files, 'check'); - console.info(`Checking format of ${files.length} file(s)`); - - // Run the formatter to check the format of files, split across (number of available - // cpu threads - 1) processess. The task is done in multiple processess to speed up - // the overall time of the task, as running across entire repositories takes a large - // amount of time. - // As a data point for illustration, using 8 process rather than 1 cut the execution - // time from 276 seconds to 39 seconds for the same 2700 files. - await runInParallel(files, `${getFormatterBinary()} --Werror -n -style=file`, (file, code) => { - // Add any files failing format checks to the list. - if (code !== 0) { - failures.push(file); - } - }); + if (failures === false) { + console.info('No files matched for formatting check.'); + process.exit(0); + } if (failures.length) { // Provide output expressing which files are failing formatting. @@ -113,18 +77,3 @@ export async function checkFiles(unfilteredFiles: string[]) { process.exit(0); } } - -/** Get the full path of the formatter binary to execute. */ -function getFormatterBinary() { - return join(getRepoBaseDir(), 'node_modules/.bin/clang-format'); -} - -/** Filter a list of files to only contain files which are expected to be formatted. */ -function filterFilesByMatchers(allFiles: string[]) { - const matchers = - getAngularDevConfig<'format', FormatConfig>().format.matchers || DEFAULT_MATCHERS; - const files = multimatch(allFiles, matchers, {dot: true}); - - console.info(`Formatting enforced on ${files.length} of ${allFiles.length} file(s)`); - return files; -} diff --git a/dev-infra/format/formatters/base-formatter.ts b/dev-infra/format/formatters/base-formatter.ts new file mode 100644 index 0000000000..d46a211801 --- /dev/null +++ b/dev-infra/format/formatters/base-formatter.ts @@ -0,0 +1,99 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {FormatConfig} from '../config'; + +// A callback to determine if the formatter run found a failure in formatting. +export type CallbackFunc = (file: string, code: number, stdout: string, stderr: string) => boolean; + +// The actions a formatter can take. +export type FormatterAction = 'check'|'format'; + +// The metadata needed for running one of the `FormatterAction`s on a file. +interface FormatterActionMetadata { + commandFlags: string; + callback: CallbackFunc; +} + +/** + * The base class for formatters to run against provided files. + */ +export abstract class Formatter { + /** + * The name of the formatter, this is used for identification in logging and for enabling and + * configuring the formatter in the config. + */ + abstract name: string; + + /** The full path file location of the formatter binary. */ + abstract binaryFilePath: string; + + /** Metadata for each `FormatterAction` available to the formatter. */ + abstract actions: { + // An action performing a check of format without making any changes. + check: FormatterActionMetadata; + // An action to format files in place. + format: FormatterActionMetadata; + }; + + /** The default matchers for the formatter for filtering files to be formatted. */ + abstract defaultFileMatcher: string[]; + + constructor(private config: FormatConfig) {} + + /** + * Retrieve the command to execute the provided action, including both the binary + * and command line flags. + */ + commandFor(action: FormatterAction) { + switch (action) { + case 'check': + return `${this.binaryFilePath} ${this.actions.check.commandFlags}`; + case 'format': + return `${this.binaryFilePath} ${this.actions.format.commandFlags}`; + default: + throw Error('Unknown action type'); + } + } + + /** + * Retrieve the callback for the provided action to determine if an action + * failed in formatting. + */ + callbackFor(action: FormatterAction) { + switch (action) { + case 'check': + return this.actions.check.callback; + case 'format': + return this.actions.format.callback; + default: + throw Error('Unknown action type'); + } + } + + /** Whether the formatter is enabled in the provided config. */ + isEnabled() { + return !!this.config[this.name]; + } + + /** Retrieve the active file matcher for the formatter. */ + getFileMatcher() { + return this.getFileMatcherFromConfig() || this.defaultFileMatcher; + } + + /** + * Retrieves the file matcher from the config provided to the constructor if provided. + */ + private getFileMatcherFromConfig(): string[]|undefined { + const formatterConfig = this.config[this.name]; + if (typeof formatterConfig === 'boolean') { + return undefined; + } + return formatterConfig.matchers; + } +} diff --git a/dev-infra/format/formatters/buildifier.ts b/dev-infra/format/formatters/buildifier.ts new file mode 100644 index 0000000000..797920030f --- /dev/null +++ b/dev-infra/format/formatters/buildifier.ts @@ -0,0 +1,54 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {join} from 'path'; + +import {getRepoBaseDir} from '../../utils/config'; + +import {Formatter} from './base-formatter'; + +/** + * Formatter for running buildifier against bazel related files. + */ +export class Buildifier extends Formatter { + name = 'buildifier'; + + binaryFilePath = join(getRepoBaseDir(), 'node_modules/.bin/buildifier'); + + defaultFileMatcher = ['**/*.bzl', '**/BUILD.bazel', '**/WORKSPACE', '**/BUILD']; + + actions = { + check: { + commandFlags: `${BAZEL_WARNING_FLAG} --lint=warn --mode=check --format=json`, + callback: + (_: string, code: number, stdout: string) => { + return code !== 0 || !JSON.parse(stdout)['success']; + }, + }, + format: { + commandFlags: `${BAZEL_WARNING_FLAG} --lint=fix --mode=fix`, + callback: + (file: string, code: number, _: string, stderr: string) => { + if (code !== 0) { + console.error(`Error running buildifier on: ${file}`); + console.error(stderr); + console.error(); + return true; + } + return false; + } + } + }; +} + +// The warning flag for buildifier copied from angular/angular's usage. +const BAZEL_WARNING_FLAG = `--warnings=attr-cfg,attr-license,attr-non-empty,attr-output-default,` + + `attr-single-file,constant-glob,ctx-args,depset-iteration,depset-union,dict-concatenation,` + + `duplicated-name,filetype,git-repository,http-archive,integer-division,load,load-on-top,` + + `native-build,native-package,output-group,package-name,package-on-top,positional-args,` + + `redefined-variable,repository-name,same-origin-load,string-iteration,unused-variable`; diff --git a/dev-infra/format/formatters/clang-format.ts b/dev-infra/format/formatters/clang-format.ts new file mode 100644 index 0000000000..c3aedad384 --- /dev/null +++ b/dev-infra/format/formatters/clang-format.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {join} from 'path'; + +import {getRepoBaseDir} from '../../utils/config'; + +import {Formatter} from './base-formatter'; + +/** + * Formatter for running clang-format against Typescript and Javascript files + */ +export class ClangFormat extends Formatter { + name = 'clang-format'; + + binaryFilePath = join(getRepoBaseDir(), 'node_modules/.bin/clang-format'); + + defaultFileMatcher = ['**/*.{t,j}s']; + + actions = { + check: { + commandFlags: `--Werror -n -style=file`, + callback: + (_: string, code: number) => { + return code !== 0; + }, + }, + format: { + commandFlags: `-i -style=file`, + callback: + (file: string, code: number, _: string, stderr: string) => { + if (code !== 0) { + console.error(`Error running clang-format on: ${file}`); + console.error(stderr); + console.error(); + return true; + } + return false; + } + } + }; +} diff --git a/dev-infra/format/formatters/index.ts b/dev-infra/format/formatters/index.ts new file mode 100644 index 0000000000..fe82cc5f07 --- /dev/null +++ b/dev-infra/format/formatters/index.ts @@ -0,0 +1,29 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {getAngularDevConfig} from '../../utils/config'; +import {FormatConfig} from '../config'; + +import {Buildifier} from './buildifier'; +import {ClangFormat} from './clang-format'; + +/** + * Get all defined formatters which are active based on the current loaded config. + */ +export function getActiveFormatters() { + let config = {}; + try { + config = getAngularDevConfig<'format', FormatConfig>().format || {}; + } catch { + } + return [new Buildifier(config), new ClangFormat(config)].filter( + formatter => formatter.isEnabled()); +} + +// Rexport symbols used for types elsewhere. +export {Formatter, FormatterAction} from './base-formatter'; diff --git a/dev-infra/format/run-commands-parallel.ts b/dev-infra/format/run-commands-parallel.ts index 77831d38c3..b2f86cb60e 100644 --- a/dev-infra/format/run-commands-parallel.ts +++ b/dev-infra/format/run-commands-parallel.ts @@ -7,52 +7,89 @@ */ import {Bar} from 'cli-progress'; +import * as multimatch from 'multimatch'; import {cpus} from 'os'; import {exec} from 'shelljs'; -const AVAILABLE_THREADS = Math.max(cpus().length - 1, 1); +import {Formatter, FormatterAction, getActiveFormatters} from './formatters'; -type CallbackFunction = (file: string, code?: number, stdout?: string, stderr?: string) => void; +const AVAILABLE_THREADS = Math.max(cpus().length - 1, 1); /** * Run the provided commands in parallel for each provided file. * + * Running the formatter is split across (number of available cpu threads - 1) processess. + * The task is done in multiple processess to speed up the overall time of the task, as running + * across entire repositories takes a large amount of time. + * As a data point for illustration, using 8 process rather than 1 cut the execution + * time from 276 seconds to 39 seconds for the same 2700 files. + * * A promise is returned, completed when the command has completed running for each file. + * The promise resolves with a list of failures, or `false` if no formatters have matched. */ -export function runInParallel(providedFiles: string[], cmd: string, callback: CallbackFunction) { - return new Promise((resolve) => { - if (providedFiles.length === 0) { - return resolve(); +export function runFormatterInParallel(allFiles: string[], action: FormatterAction) { + return new Promise((resolve) => { + const formatters = getActiveFormatters(); + const failures: string[] = []; + const pendingCommands: {formatter: Formatter, file: string}[] = []; + + for (const formatter of formatters) { + pendingCommands.push(...multimatch(allFiles, formatter.getFileMatcher(), { + dot: true + }).map(file => ({formatter, file}))); } + + // If no commands are generated, resolve the promise as `false` as no files + // were run against the any formatters. + if (pendingCommands.length === 0) { + return resolve(false); + } + + switch (action) { + case 'format': + console.info(`Formatting ${pendingCommands.length} file(s)`); + break; + case 'check': + console.info(`Checking format of ${pendingCommands.length} file(s)`); + break; + default: + throw Error(`Invalid format action "${action}": allowed actions are "format" and "check"`); + } + // The progress bar instance to use for progress tracking. const progressBar = new Bar({format: `[{bar}] ETA: {eta}s | {value}/{total} files`, clearOnComplete: true}); // A local copy of the files to run the command on. - const files = providedFiles.slice(); // An array to represent the current usage state of each of the threads for parallelization. const threads = new Array(AVAILABLE_THREADS).fill(false); // Recursively run the command on the next available file from the list using the provided // thread. function runCommandInThread(thread: number) { - // Get the next file. - const file = files.pop(); + const nextCommand = pendingCommands.pop(); // If no file was pulled from the array, return as there are no more files to run against. - if (!file) { + if (nextCommand === undefined) { + threads[thread] = false; return; } + // Get the file and formatter for the next command. + const {file, formatter} = nextCommand; + exec( - `${cmd} ${file}`, + `${formatter.commandFor(action)} ${file}`, {async: true, silent: true}, (code, stdout, stderr) => { // Run the provided callback function. - callback(file, code, stdout, stderr); + const failed = formatter.callbackFor(action)(file, code, stdout, stderr); + if (failed) { + failures.push(file); + } // Note in the progress bar another file being completed. progressBar.increment(1); // If more files exist in the list, run again to work on the next file, // using the same slot. - if (files.length) { + if (pendingCommands.length) { return runCommandInThread(thread); } // If not more files are available, mark the thread as unused. @@ -61,7 +98,7 @@ export function runInParallel(providedFiles: string[], cmd: string, callback: Ca // completed and resolve the promise. if (threads.every(active => !active)) { progressBar.stop(); - resolve(); + resolve(failures); } }, ); @@ -70,7 +107,7 @@ export function runInParallel(providedFiles: string[], cmd: string, callback: Ca } // Start the progress bar - progressBar.start(files.length, 0); + progressBar.start(pendingCommands.length, 0); // Start running the command on files from the least in each available thread. threads.forEach((_, idx) => runCommandInThread(idx)); }); diff --git a/dev-infra/tmpl-package.json b/dev-infra/tmpl-package.json index 364513cc30..86f4c39cfd 100644 --- a/dev-infra/tmpl-package.json +++ b/dev-infra/tmpl-package.json @@ -9,6 +9,7 @@ "ts-circular-deps": "./ts-circular-dependencies/index.js" }, "peerDependencies": { + "@bazel/buildifier": "", "chalk": "", "clang-format": "", "cli-progress": "",