From 998efc26618add25e9058dc6370645b263353efa Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 2 Apr 2024 22:30:28 -0400 Subject: [PATCH] refactor: add include_sources and include_transitive_declarations to js_filegroup rules and gather_files_from_js_providers + gather_runfiles helpers (#1585) --- docs/js_filegroup.md | 8 ++-- js/private/js_filegroup.bzl | 19 ++++++-- js/private/js_helpers.bzl | 45 +++++++++++++++---- js/private/js_run_devserver.bzl | 2 + .../test/create_launcher/custom_test.bzl | 2 + 5 files changed, 61 insertions(+), 15 deletions(-) diff --git a/docs/js_filegroup.md b/docs/js_filegroup.md index dff5a631b..8fc8fdf75 100644 --- a/docs/js_filegroup.md +++ b/docs/js_filegroup.md @@ -7,8 +7,8 @@ Helper rule to gather files from JsInfo providers of targets and provide them as ## js_filegroup 
-js_filegroup(name, include_declarations, include_npm_linked_packages, include_transitive_sources,
-             srcs)
+js_filegroup(name, include_declarations, include_npm_linked_packages, include_sources,
+             include_transitive_declarations, include_transitive_sources, srcs)
Gathers files from the JsInfo providers from targets in srcs and provides them as default outputs. @@ -22,8 +22,10 @@ This helper rule is used by the `js_run_binary` macro. | Name | Description | Type | Mandatory | Default | | :------------- | :------------- | :------------- | :------------- | :------------- | | name | A unique name for this target. | Name | required | | -| include_declarations | When True, declarations and transitive_declarations from JsInfo providers in srcs targets are included in the default outputs of the target.

Defaults to false since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | False | +| include_declarations | When True, declarations from JsInfo providers in srcs targets are included in the default outputs of the target.

Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | False | | include_npm_linked_packages | When True, files in npm_linked_packages and transitive_npm_linked_packages from JsInfo providers in srcs targets are included in the default outputs of the target.

transitive_files from NpmPackageStoreInfo providers in data targets are also included in the default outputs of the target. | Boolean | optional | True | +| include_sources | When True, sources from JsInfo providers in srcs targets are included in the default outputs of the target. | Boolean | optional | True | +| include_transitive_declarations | When True, transitive_declarations from JsInfo providers in srcs targets are included in the default outputs of the target.

Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes. | Boolean | optional | False | | include_transitive_sources | When True, transitive_sources from JsInfo providers in srcs targets are included in the default outputs of the target. | Boolean | optional | True | | srcs | List of targets to gather files from. | List of labels | optional | [] | diff --git a/js/private/js_filegroup.bzl b/js/private/js_filegroup.bzl index 1870d1ecc..9c6df1426 100644 --- a/js/private/js_filegroup.bzl +++ b/js/private/js_filegroup.bzl @@ -10,13 +10,13 @@ This helper rule is used by the `js_run_binary` macro. def _js_filegroup_impl(ctx): return DefaultInfo(files = _gather_files_from_js_providers( targets = ctx.attr.srcs, + include_sources = ctx.attr.include_sources, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_transitive_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, )) -# TODO(2.0): Add an include_direct_sources & include_direct_declarations & include_direct_npm_linked_packages and -# rename include_declarations to include_transitive declarations & include_npm_linked_packages to include_transitive_npm_linked_packages. js_filegroup = rule( doc = _DOC, implementation = _js_filegroup_impl, @@ -25,14 +25,25 @@ js_filegroup = rule( doc = """List of targets to gather files from.""", allow_files = True, ), + "include_sources": attr.bool( + doc = """When True, `sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target.""", + default = True, + ), "include_transitive_sources": attr.bool( doc = """When True, `transitive_sources` from `JsInfo` providers in `srcs` targets are included in the default outputs of the target.""", default = True, ), "include_declarations": attr.bool( - doc = """When True, `declarations` and `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. + doc = """When True, `declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. + + Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip + time due to having to generate typings on source file changes.""", + default = False, + ), + "include_transitive_declarations": attr.bool( + doc = """When True, `transitive_declarations` from `JsInfo` providers in srcs targets are included in the default outputs of the target. - Defaults to false since declarations are generally not needed at runtime and introducing them could slow down developer round trip + Defaults to False since declarations are generally not needed at runtime and introducing them could slow down developer round trip time due to having to generate typings on source file changes.""", default = False, ), diff --git a/js/private/js_helpers.bzl b/js/private/js_helpers.bzl index 862ea9033..0e766ae3e 100644 --- a/js/private/js_helpers.bzl +++ b/js/private/js_helpers.bzl @@ -174,7 +174,19 @@ this option is not needed. return copy_file_to_bin_action(ctx, file) -def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_to_bin = False, no_copy_to_bin = [], include_transitive_sources = True, include_declarations = False, include_npm_linked_packages = True): +def gather_runfiles( + ctx, + sources, + data, + deps, + data_files = [], + copy_data_files_to_bin = False, + no_copy_to_bin = [], + include_sources = True, + include_transitive_sources = True, + include_declarations = False, + include_transitive_declarations = False, + include_npm_linked_packages = True): """Creates a runfiles object containing files in `sources`, default outputs from `data` and transitive runfiles from `data` & `deps`. As a defense in depth against `data` & `deps` targets not supplying all required runfiles, also @@ -210,10 +222,14 @@ def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_t file such as a file in an external repository. In most cases, this option is not needed. See `copy_data_files_to_bin` docstring for more info. + include_sources: see js_filegroup documentation + include_transitive_sources: see js_filegroup documentation include_declarations: see js_filegroup documentation + include_transitive_declarations: see js_filegroup documentation + include_npm_linked_packages: see js_filegroup documentation Returns: @@ -231,12 +247,13 @@ def gather_runfiles(ctx, sources, data, deps, data_files = [], copy_data_files_t for target in data ]) - # Gather the transitive sources & transitive npm linked packages from the JsInfo & - # NpmPackageStoreInfo providers of data & deps targets. + # Gather files from JsInfo providers of data & deps transitive_files_depsets.append(gather_files_from_js_providers( targets = data + deps, + include_sources = include_sources, include_transitive_sources = include_transitive_sources, include_declarations = include_declarations, + include_transitive_declarations = include_transitive_declarations, include_npm_linked_packages = include_npm_linked_packages, )) @@ -296,25 +313,31 @@ def envs_for_log_level(log_level): def gather_files_from_js_providers( targets, + include_sources, include_transitive_sources, include_declarations, + include_transitive_declarations, include_npm_linked_packages): """Gathers files from JsInfo and NpmPackageStoreInfo providers. Args: targets: list of target to gather from + include_sources: see js_filegroup documentation include_transitive_sources: see js_filegroup documentation include_declarations: see js_filegroup documentation + include_transitive_declarations: see js_filegroup documentation include_npm_linked_packages: see js_filegroup documentation Returns: A depset of files """ - files_depsets = [ - target[JsInfo].sources - for target in targets - if JsInfo in target and hasattr(target[JsInfo], "sources") - ] + files_depsets = [] + if include_sources: + files_depsets = [ + target[JsInfo].sources + for target in targets + if JsInfo in target and hasattr(target[JsInfo], "sources") + ] if include_transitive_sources: files_depsets.extend([ target[JsInfo].transitive_sources @@ -322,6 +345,12 @@ def gather_files_from_js_providers( if JsInfo in target and hasattr(target[JsInfo], "transitive_sources") ]) if include_declarations: + files_depsets.extend([ + target[JsInfo].declarations + for target in targets + if JsInfo in target and hasattr(target[JsInfo], "declarations") + ]) + if include_transitive_declarations: files_depsets.extend([ target[JsInfo].transitive_declarations for target in targets diff --git a/js/private/js_run_devserver.bzl b/js/private/js_run_devserver.bzl index 00950c5df..433fd2579 100644 --- a/js/private/js_run_devserver.bzl +++ b/js/private/js_run_devserver.bzl @@ -42,8 +42,10 @@ def _js_run_devserver_impl(ctx): transitive_runfiles = [_gather_files_from_js_providers( targets = ctx.attr.data, + include_sources = True, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, )] diff --git a/js/private/test/create_launcher/custom_test.bzl b/js/private/test/create_launcher/custom_test.bzl index 2e1a436e9..a625b34be 100644 --- a/js/private/test/create_launcher/custom_test.bzl +++ b/js/private/test/create_launcher/custom_test.bzl @@ -21,8 +21,10 @@ def _custom_test_impl(ctx): files = ctx.files.data, transitive_files = js_lib_helpers.gather_files_from_js_providers( targets = ctx.attr.data, + include_sources = True, include_transitive_sources = ctx.attr.include_transitive_sources, include_declarations = ctx.attr.include_declarations, + include_transitive_declarations = ctx.attr.include_declarations, include_npm_linked_packages = ctx.attr.include_npm_linked_packages, ), ).merge(launcher.runfiles).merge_all([