build: create bazel marco to test for circular dependencies (#34774)

Creates a Bazel macro that can be used to test packages for
circular dependencies. We face one limitation with Bazel:

 * Built packages use module imports, and not relative source file
 paths. This means we need custom resolution.

Fortunately, tools like `madge` support custom resolution.

Also removes the outdated `check-cycles` gulp task that
didn't catch circular dependencies. It seems like the test
became broken when we switched the packages-dist output to Bazel. It
breaks because the Bazel output doesn't use relative paths, but uses
the module imports. This will be handled in the new Bazel macro/rule.

PR Close #34774
This commit is contained in:
Paul Gschwendtner 2020-01-14 15:15:01 +01:00 committed by Andrew Kushnir
parent 0c8d085666
commit fd3cfbb678
8 changed files with 590 additions and 249 deletions

View File

@ -682,16 +682,6 @@ jobs:
- run: yarn karma start ./karma-js.conf.js --single-run --browsers=${KARMA_JS_BROWSERS} - run: yarn karma start ./karma-js.conf.js --single-run --browsers=${KARMA_JS_BROWSERS}
- run: ./scripts/saucelabs/stop-tunnel.sh - run: ./scripts/saucelabs/stop-tunnel.sh
legacy-misc-tests:
executor: default-executor
steps:
- custom_attach_workspace
- init_environment
- run: yarn gulp check-cycle
# TODO: disabled because the Bazel packages-dist does not seem to have map files for
# the ESM5/ES2015 output. See: https://github.com/angular/angular/issues/27966
# - run: yarn gulp source-map-test
# Job to run unit tests from angular/components. Needs a browser since all # Job to run unit tests from angular/components. Needs a browser since all
# component unit tests assume they're running in the browser environment. # component unit tests assume they're running in the browser environment.
material-unit-tests: material-unit-tests:
@ -802,9 +792,6 @@ workflows:
- build-ivy-npm-packages: - build-ivy-npm-packages:
requires: requires:
- setup - setup
- legacy-misc-tests:
requires:
- build-npm-packages
- legacy-unit-tests-saucelabs: - legacy-unit-tests-saucelabs:
requires: requires:
- setup - setup
@ -879,7 +866,6 @@ workflows:
- build-npm-packages - build-npm-packages
- build-ivy-npm-packages - build-ivy-npm-packages
- legacy-unit-tests-saucelabs - legacy-unit-tests-saucelabs
- legacy-misc-tests
- material-unit-tests: - material-unit-tests:
requires: requires:
- build-npm-packages - build-npm-packages

View File

@ -50,7 +50,6 @@ gulp.task('tslint', ['tools:build'], loadTask('lint'));
gulp.task('validate-commit-messages', loadTask('validate-commit-message')); gulp.task('validate-commit-messages', loadTask('validate-commit-message'));
gulp.task('source-map-test', loadTask('source-map-test')); gulp.task('source-map-test', loadTask('source-map-test'));
gulp.task('tools:build', loadTask('tools-build')); gulp.task('tools:build', loadTask('tools-build'));
gulp.task('check-cycle', loadTask('check-cycle'));
gulp.task('changelog', loadTask('changelog')); gulp.task('changelog', loadTask('changelog'));
gulp.task('changelog:zonejs', loadTask('changelog-zonejs')); gulp.task('changelog:zonejs', loadTask('changelog-zonejs'));
gulp.task('check-env', () => {/* this is a noop because the env test ran already above */}); gulp.task('check-env', () => {/* this is a noop because the env test ran already above */});

View File

@ -158,7 +158,7 @@
"jpm": "1.3.1", "jpm": "1.3.1",
"karma-browserstack-launcher": "^1.3.0", "karma-browserstack-launcher": "^1.3.0",
"karma-sauce-launcher": "^2.0.2", "karma-sauce-launcher": "^2.0.2",
"madge": "0.5.0", "madge": "^3.6.0",
"mutation-observer": "^1.0.3", "mutation-observer": "^1.0.3",
"rewire": "2.5.2", "rewire": "2.5.2",
"sauce-connect": "https://saucelabs.com/downloads/sc-4.5.1-linux.tar.gz", "sauce-connect": "https://saucelabs.com/downloads/sc-4.5.1-linux.tar.gz",

View File

@ -0,0 +1 @@
exports_files(["madge-resolve.config.js"])

View File

@ -0,0 +1,37 @@
# 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
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_test")
MADGE_CONFIG_LABEL = "//tools/circular_dependency_test:madge-resolve.config.js"
"""
Creates a test target that ensures that no circular dependencies can
be found in the given entry point file.
"""
def circular_dependency_test(name, deps, entry_point, **kwargs):
nodejs_test(
name = name,
data = ["@npm//madge", MADGE_CONFIG_LABEL] + deps,
entry_point = "@npm//:node_modules/madge/bin/cli.js",
templated_args = [
"--circular",
"--no-spinner",
# NOTE: We cannot use `$(location)` to resolve labels. This is because `ts_library`
# does not pre-declare outputs in the rule. Hence, the outputs cannot be referenced
# through labels (i.e. `//packages/core:index.js`). Read more here:
# https://docs.bazel.build/versions/2.0.0/skylark/rules.html#outputs
# TODO: revisit once https://github.com/bazelbuild/rules_nodejs/issues/1563 is solved.
"$(rlocation %s)" % entry_point,
# Madge supports custom module resolution, but expects a configuration file
# similar to a Webpack configuration file setting the `resolve` option.
"--webpack-config",
"$(rlocation $(location %s))" % MADGE_CONFIG_LABEL,
],
testonly = 1,
expected_exit_code = 0,
**kwargs
)

View File

@ -0,0 +1,38 @@
/**
* @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
*/
/**
* Custom resolution plugin for Webpack's `resolve-enhanced` package that is used by
* Madge for resolving imports. The plugin extends the resolution by leveraging the
* runfile resolution and module mappings handled in the module info aspect.
*/
class BazelRunfileResolutionPlugin {
apply(resolver) {
resolver.plugin('module', (request, callback) => {
try {
// Resolve the module through the `require.resolve` method which has been patched
// in the Bazel NodeJS loader to respect runfiles and module mappings. This allows
// Madge to handle module mappings specified in `ts_library` and `ng_module` targets.
const resolvedPath = require.resolve(request.request);
// Update the request to refer to the runfile resolved file path.
resolver.doResolve('resolve', {...request, request: resolvedPath}, null, callback, true);
return;
} catch {
}
// If the file could not be resolved through Bazel's runfile resolution, proceed
// with the default module resolvers.
callback();
});
}
}
// Configures a plugin which ensures that Madge can properly resolve specified
// dependencies through their configured module names.
module.exports = {
resolve: {plugins: [new BazelRunfileResolutionPlugin()]}
};

View File

@ -1,28 +0,0 @@
/**
* @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
*/
// tslint:disable:no-console
module.exports = (gulp) => (done) => {
const madge = require('madge');
// TODO: This only checks for circular dependencies within each package because
// imports to other packages cannot be resolved by Madge when CommonJS is used.
// We should consider updating Madge and use a tsonfig to check across packages.
const dependencyObject = madge(['dist/packages-dist/'], {
format: 'cjs',
extensions: ['.js'],
onParseFile: function(data) { data.src = data.src.replace(/\/\* circular \*\//g, '//'); }
});
const circularDependencies = dependencyObject.circular().getArray();
if (circularDependencies.length > 0) {
console.log('Found circular dependencies!');
console.log(circularDependencies);
process.exit(1);
}
done();
};

718
yarn.lock

File diff suppressed because it is too large Load Diff