From 4f9374951d67c75f67a31c110bd61ab72563db7d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 3 Dec 2018 22:58:08 +0100 Subject: [PATCH] fix(bazel): ng_package cannot be run multiple times without clean (#27200) Currently when building the `ng_package` multiple times, the old `ng_package` output will be copied over to the new `ng_package` content. Resulting in packages like `src/cdk/npm_package/npm_package/npm_package/AND_MORE`. This happens because currently all TypeScript definition files are resolved from within the `binDir`. This is just wrong because it could then take up the `d.ts` files from the previous `ng_package` output. All typescript definitions that belong to the target package, should be resolved through Bazel and copied based on that computation. Also fixes that `esm` files aren't written to the `ng_package` on Windows. This is because we try to flatten paths using the `path.delimiter` while the path is always using Posix delimiters (causing the paths to be incorrect) PR Close #27200 --- .../ng_package/collect-type-definitions.bzl | 41 +++++++++++++++++++ packages/bazel/src/ng_package/ng_package.bzl | 40 ++++++++++-------- packages/bazel/src/ng_package/packager.ts | 18 ++++---- 3 files changed, 74 insertions(+), 25 deletions(-) create mode 100644 packages/bazel/src/ng_package/collect-type-definitions.bzl diff --git a/packages/bazel/src/ng_package/collect-type-definitions.bzl b/packages/bazel/src/ng_package/collect-type-definitions.bzl new file mode 100644 index 0000000000..0f2f616ffa --- /dev/null +++ b/packages/bazel/src/ng_package/collect-type-definitions.bzl @@ -0,0 +1,41 @@ +# Copyright Google LLC. 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 + +"""Collect TypeScript definition files from a rule context. + +This is used to find all files that will be copied into a "ng_package". +""" + +def _filter_typing_files(files): + return [file for file in files if file.path.endswith(".d.ts")] + +def collect_type_definitions(ctx): + """Returns a file tree containing only TypeScript definition files. + + This is useful when packaging a "ng_package" where we only want to package specified + definition files. + + Args: + ctx: ctx. + + Returns: + A file tree containing only TypeScript definition files. + """ + + # Add all source files and filter for TypeScript definition files + # See: https://docs.bazel.build/versions/master/skylark/lib/File.html#is_source + collected_files = _filter_typing_files([d for d in ctx.files.deps if d.is_source]) + + # In case source files have been explicitly specified in the attributes, just collect + # them and filter for definition files. + if hasattr(ctx.attr, "srcs"): + collected_files += _filter_typing_files(ctx.files.srcs) + + # Collect all TypeScript definition files from the specified dependencies. + for dep in ctx.attr.deps: + if hasattr(dep, "typescript"): + collected_files += dep.typescript.transitive_declarations.to_list() + + return collected_files diff --git a/packages/bazel/src/ng_package/ng_package.bzl b/packages/bazel/src/ng_package/ng_package.bzl index 12d03e574f..d20644134b 100644 --- a/packages/bazel/src/ng_package/ng_package.bzl +++ b/packages/bazel/src/ng_package/ng_package.bzl @@ -30,6 +30,7 @@ load( load("@build_bazel_rules_nodejs//:internal/node.bzl", "sources_aspect") load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleInfo") load("//packages/bazel/src:esm5.bzl", "esm5_outputs_aspect", "esm5_root_dir", "flatten_esm5") +load("//packages/bazel/src/ng_package:collect-type-definitions.bzl", "collect_type_definitions") _DEFAULT_NG_PACKAGER = "@npm//@angular/bazel/bin:packager" @@ -153,11 +154,20 @@ def _flatten_paths(directory): return result # takes an depset of files and returns an array that doesn't contain any generated files by ngc -def _filter_out_generated_files(files): +def _filter_out_generated_files(files, extension, filter_external_files): result = [] for file in files: - if (not (file.path.endswith(".ngfactory.js") or file.path.endswith(".ngsummary.js") or file.path.endswith(".ngstyle.js"))): + # If the "filter_external_files" parameter has been set to true, filter out files + # that refer to external workspaces. + if filter_external_files and file.short_path.startswith("../"): + continue + + # Filter out files that are generated by the Angular Compiler CLI. + if (not (file.path.endswith(".ngfactory.%s" % extension) or + file.path.endswith(".ngsummary.%s" % extension) or + file.path.endswith(".ngstyle.%s" % extension))): result.append(file) + return depset(result) def _esm2015_root_dir(ctx): @@ -174,8 +184,9 @@ def _filter_js_inputs(all_inputs): def _ng_package_impl(ctx): npm_package_directory = ctx.actions.declare_directory("%s.ng_pkg" % ctx.label.name) - esm_2015_files = _filter_out_generated_files(collect_es6_sources(ctx)) - esm5_sources = _filter_out_generated_files(flatten_esm5(ctx)) + esm_2015_files = _filter_out_generated_files(collect_es6_sources(ctx), "js", False) + esm5_sources = _filter_out_generated_files(flatten_esm5(ctx), "js", False) + type_definitions = _filter_out_generated_files(collect_type_definitions(ctx), "d.ts", True) # These accumulators match the directory names where the files live in the # Angular package format. @@ -284,11 +295,7 @@ def _ng_package_impl(ctx): ctx.files.srcs + ctx.files.data + esm5_sources.to_list() + - depset(transitive = [ - d.typescript.transitive_declarations - for d in ctx.attr.deps - if hasattr(d, "typescript") - ]).to_list() + + type_definitions.to_list() + [f.js for f in fesm2015 + fesm5 + esm2015 + esm5 + bundles] + [f.map for f in fesm2015 + fesm5 + esm2015 + esm5 + bundles if f.map] ) @@ -321,15 +328,16 @@ def _ng_package_impl(ctx): # placeholder packager_args.add("") - packager_args.add_joined(_flatten_paths(fesm2015), join_with = ",") - packager_args.add_joined(_flatten_paths(fesm5), join_with = ",") - packager_args.add_joined(_flatten_paths(esm2015), join_with = ",") - packager_args.add_joined(_flatten_paths(esm5), join_with = ",") - packager_args.add_joined(_flatten_paths(bundles), join_with = ",") - packager_args.add_joined([s.path for s in ctx.files.srcs], join_with = ",") + packager_args.add_joined(_flatten_paths(fesm2015), join_with = ",", omit_if_empty = False) + packager_args.add_joined(_flatten_paths(fesm5), join_with = ",", omit_if_empty = False) + packager_args.add_joined(_flatten_paths(esm2015), join_with = ",", omit_if_empty = False) + packager_args.add_joined(_flatten_paths(esm5), join_with = ",", omit_if_empty = False) + packager_args.add_joined(_flatten_paths(bundles), join_with = ",", omit_if_empty = False) + packager_args.add_joined([s.path for s in ctx.files.srcs], join_with = ",", omit_if_empty = False) + packager_args.add_joined([s.path for s in type_definitions], join_with = ",", omit_if_empty = False) # TODO: figure out a better way to gather runfiles providers from the transitive closure. - packager_args.add_joined([d.path for d in ctx.files.data], join_with = ",") + packager_args.add_joined([d.path for d in ctx.files.data], join_with = ",", omit_if_empty = False) if ctx.file.license_banner: packager_inputs.append(ctx.file.license_banner) diff --git a/packages/bazel/src/ng_package/packager.ts b/packages/bazel/src/ng_package/packager.ts index 5a20f56c35..b9d7692a3d 100644 --- a/packages/bazel/src/ng_package/packager.ts +++ b/packages/bazel/src/ng_package/packager.ts @@ -73,6 +73,9 @@ function main(args: string[]): number { // List of all files in the ng_package rule's srcs. srcsArg, + // List of all type definitions that need to packaged into the ng_package. + typeDefinitionsArg, + // List of all files in the ng_package rule's data. dataArg, @@ -85,6 +88,7 @@ function main(args: string[]): number { const esm2015 = esm2015Arg.split(',').filter(s => !!s); const esm5 = esm5Arg.split(',').filter(s => !!s); const bundles = bundlesArg.split(',').filter(s => !!s); + const typeDefinitions = typeDefinitionsArg.split(',').filter(s => !!s); const srcs = srcsArg.split(',').filter(s => !!s); const dataFiles: string[] = dataArg.split(',').filter(s => !!s); const modulesManifest = JSON.parse(modulesManifestArg); @@ -134,7 +138,8 @@ function main(args: string[]): number { * @param outDir path where we copy the file, relative to the out */ function writeEsmFile(file: string, suffix: string, outDir: string) { - const root = file.substr(0, file.lastIndexOf(suffix + path.sep) + suffix.length + 1); + // Note that the specified file path is always using the posix path delimiter. + const root = file.substr(0, file.lastIndexOf(`${suffix}/`) + suffix.length + 1); const rel = path.dirname(path.relative(path.join(root, srcDir), file)); if (!rel.startsWith('..')) { copyFile(file, path.join(out, outDir), rel); @@ -148,8 +153,9 @@ function main(args: string[]): number { fesm2015.forEach(file => { copyFile(file, out, 'fesm2015'); }); fesm5.forEach(file => { copyFile(file, out, 'fesm5'); }); - const allsrcs = shx.find('-R', binDir); - allsrcs.filter(hasFileExtension('.d.ts')).forEach((f: string) => { + // Copy all type definitions into the package. This is necessary so that developers can use + // the package with type definitions. + typeDefinitions.forEach((f: string) => { const content = fs.readFileSync(f, 'utf-8') // Strip the named AMD module for compatibility with non-bazel users .replace(/^\/\/\/ \n/gm, ''); @@ -235,12 +241,6 @@ function main(args: string[]): number { return `./${result}`; } - /** Gets a predicate function to filter non-generated files with a specified extension. */ - function hasFileExtension(ext: string): (path: string) => boolean { - return f => f.endsWith(ext) && !f.endsWith(`.ngfactory${ext}`) && - !f.endsWith(`.ngsummary${ext}`); - } - function copyFile(file: string, baseDir: string, relative = '.') { const dir = path.join(baseDir, relative); shx.mkdir('-p', dir);