Skip to content

Replace legacy struct providers with modern ones #599

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Replace legacy struct providers with modern ones #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6db403
Replace legacy struct providers with modern ones
comius Jan 17, 2024
daf6131
Merge branch 'master' into cleanup-legacy-providers
gkdn Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions closure/compiler/closure_js_aspect.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
load(
"//closure/private:defs.bzl",
"CLOSURE_WORKER_ATTR",
"ClosureJsLegacyRunfilesInfo",
"ClosureJsLibraryInfo",
"JS_FILE_TYPE",
"collect_js",
"collect_runfiles",
Expand All @@ -26,13 +28,13 @@ load(
)

def _closure_js_aspect_impl(target, ctx):
if hasattr(target, "closure_js_library"):
return struct()
if ClosureJsLibraryInfo in target:
return []

# This aspect is currently a no-op in the open source world. We intend to add
# content to it in the future. It is still provided to ensure the Skylark API
# is well defined.
return struct()
return []

closure_js_aspect = aspect(
implementation = _closure_js_aspect_impl,
Expand Down
24 changes: 13 additions & 11 deletions closure/compiler/closure_js_binary.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
load(
"//closure/private:defs.bzl",
"CLOSURE_JS_TOOLCHAIN_ATTRS",
"ClosureCssBinaryInfo",
"ClosureJsBinaryInfo",
"ClosureJsLibraryInfo",
"JS_LANGUAGES",
"JS_LANGUAGE_IN",
"JS_LANGUAGE_OUT_DEFAULT",
Expand Down Expand Up @@ -78,7 +81,7 @@ def _impl(ctx):
", ".join(JS_LANGUAGES.to_list()),
))

deps = unfurl(ctx.attr.deps, provider = "closure_js_library")
deps = unfurl(ctx.attr.deps, provider = ClosureJsLibraryInfo).exports
js = collect_js(deps, ctx.attr._closure_library_base, css = ctx.attr.css)
if not js.srcs:
fail("There are no JS source files in the transitive closure")
Expand Down Expand Up @@ -281,27 +284,26 @@ def _impl(ctx):
# promise to compile. Its fulfillment is the prerogative of ancestors which
# are free to ignore the binary in favor of the raw sauces propagated by the
# closure_js_library provider, in which case, no compilation is performed.
return struct(
files = depset(files),
closure_js_library = js,
closure_js_binary = struct(
return [
ClosureJsBinaryInfo(
bin = ctx.outputs.bin,
map = ctx.outputs.map,
language = ctx.attr.language,
),
runfiles = ctx.runfiles(
js,
DefaultInfo(files = depset(files), runfiles = ctx.runfiles(
files = files + ctx.files.data,
transitive_files = depset(transitive = [
collect_runfiles(deps),
collect_runfiles([ctx.attr.css]),
collect_runfiles(ctx.attr.data),
]),
),
)
)),
]

def _validate_css_graph(ctx, js):
if ctx.attr.css:
missing = difference(js.stylesheets, ctx.attr.css.closure_css_binary.labels)
missing = difference(js.stylesheets, ctx.attr.css[ClosureCssBinaryInfo].labels)
if missing:
fail("Dependent JS libraries depend on CSS libraries that weren't " +
"compiled into the referenced CSS binary: " +
Expand All @@ -315,7 +317,7 @@ closure_js_binary = rule(
implementation = _impl,
attrs = dict({
"compilation_level": attr.string(default = "ADVANCED"),
"css": attr.label(providers = ["closure_css_binary"]),
"css": attr.label(providers = [ClosureCssBinaryInfo]),
"debug": attr.bool(default = False),
"defs": attr.string_list(),
# TODO(tjgq): Remove the deprecated STRICT/LOOSE in favor of PRUNE/PRUNE_LEGACY.
Expand All @@ -332,7 +334,7 @@ closure_js_binary = rule(
),
"deps": attr.label_list(
aspects = [closure_js_aspect],
providers = ["closure_js_library"],
providers = [ClosureJsLibraryInfo],
),
"entry_points": attr.string_list(),
"formatting": attr.string(),
Expand Down
48 changes: 25 additions & 23 deletions closure/compiler/closure_js_library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
load(
"//closure/private:defs.bzl",
"CLOSURE_JS_TOOLCHAIN_ATTRS",
"ClosureCssLibraryInfo",
"ClosureJsLibraryInfo",
"JS_FILE_TYPE",
"JS_LANGUAGE_IN",
"collect_js",
Expand Down Expand Up @@ -163,7 +165,7 @@ def _closure_js_library_impl(

# Create a list of direct children of this rule. If any direct dependencies
# have the exports attribute, those labels become direct dependencies here.
deps = unfurl(deps, provider = "closure_js_library")
deps = unfurl(deps, provider = ClosureJsLibraryInfo).exports

# Collect all the transitive stuff the child rules have propagated. Bazel has
# a special nested set data structure that makes this efficient.
Expand All @@ -177,7 +179,7 @@ def _closure_js_library_impl(
# which is a superset of the CSS libraries in its transitive closure.
stylesheets = []
for dep in deps:
if hasattr(dep, "closure_css_library"):
if ClosureCssLibraryInfo in dep:
stylesheets.append(dep.label)

# JsChecker is a program that's run via the ClosureWorker persistent Bazel
Expand Down Expand Up @@ -263,7 +265,7 @@ def _closure_js_library_impl(
info_files = []
for dep in deps:
# Polymorphic rules, e.g. closure_css_library, might not provide this.
info = getattr(dep.closure_js_library, "info", None)
info = getattr(dep[ClosureJsLibraryInfo], "info", None)
if info:
args.add("--dep", info)
info_files.append(info)
Expand Down Expand Up @@ -303,7 +305,7 @@ def _closure_js_library_impl(
# interface because other Skylark rules can be designed to do things with
# this data. Other Skylark rules can even export their own provider with the
# same name to become polymorphically compatible with this one.
return struct(
return [
# Iterable<Target> of deps that should only become deps in parent rules.
# Exports are not deps of the Target to which they belong. The exports
# provider does not contain the exports its deps export. Targets in this
Expand All @@ -320,10 +322,10 @@ def _closure_js_library_impl(
# the exports attribute does not exist. The exports feature can be abused
# by users to circumvent strict deps checking and therefore should be
# used with caution.
exports = unfurl(exports),
unfurl(exports),
# All of the subproviders below are considered optional and MUST be
# accessed using getattr(x, y, default). See collect_js() in defs.bzl.
closure_js_library = struct(
ClosureJsLibraryInfo(
# File pointing to a ClosureJsLibrary protobuf file in pbtxt format
# that's generated by this specific Target. It contains some metadata
# as well as information extracted from inside the srcs files, e.g.
Expand Down Expand Up @@ -366,7 +368,7 @@ def _closure_js_library_impl(
# of the srcs subprovider. This field exists for optimization.
has_closure_library = js.has_closure_library,
),
)
]

def _closure_js_library(ctx):
if not ctx.files.srcs and not ctx.files.externs and not ctx.attr.exports:
Expand Down Expand Up @@ -407,22 +409,22 @@ def _closure_js_library(ctx):
ctx.outputs.typecheck,
)

return struct(
files = depset(),
exports = library.exports,
closure_js_library = library.closure_js_library,
# The usual suspects are exported as runfiles, in addition to raw source.
runfiles = ctx.runfiles(
files = srcs + ctx.files.data,
transitive_files = depset(
transitive = [
collect_runfiles(unfurl(ctx.attr.deps, provider = "closure_js_library")),
collect_runfiles(ctx.attr.data),
collect_runfiles(library.exports),
],
return library + [
DefaultInfo(
files = depset(),
# The usual suspects are exported as runfiles, in addition to raw source.
runfiles = ctx.runfiles(
files = srcs + ctx.files.data,
transitive_files = depset(
transitive = [
collect_runfiles(unfurl(ctx.attr.deps, provider = ClosureJsLibraryInfo).exports),
collect_runfiles(ctx.attr.data),
collect_runfiles(unfurl(ctx.attr.exports).exports),
],
),
),
),
)
]

closure_js_library = rule(
implementation = _closure_js_library,
Expand All @@ -435,11 +437,11 @@ closure_js_library = rule(
"data": attr.label_list(allow_files = True),
"deps": attr.label_list(
aspects = [closure_js_aspect],
providers = ["closure_js_library"],
providers = [ClosureJsLibraryInfo],
),
"exports": attr.label_list(
aspects = [closure_js_aspect],
providers = ["closure_js_library"],
providers = [ClosureJsLibraryInfo],
),
"includes": attr.string_list(),
"no_closure_library": attr.bool(),
Expand Down
124 changes: 101 additions & 23 deletions closure/private/defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,84 @@ CLOSURE_JS_TOOLCHAIN_ATTRS = {
"_unusable_type_definition": UNUSABLE_TYPE_DEFINITION,
}

ClosureJsLibraryInfo = provider("ClosureJsLibraryInfo", fields = {
"info": """
File pointing to a ClosureJsLibrary protobuf file in pbtxt format
that's generated by this specific Target. It contains some metadata
as well as information extracted from inside the srcs files, e.g.
goog.provide'd namespaces. It is used for strict dependency
checking, a.k.a. layering checks.
""",
"infos": """
NestedSet<File> of all info files in the transitive closure. This
is used by JsCompiler to apply error suppression on a file-by-file
basis.""",
"ijs": "",
"ijs_files": "",
"srcs": """
NestedSet<File> of all JavaScript source File artifacts in the
transitive closure. These files MUST be JavaScript.""",
"js_module_roots": """
NestedSet<String> of all execroot path prefixes in the transitive
closure. For very simple projects, it will be empty. It is useful
for getting rid of Bazel generated directories, workspace names,
etc. out of module paths. It contains the cartesian product of
generated roots, external repository roots, and includes
prefixes. This is passed to JSCompiler via the --js_module_root
flag. See find_js_module_roots() in defs.bzl. """,
"modules": """
NestedSet<String> of all ES6 module name strings in the transitive
closure. These are generated from the source file path relative to
the longest matching root prefix. It is used to guarantee that
within any given transitive closure, no namespace collisions
exist. These MUST NOT begin with "/" or ".", or contain "..".""",
"descriptors": """
NestedSet<File> of all protobuf definitions in the transitive
closure. It is used so Closure Templates can have information about
the structure of protobufs so they can be easily rendered in .soy
files with type safety. See closure_js_template_library.bzl.""",
"stylesheets": """
NestedSet<Label> of all closure_css_library rules in the transitive
closure. This is used by closure_js_binary can guarantee the
completeness of goog.getCssName() substitutions.""",
"has_closure_library": """
Boolean indicating indicating if Closure Library's base.js is part
of the srcs subprovider. This field exists for optimization.""",
"language": "",
})

ClosureExportsInfo = provider("ClosureExportsInfo", fields = {"exports": """
Copy link
Collaborator

@gkdn gkdn Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are introducing proper providers, would it be possible to get rid of this as well?

Copy link
Contributor Author

@comius comius Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For ClosureExportsInfo, it looks possible to remove it, by adding exports to ClosuresJsLibraryInfo and WebFilesInfo.

ClosureJsLegacyRunfilesInfo looks easier to remove, by using regular runfiles instead.

Both of those removals require some effort and should probably be done by code owners and not as a part of a large cleanup, where we're fixing about 80 other rulesets.

Copy link
Collaborator

@gkdn gkdn Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's fair.

Iterable<Target> of deps that should only become deps in parent rules.
Exports are not deps of the Target to which they belong. The exports
provider does not contain the exports its deps export. Targets in this
provider are not necessarily guaranteed to have a closure_js_library
provider. Rules allowing closure_js_library deps MUST also treat
exports of those deps as direct dependencies of the Target. If those
rules are library rules, then they SHOULD also provide an exports
attribute of their own which is propagated to parent targets via the
exports provider, along with any exports those exports export. The
exports attribute MUST NOT contain files and SHOULD NOT impose
restrictions on what providers a Target must have. Rules exporting this
provider MUST NOT allow deps to be set if srcs is empty. Aspects
exporting this provider MAY turn deps into exports if srcs is empty and
the exports attribute does not exist. The exports feature can be abused
by users to circumvent strict deps checking and therefore should be
used with caution."""})

ClosureJsBinaryInfo = provider("ClosureJsBinaryInfo", fields = ["bin", "map", "language"])
ClosureCssBinaryInfo = provider("ClosureCssBinaryInfo", fields = ["bin", "map", "renaming_map", "labels"])

ClosureCssLibraryInfo = provider("ClosureCssLibraryInfo", fields = [
"srcs",
"labels",
"transitive",
"orientation",
])

ClosureJsLegacyRunfilesInfo = provider("ClosureJsLegacyRunfilesInfo", fields = ["runfiles"])

WebFilesInfo = provider("WebFilesInfo", fields = ["manifest", "manifests", "webpaths", "dummy"])

def get_jsfile_path(f):
"""Returns the file path for a JavaScript file, otherwise None.

Expand All @@ -74,13 +152,13 @@ def unfurl(deps, provider = ""):
"""Returns deps as well as deps exported by parent rules."""
res = []
for dep in deps:
if not provider or hasattr(dep, provider):
if not provider or provider in dep:
res.append(dep)
if hasattr(dep, "exports"):
for edep in dep.exports:
if not provider or hasattr(edep, provider):
if ClosureExportsInfo in dep:
for edep in dep[ClosureExportsInfo].exports:
if not provider or provider in edep:
res.append(edep)
return res
return ClosureExportsInfo(exports = res)

def collect_js(
deps,
Expand All @@ -98,16 +176,16 @@ def collect_js(
js_module_roots = []
has_closure_library = False
for dep in deps:
srcs += [getattr(dep.closure_js_library, "srcs", depset())]
ijs_files += [getattr(dep.closure_js_library, "ijs_files", depset())]
infos += [getattr(dep.closure_js_library, "infos", depset())]
modules += [getattr(dep.closure_js_library, "modules", depset())]
descriptors += [getattr(dep.closure_js_library, "descriptors", depset())]
stylesheets += [getattr(dep.closure_js_library, "stylesheets", depset())]
js_module_roots += [getattr(dep.closure_js_library, "js_module_roots", depset())]
srcs += [getattr(dep[ClosureJsLibraryInfo], "srcs", depset())]
ijs_files += [getattr(dep[ClosureJsLibraryInfo], "ijs_files", depset())]
infos += [getattr(dep[ClosureJsLibraryInfo], "infos", depset())]
modules += [getattr(dep[ClosureJsLibraryInfo], "modules", depset())]
descriptors += [getattr(dep[ClosureJsLibraryInfo], "descriptors", depset())]
stylesheets += [getattr(dep[ClosureJsLibraryInfo], "stylesheets", depset())]
js_module_roots += [getattr(dep[ClosureJsLibraryInfo], "js_module_roots", depset())]
has_closure_library = (
has_closure_library or
getattr(dep.closure_js_library, "has_closure_library", False)
getattr(dep[ClosureJsLibraryInfo], "has_closure_library", False)
)
if no_closure_library:
if has_closure_library:
Expand All @@ -117,9 +195,9 @@ def collect_js(
has_closure_library = True

if css:
direct_srcs += [css.closure_css_binary.renaming_map]
direct_srcs += [css[ClosureCssBinaryInfo].renaming_map]

return struct(
return ClosureJsLibraryInfo(
srcs = depset(direct_srcs, transitive = srcs),
js_module_roots = depset(transitive = js_module_roots),
ijs_files = depset(transitive = ijs_files),
Expand All @@ -135,14 +213,14 @@ def collect_css(deps, orientation = None):
srcs = []
labels = []
for dep in deps:
if hasattr(dep.closure_css_library, "srcs"):
srcs.append(getattr(dep.closure_css_library, "srcs"))
if hasattr(dep.closure_css_library, "labels"):
labels.append(getattr(dep.closure_css_library, "labels"))
if hasattr(dep[ClosureCssLibraryInfo], "srcs"):
srcs.append(getattr(dep[ClosureCssLibraryInfo], "srcs"))
if hasattr(dep[ClosureCssLibraryInfo], "labels"):
labels.append(getattr(dep[ClosureCssLibraryInfo], "labels"))
if orientation:
if dep.closure_css_library.orientation != orientation:
if dep[ClosureCssLibraryInfo].orientation != orientation:
fail("%s does not have the same orientation" % dep.label)
orientation = dep.closure_css_library.orientation
orientation = dep[ClosureCssLibraryInfo].orientation
return struct(
srcs = depset(transitive = srcs),
labels = depset(transitive = labels),
Expand All @@ -153,8 +231,8 @@ def collect_runfiles(targets):
"""Aggregates data runfiles from targets."""
data = []
for target in targets:
if hasattr(target, "closure_legacy_js_runfiles"):
data.append(target.closure_legacy_js_runfiles)
if target and ClosureJsLegacyRunfilesInfo in target:
data.append(target[ClosureJsLegacyRunfilesInfo].runfiles)
continue
if hasattr(target, "runfiles"):
data.append(target.runfiles.files)
Expand Down
Loading