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

@@ -45,25 +45,27 @@
     ]
   },
   "format": {
-    "matchers": [
+    "matchers": {
-      "dev-infra/**/*.{js,ts}",
+      "jsTs": [
-      "packages/**/*.{js,ts}",
+        "dev-infra/**/*.{js,ts}",
-      "!packages/zone.js",
+        "packages/**/*.{js,ts}",
-      "!packages/common/locales/**/*.{js,ts}",
+        "!packages/zone.js",
-      "!packages/common/src/i18n/available_locales.ts",
+        "!packages/common/locales/**/*.{js,ts}",
-      "!packages/common/src/i18n/currencies.ts",
+        "!packages/common/src/i18n/available_locales.ts",
-      "!packages/common/src/i18n/locale_en.ts",
+        "!packages/common/src/i18n/currencies.ts",
-      "modules/benchmarks/**/*.{js,ts}",
+        "!packages/common/src/i18n/locale_en.ts",
-      "modules/playground/**/*.{js,ts}",
+        "modules/benchmarks/**/*.{js,ts}",
-      "tools/**/*.{js,ts}",
+        "modules/playground/**/*.{js,ts}",
-      "!tools/gulp-tasks/cldr/extract.js",
+        "tools/**/*.{js,ts}",
-      "!tools/public_api_guard/**/*.d.ts",
+        "!tools/gulp-tasks/cldr/extract.js",
-      "!tools/ts-api-guardian/test/fixtures/**",
+        "!tools/public_api_guard/**/*.d.ts",
-      "./*.{js,ts}",
+        "!tools/ts-api-guardian/test/fixtures/**",
-      "!**/node_modules/**",
+        "./*.{js,ts}",
-      "!**/dist/**",
+        "!**/node_modules/**",
-      "!**/built/**",
+        "!**/dist/**",
-      "!shims_for_IE.js"
+        "!**/built/**",
-    ]
+        "!shims_for_IE.js"
+      ]
+    }
   }
 }

dev-infra/format/BUILD.bazel

@@ -2,12 +2,9 @@ load("@npm_bazel_typescript//:index.bzl", "ts_library")
 
 ts_library(
     name = "format",
-    srcs = [
+    srcs = glob([
-        "cli.ts",
+        "**/*.ts",
-        "config.ts",
+    ]),
-        "format.ts",
-        "run-commands-parallel.ts",
-    ],
     module_name = "@angular/dev-infra-private/format",
     visibility = ["//dev-infra:__subpackages__"],
     deps = [

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 <files..>', 'Run the formatter on provided files', {}, ({check, files}) => {
+      .command(
-        const executionCmd = check ? checkFiles : formatFiles;
+          'files <files..>', 'Run the formatter on provided files', {},
-        executionCmd(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`);
       });
 }
 

dev-infra/format/config.ts

@@ -7,5 +7,7 @@
  */
 
 export interface FormatConfig {
-  matchers: string[];
+  [keyof: string]: boolean|{
+    matchers: string[];
+  };
 }

dev-infra/format/format.ts

@@ -7,46 +7,22 @@
  */
 
 import {prompt} from 'inquirer';
-import * as multimatch from 'multimatch';
+import {runFormatterInParallel} from './run-commands-parallel';
-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'];
 
 /**
  * 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;
+  let failures = await runFormatterInParallel(files, 'format');
-  // All files which formatting should be applied to.
-  const files = filterFilesByMatchers(unfilteredFiles);
 
-  console.info(`Formatting ${files.length} file(s)`);
+  if (failures === false) {
-
+    console.info('No files matched for formatting.');
-
+    process.exit(0);
-  // 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();
-    }
-  });
 
   // 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[]) {
+export async function checkFiles(files: string[]) {
-  // All files which formatting should be applied to.
-  const files = filterFilesByMatchers(unfilteredFiles);
   // 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)`);
+  if (failures === false) {
-
+    console.info('No files matched for formatting check.');
-  // Run the formatter to check the format of files, split across (number of available
+    process.exit(0);
-  // 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.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;
-}

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;
+  }
+}

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`;

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;
+          }
+    }
+  };
+}

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';

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) {
+export function runFormatterInParallel(allFiles: string[], action: FormatterAction) {
-  return new Promise<void>((resolve) => {
+  return new Promise<false|string[]>((resolve) => {
-    if (providedFiles.length === 0) {
+    const formatters = getActiveFormatters();
-      return resolve();
+    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<boolean>(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 nextCommand = pendingCommands.pop();
-      const file = files.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));
   });

dev-infra/tmpl-package.json

@@ -9,6 +9,7 @@
     "ts-circular-deps": "./ts-circular-dependencies/index.js"
   },
   "peerDependencies": {
+    "@bazel/buildifier": "<from-root>",
     "chalk": "<from-root>",
     "clang-format": "<from-root>",
     "cli-progress": "<from-root>",