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

Migrate from gulp format to ng-dev format #36726

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .circleci/config.yml
Expand Up @@ -278,7 +278,8 @@ jobs:
- run: 'yarn bazel:lint ||
(echo -e "\n.bzl files have lint errors. Please run ''yarn bazel:lint-fix''"; exit 1)'

- run: yarn -s lint --branch $CI_GIT_BASE_REVISION
- run: yarn -s tslint
- run: yarn -s ng-dev format changed $CI_GIT_BASE_REVISION --check
- run: yarn -s ts-circular-deps:check
- run: yarn -s ng-dev pullapprove verify
- run: yarn -s ng-dev commit-message validate-range --range $CI_COMMIT_RANGE
Expand Down
22 changes: 22 additions & 0 deletions .dev-infra.json
Expand Up @@ -43,5 +43,27 @@
"ve",
"zone.js"
]
},
"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"
]
}
}
3 changes: 2 additions & 1 deletion dev-infra/BUILD.bazel
Expand Up @@ -9,9 +9,10 @@ ts_library(
module_name = "@angular/dev-infra-private",
deps = [
"//dev-infra/commit-message",
"//dev-infra/format",
"//dev-infra/pullapprove",
"//dev-infra/ts-circular-dependencies",
"//dev-infra/utils:config",
"//dev-infra/utils",
"@npm//@types/node",
"@npm//@types/yargs",
"@npm//yargs",
Expand Down
2 changes: 2 additions & 0 deletions dev-infra/cli.ts
Expand Up @@ -10,13 +10,15 @@ import * as yargs from 'yargs';
import {tsCircularDependenciesBuilder} from './ts-circular-dependencies/index';
import {buildPullapproveParser} from './pullapprove/cli';
import {buildCommitMessageParser} from './commit-message/cli';
import {buildFormatParser} from './format/cli';

yargs.scriptName('ng-dev')
.demandCommand()
.recommendCommands()
.command('ts-circular-deps <command>', '', tsCircularDependenciesBuilder)
.command('pullapprove <command>', '', buildPullapproveParser)
.command('commit-message <command>', '', buildCommitMessageParser)
.command('format <command>', '', buildFormatParser)
.wrap(120)
.strict()
.parse();
4 changes: 2 additions & 2 deletions dev-infra/commit-message/BUILD.bazel
Expand Up @@ -13,7 +13,7 @@ ts_library(
module_name = "@angular/dev-infra-private/commit-message",
visibility = ["//dev-infra:__subpackages__"],
deps = [
"//dev-infra/utils:config",
"//dev-infra/utils",
"@npm//@types/node",
"@npm//@types/shelljs",
"@npm//@types/yargs",
Expand All @@ -29,7 +29,7 @@ ts_library(
srcs = ["validate.spec.ts"],
deps = [
":commit-message",
"//dev-infra/utils:config",
"//dev-infra/utils",
"@npm//@types/events",
"@npm//@types/jasmine",
"@npm//@types/node",
Expand Down
27 changes: 27 additions & 0 deletions dev-infra/format/BUILD.bazel
@@ -0,0 +1,27 @@
load("@npm_bazel_typescript//:index.bzl", "ts_library")

ts_library(
name = "format",
srcs = [
"cli.ts",
"config.ts",
"format.ts",
"run-commands-parallel.ts",
],
module_name = "@angular/dev-infra-private/format",
visibility = ["//dev-infra:__subpackages__"],
deps = [
"//dev-infra/utils",
"@npm//@types/cli-progress",
"@npm//@types/inquirer",
"@npm//@types/node",
"@npm//@types/shelljs",
"@npm//@types/yargs",
"@npm//cli-progress",
"@npm//inquirer",
"@npm//multimatch",
"@npm//shelljs",
"@npm//tslib",
"@npm//yargs",
],
)
45 changes: 45 additions & 0 deletions dev-infra/format/cli.ts
@@ -0,0 +1,45 @@
/**
* @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 * as yargs from 'yargs';

import {allChangedFilesSince, allFiles} from '../utils/repo-files';

import {checkFiles, formatFiles} from './format';

/** Build the parser for the format commands. */
export function buildFormatParser(localYargs: yargs.Argv) {
return localYargs.help()
.strict()
.demandCommand()
.option('check', {
type: 'boolean',
default: process.env['CI'] ? true : false,
description: 'Run the formatter to check formatting rather than updating code format'
})
.command(
'all', 'Run the formatter on all files in the repository', {},
({check}) => {
const executionCmd = check ? checkFiles : formatFiles;
executionCmd(allFiles());
})
.command(
'changed [shaOrRef]', 'Run the formatter on files changed since the provided sha/ref', {},
({shaOrRef, check}) => {
const sha = shaOrRef || 'master';
const executionCmd = check ? checkFiles : formatFiles;
executionCmd(allChangedFilesSince(sha));
})
.command('files <files..>', 'Run the formatter on provided files', {}, ({check, files}) => {
const executionCmd = check ? checkFiles : formatFiles;
executionCmd(files);
});
}

if (require.main === module) {
buildFormatParser(yargs).parse();
}
11 changes: 11 additions & 0 deletions dev-infra/format/config.ts
@@ -0,0 +1,11 @@
/**
* @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
*/

export interface FormatConfig {
matchers: string[];
}
130 changes: 130 additions & 0 deletions dev-infra/format/format.ts
@@ -0,0 +1,130 @@
/**
* @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 {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'];

/**
* Format provided files in place.
*/
export async function formatFiles(unfilteredFiles: string[]) {
// Whether any files failed to format.
let formatFailed = false;
// All files which formatting should be applied to.
const files = filterFilesByMatchers(unfilteredFiles);

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
josephperrott marked this conversation as resolved.
Show resolved Hide resolved
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) {
console.error(`Formatting failed, see errors above for more information.`);
process.exit(1);
}
console.info(`√ Formatting complete.`);
process.exit(0);
}

/**
* Check provided files for formatting correctness.
*/
export async function checkFiles(unfilteredFiles: string[]) {
// All files which formatting should be applied to.
const files = filterFilesByMatchers(unfilteredFiles);
// Files which are currently not formatted correctly.
const failures: string[] = [];

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.length) {
// Provide output expressing which files are failing formatting.
console.group('\nThe following files are out of format:');
for (const file of failures) {
console.info(` - ${file}`);
}
console.groupEnd();
console.info();

// If the command is run in a non-CI environment, prompt to format the files immediately.
let runFormatter = false;
if (!process.env['CI']) {
runFormatter = (await prompt({
type: 'confirm',
name: 'runFormatter',
message: 'Format the files now?',
})).runFormatter;
}

if (runFormatter) {
// Format the failing files as requested.
await formatFiles(failures);
process.exit(0);
} else {
// Inform user how to format files in the future.
console.info();
console.info(`To format the failing file run the following command:`);
console.info(` yarn ng-dev format files ${failures.join(' ')}`);
process.exit(1);
}
} else {
console.info('√ All files correctly formatted.');
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;
}
77 changes: 77 additions & 0 deletions dev-infra/format/run-commands-parallel.ts
@@ -0,0 +1,77 @@
/**
* @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 {Bar} from 'cli-progress';
import {cpus} from 'os';
import {exec} from 'shelljs';

const AVAILABLE_THREADS = Math.max(cpus().length - 1, 1);

type CallbackFunction = (file: string, code?: number, stdout?: string, stderr?: string) => void;

/**
* Run the provided commands in parallel for each provided file.
*
* A promise is returned, completed when the command has completed running for each file.
*/
export function runInParallel(providedFiles: string[], cmd: string, callback: CallbackFunction) {
return new Promise<void>((resolve) => {
if (providedFiles.length === 0) {
return resolve();
}
// 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 file = files.pop();
// If no file was pulled from the array, return as there are no more files to run against.
if (!file) {
return;
}

exec(
`${cmd} ${file}`,
{async: true, silent: true},
(code, stdout, stderr) => {
// Run the provided callback function.
callback(file, code, stdout, stderr);
// 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) {
return runCommandInThread(thread);
}
// If not more files are available, mark the thread as unused.
threads[thread] = false;
// If all of the threads are false, as they are unused, mark the progress bar
// completed and resolve the promise.
if (threads.every(active => !active)) {
progressBar.stop();
resolve();
}
},
);
// Mark the thread as in use as the command execution has been started.
threads[thread] = true;
}

// Start the progress bar
progressBar.start(files.length, 0);
// Start running the command on files from the least in each available thread.
threads.forEach((_, idx) => runCommandInThread(idx));
});
}
2 changes: 1 addition & 1 deletion dev-infra/pullapprove/BUILD.bazel
Expand Up @@ -13,7 +13,7 @@ ts_library(
module_name = "@angular/dev-infra-private/pullapprove",
visibility = ["//dev-infra:__subpackages__"],
deps = [
"//dev-infra/utils:config",
"//dev-infra/utils",
"@npm//@types/minimatch",
"@npm//@types/node",
"@npm//@types/shelljs",
Expand Down