From 9cda5335f89596abb34d004533e91e9e7edaf546 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 3 Jul 2019 10:43:29 +0200 Subject: [PATCH 1/3] rule_test: apply "tags" to all rules in the macro RELNOTES: rule_test: fix Bazel 0.27 regression ("tags" attribute was ingored, https://github.com/bazelbuild/bazel/issues/8723 Fixes https://github.com/bazelbuild/bazel/issues/8723 Change-Id: I25ac338e4978084085b8c49d6d0a1c47d8dc4fd1 --- src/test/shell/bazel/rule_test_test.sh | 45 +++++++++++++++ tools/build_rules/test_rules.bzl | 77 +++++++++++++++++--------- 2 files changed, 96 insertions(+), 26 deletions(-) diff --git a/src/test/shell/bazel/rule_test_test.sh b/src/test/shell/bazel/rule_test_test.sh index 2fb41a64c8ffd1..b54ba2c46a5f0b 100755 --- a/src/test/shell/bazel/rule_test_test.sh +++ b/src/test/shell/bazel/rule_test_test.sh @@ -175,4 +175,49 @@ EOF bazel build //:turtle_rule_test &> $TEST_log || fail "turtle_rule_test failed" } +# Regression test for https://github.com/bazelbuild/bazel/issues/8723 +# +# rule_test() is a macro that expands to a sh_test and _rule_test_rule. +# Expect that rule_tags(..., tags) is applied to both rules, but rule_tags(..., visibility) is only +# applied to the sh_test because _rule_test_rule should be private. +function test_kwargs_with_macro_rules() { + create_new_workspace + cat > BUILD <<'EOF' +load("@bazel_tools//tools/build_rules:test_rules.bzl", "rule_test") + +genrule( + name = "x", + srcs = ["@does_not_exist//:bad"], + outs = ["x.out"], + cmd = "touch $@", + tags = ["dont_build_me"], +) + +rule_test( + name = "x_test", + rule = "//:x", + generates = ["x.out"], + visibility = ["//foo:__pkg__"], + tags = ["dont_build_me"], +) +EOF + + bazel build --build_tag_filters=-dont_build_me //:all >& "$TEST_log" || fail "build failed" + + bazel query --output=label 'attr(tags, dont_build_me, //:all)' >& "$TEST_log" || fail "query failed" + expect_log '//:x_test_impl' + expect_log '//:x_test\b' + expect_log '//:x\b' + + bazel query --output=label 'attr(visibility, private, //:all)' >& "$TEST_log" || fail "query failed" + expect_log '//:x_test_impl' + expect_not_log '//:x_test\b' + expect_not_log '//:x\b' + + bazel query --output=label 'attr(visibility, foo, //:all)' >& "$TEST_log" || fail "query failed" + expect_log '//:x_test\b' + expect_not_log '//:x_test_impl' + expect_not_log '//:x\b' +} + run_suite "rule_test tests" diff --git a/tools/build_rules/test_rules.bzl b/tools/build_rules/test_rules.bzl index 53079cbae0fa0d..ac1cf6ceda96bd 100644 --- a/tools/build_rules/test_rules.bzl +++ b/tools/build_rules/test_rules.bzl @@ -35,6 +35,12 @@ def _make_sh_test(name, **kwargs): **kwargs ) +def _merge_dicts(d1, d2): + r = {} + r.update(d1) + r.update(d2) + return r + ### First, trivial tests that either always pass, always fail, ### or sometimes pass depending on a trivial computation. @@ -75,11 +81,16 @@ _successful_rule = rule( def successful_test(name, msg, **kwargs): _successful_rule( - name = name + "_impl", - msg = msg, - out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], + **_merge_dicts( + kwargs, + dict( + name = name + "_impl", + msg = msg, + out = name + "_impl.sh", + testonly = 1, + visibility = ["//visibility:private"], + ), + ) ) _make_sh_test(name, **kwargs) @@ -121,11 +132,16 @@ _failed_rule = rule( def failed_test(name, msg, **kwargs): _failed_rule( - name = name + "_impl", - msg = msg, - out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], + **_merge_dicts( + kwargs, + dict( + name = name + "_impl", + msg = msg, + out = name + "_impl.sh", + testonly = 1, + visibility = ["//visibility:private"], + ), + ) ) _make_sh_test(name, **kwargs) @@ -306,13 +322,18 @@ _rule_test_rule = rule( def rule_test(name, rule, generates = None, provides = None, **kwargs): _rule_test_rule( - name = name + "_impl", - rule = rule, - generates = generates, - provides = provides, - out = name + ".sh", - testonly = 1, - visibility = ["//visibility:private"], + **_merge_dicts( + kwargs, + dict( + name = name + "_impl", + rule = rule, + generates = generates, + provides = provides, + out = name + ".sh", + testonly = 1, + visibility = ["//visibility:private"], + ), + ) ) _make_sh_test(name, **kwargs) @@ -372,14 +393,18 @@ _file_test_rule = rule( def file_test(name, file, content = None, regexp = None, matches = None, **kwargs): _file_test_rule( - name = name + "_impl", - file = file, - content = content or "", - regexp = regexp or "", - matches = matches if (matches != None) else -1, - out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], + **_merge_dicts( + kwargs, + dict( + name = name + "_impl", + file = file, + content = content or "", + regexp = regexp or "", + matches = matches if (matches != None) else -1, + out = name + "_impl.sh", + testonly = 1, + visibility = ["//visibility:private"], + ), + ) ) - _make_sh_test(name, **kwargs) From eabd39366f588d24415c117fe3fac8309fcc7367 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Wed, 3 Jul 2019 11:14:07 +0200 Subject: [PATCH 2/3] Add a negative test Change-Id: Iee65afe1b7b8d3f2e09057804249396193ed241b --- src/test/shell/bazel/rule_test_test.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/shell/bazel/rule_test_test.sh b/src/test/shell/bazel/rule_test_test.sh index b54ba2c46a5f0b..389517749c7d83 100755 --- a/src/test/shell/bazel/rule_test_test.sh +++ b/src/test/shell/bazel/rule_test_test.sh @@ -202,6 +202,8 @@ rule_test( ) EOF + bazel build //:all >& "$TEST_log" && fail "should have failed" || true + bazel build --build_tag_filters=-dont_build_me //:all >& "$TEST_log" || fail "build failed" bazel query --output=label 'attr(tags, dont_build_me, //:all)' >& "$TEST_log" || fail "query failed" From 0c222880322a5e558e469ce8f1e6eeb7e440f4a5 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Thu, 4 Jul 2019 09:31:26 +0200 Subject: [PATCH 3/3] Filter test-only args from the macro's build rule Change-Id: Ie53b8787ab8679ff4896820ef552c2696eac1d09 --- src/test/shell/bazel/rule_test_test.sh | 12 +++++++-- tools/build_rules/test_rules.bzl | 37 +++++++++++++++----------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/test/shell/bazel/rule_test_test.sh b/src/test/shell/bazel/rule_test_test.sh index 389517749c7d83..195dc2bb27b761 100755 --- a/src/test/shell/bazel/rule_test_test.sh +++ b/src/test/shell/bazel/rule_test_test.sh @@ -178,8 +178,10 @@ EOF # Regression test for https://github.com/bazelbuild/bazel/issues/8723 # # rule_test() is a macro that expands to a sh_test and _rule_test_rule. -# Expect that rule_tags(..., tags) is applied to both rules, but rule_tags(..., visibility) is only -# applied to the sh_test because _rule_test_rule should be private. +# Expect that: +# * test- and build-rule attributes (e.g. "tags") are applied to both rules, +# * test-only attributes are applied only to the sh_rule, +# * the build rule has its own visibility function test_kwargs_with_macro_rules() { create_new_workspace cat > BUILD <<'EOF' @@ -199,6 +201,12 @@ rule_test( generates = ["x.out"], visibility = ["//foo:__pkg__"], tags = ["dont_build_me"], + args = ["x"], + flaky = False, + local = True, + shard_count = 2, + size = "small", + timeout = "short", ) EOF diff --git a/tools/build_rules/test_rules.bzl b/tools/build_rules/test_rules.bzl index ac1cf6ceda96bd..ef26b8f40d6187 100644 --- a/tools/build_rules/test_rules.bzl +++ b/tools/build_rules/test_rules.bzl @@ -35,10 +35,25 @@ def _make_sh_test(name, **kwargs): **kwargs ) -def _merge_dicts(d1, d2): +_TEST_ATTRS = { + "args": None, + "size": None, + "timeout": None, + "flaky": None, + "local": None, + "shard_count": None, +} + +def _helper_rule_attrs(test_attrs, own_attrs): r = {} - r.update(d1) - r.update(d2) + r.update({k: v for k, v in test_attrs.items() if k not in _TEST_ATTRS}) + r.update(own_attrs) + r.update( + dict( + testonly = 1, + visibility = ["//visibility:private"], + ), + ) return r ### First, trivial tests that either always pass, always fail, @@ -81,14 +96,12 @@ _successful_rule = rule( def successful_test(name, msg, **kwargs): _successful_rule( - **_merge_dicts( + **_helper_rule_attrs( kwargs, dict( name = name + "_impl", msg = msg, out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], ), ) ) @@ -132,14 +145,12 @@ _failed_rule = rule( def failed_test(name, msg, **kwargs): _failed_rule( - **_merge_dicts( + **_helper_rule_attrs( kwargs, dict( name = name + "_impl", msg = msg, out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], ), ) ) @@ -322,7 +333,7 @@ _rule_test_rule = rule( def rule_test(name, rule, generates = None, provides = None, **kwargs): _rule_test_rule( - **_merge_dicts( + **_helper_rule_attrs( kwargs, dict( name = name + "_impl", @@ -330,8 +341,6 @@ def rule_test(name, rule, generates = None, provides = None, **kwargs): generates = generates, provides = provides, out = name + ".sh", - testonly = 1, - visibility = ["//visibility:private"], ), ) ) @@ -393,7 +402,7 @@ _file_test_rule = rule( def file_test(name, file, content = None, regexp = None, matches = None, **kwargs): _file_test_rule( - **_merge_dicts( + **_helper_rule_attrs( kwargs, dict( name = name + "_impl", @@ -402,8 +411,6 @@ def file_test(name, file, content = None, regexp = None, matches = None, **kwarg regexp = regexp or "", matches = matches if (matches != None) else -1, out = name + "_impl.sh", - testonly = 1, - visibility = ["//visibility:private"], ), ) )