Skip to content

Commit

Permalink
fix: allow "src" and "bin" module mappings to win over "runfiles"
Browse files Browse the repository at this point in the history
This fixes an issue in the Angular repo where the duplicate `runfiles` mappings:

```
["runfiles", "angular/packages/compiler"] and ["bin", "angular/packages/compiler"]
```

sneaks through this nodejs_binaries:

```
nodejs_binary(
name = "ngc_bin",
data = [
"//packages/compiler-cli",
"@npm//chokidar",
"@npm//reflect-metadata",
],
entry_point = "//packages/compiler-cli:src/main.ts",
)

nodejs_binary(
name = "ng_xi18n",
data = [
"//packages/compiler-cli",
"@npm//chokidar",
"@npm//reflect-metadata",
],
entry_point = "//packages/compiler-cli:src/extract_i18n.ts",
)
```

which depends on `//packages/compiler-cli` which depends on `//packages/compiler`. So when building the ng_test target:

```
nodejs_test(
name = "integrationtest",
data = [
":ngc_bin",
":ng_xi18n",
"@nodejs//:node",
"@npm//domino",
"@npm//chokidar",
"@npm//source-map-support",
"@npm//shelljs",
"@npm//typescript",
"@npm//reflect-metadata",
"@npm//rxjs",
"@npm//tslib",
"@npm//jasmine/bin:jasmine",
"@npm//xhr2",
"@npm//@types/node",
"@npm//@types/jasmine",
# we need to reference zone.d.ts typing file from zone.js build target
# instead of npm because angular repo will not depends on npm zone.js
# any longer.
"//packages/zone.js/lib:zone_d_ts",
# we need to reference zone.js npm_package build target
# instead of npm because angular repo will not depends on npm zone.js
# any longer, so we need to build a zone.js npm release first.
"//packages/zone.js:npm_package",
"//packages/animations:npm_package",
"//packages/common:npm_package",
"//packages/compiler:npm_package",
"//packages/compiler-cli:npm_package",
"//packages/core:npm_package",
"//packages/forms:npm_package",
"//packages/http:npm_package",
"//packages/platform-browser:npm_package",
"//packages/platform-browser-dynamic:npm_package",
"//packages/platform-server:npm_package",
"//packages/router:npm_package",
] + glob(["**/*"]),
entry_point = "test.js",
tags = ["no-ivy-aot"],
)
```

it has both mappings causing the linker to error out with conflicting module mappings `["runfiles", "angular/packages/compiler"] and ["bin", "angular/packages/compiler"]`.

Fix is to just allow this and allow both "src" and "bin" to win a conflicting over "runfiles" mapping.
  • Loading branch information
gregmagolan committed Jan 29, 2020
1 parent 765914e commit 110e00e
Showing 1 changed file with 24 additions and 17 deletions.
41 changes: 24 additions & 17 deletions internal/linker/link_node_modules.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,21 @@ def add_arg(args, arg):
else:
args.add(arg)

def _link_mapping(label, mappings, k, v):
if k in mappings and mappings[k] != v:
if v[1] == mappings[k][1]:
# Allow "src" and "bin" to win over "runfiles"
# For example,
# mappings[k] = ["runfiles", "angular/packages/compiler"]
# v = ["bin", "angular/packages/compiler"]
if mappings[k][0] == "runfiles":
return True
elif v[0] != "runfiles":
return False
fail(("conflicting mapping at %s: %s maps to both %s and %s" % (label, k, mappings[k], v)), "deps")
else:
return True

def write_node_modules_manifest(ctx, extra_data = []):
"""Writes a manifest file read by the linker, containing info about resolving runtime dependencies
Expand Down Expand Up @@ -62,11 +77,9 @@ def write_node_modules_manifest(ctx, extra_data = []):

# ...first-party packages to be linked into the node_modules tree
for k, v in getattr(dep, _ASPECT_RESULT_NAME, {}).items():
if k in mappings and mappings[k] != v:
fail(("conflicting module mapping at %s: %s maps to both %s and %s" %
(dep.label, k, mappings[k], v)), "deps")
_debug(ctx.var, "Linking %s: %s" % (k, v))
mappings[k] = v
if _link_mapping(dep.label, mappings, k, v):
_debug(ctx.var, "Linking %s: %s" % (k, v))
mappings[k] = v

# Write the result to a file, and use the magic node option --bazel_node_modules_manifest
# The launcher.sh will peel off this argument and pass it to the linker rather than the program.
Expand Down Expand Up @@ -102,11 +115,6 @@ def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name
for name in _MODULE_MAPPINGS_DEPS_NAMES:
for dep in getattr(attrs, name, []):
for k, v in getattr(dep, _ASPECT_RESULT_NAME, {}).items():
if k in mappings and mappings[k] != v:
fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
(label, k, mappings[k], v)), "deps")
_debug(vars, "target %s propagating module mapping %s: %s" % (dep, k, v))

# A package which was reachable transitively via a *_binary or *_test
# rule is assumed to be in the runfiles of that binary,
# so we switch the linker target root.
Expand All @@ -115,9 +123,10 @@ def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name
# but it seems that this info is only available in Bazel Java internals.
# TODO: revisit whether this is the correct condition after downstream testing
if rule_kind.endswith("_binary") or rule_kind.endswith("_test"):
mappings[k] = ["runfiles", v[1]]
else:
v = ["runfiles", v[1]]
if _link_mapping(label, mappings, k, v):
mappings[k] = v
_debug(vars, "target %s propagating module mapping %s: %s" % (dep, k, v))

if not getattr(attrs, "module_name", None) and not getattr(attrs, "module_root", None):
# No mappings contributed here, short-circuit with the transitive ones we collected
Expand All @@ -141,12 +150,10 @@ def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name
else:
mr = ["bin", mr]

if mn in mappings and mappings[mn] != mr:
fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
(label, mn, mappings[mn], mr)), "deps")
_debug(vars, "target %s adding module mapping %s: %s" % (label, mn, mr))
if _link_mapping(label, mappings, mn, mr):
_debug(vars, "target %s adding module mapping %s: %s" % (label, mn, mr))
mappings[mn] = mr

mappings[mn] = mr
return mappings

# When building a mapping for use at runtime, we need paths to be relative to
Expand Down

0 comments on commit 110e00e

Please sign in to comment.