Skip to content
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.

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

Pass through apple_split_cpu #196

Merged
merged 2 commits into from
Jan 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion rules/transition_support.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def _apple_rule_transition_impl(settings, attr):
ret = {
"//command_line_option:apple configuration distinguisher": "applebin_" + platform_type,
"//command_line_option:apple_platform_type": platform_type,
"//command_line_option:apple_split_cpu": "",
"//command_line_option:apple_split_cpu": settings["//command_line_option:apple_split_cpu"],
"//command_line_option:compiler": settings["//command_line_option:apple_compiler"],
"//command_line_option:cpu": _cpu_string(platform_type, settings),
"//command_line_option:crosstool_top": (
Expand Down Expand Up @@ -104,6 +104,7 @@ _apple_rule_transition = transition(
"//command_line_option:macos_cpus",
"//command_line_option:tvos_cpus",
"//command_line_option:watchos_cpus",
"//command_line_option:apple_split_cpu",
],
outputs = [
"//command_line_option:apple configuration distinguisher",
Expand Down
3 changes: 3 additions & 0 deletions tests/ios/app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ load("//rules:framework.bzl", "apple_framework")
load("//rules:app.bzl", "ios_application")
load("//rules:test.bzl", "ios_unit_test")
load("@bazel_skylib//rules:common_settings.bzl", "string_flag")
load(":analysis-tests.bzl", "make_tests")

apple_framework(
name = "CppLib",
Expand Down Expand Up @@ -280,3 +281,5 @@ ios_application(
":SwiftLib",
],
)

make_tests()
Copy link
Contributor

Choose a reason for hiding this comment

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

is it that we can add more tests under this method in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly a target can't be instantiated from a .bzl file, and I went off the convention of putting the analysis test definitions adjacent to the impl in the .bzl file to keep the BUILD file minimal. Also a nice place to put more instances of _identical_outputs_test e.g. for app VS test transitive dep consistency.

100 changes: 100 additions & 0 deletions tests/ios/app/analysis-tests.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts")

_TestFiles = provider(
fields = {
"files": "A glob of files collected for later assertions",
},
)

def _identical_outputs_test_impl(ctx):
env = analysistest.begin(ctx)
all_files = []
asserts.true(env, len(ctx.attr.deps) > 1)

for dep in ctx.attr.deps:
if not _TestFiles in dep:
continue

# The input root is the part of the file usually rooted in bazel-out.
# For starlark transitions output dirs are fingerprinted by the hash of the
# relevant configuration keys.
# src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java
for input in dep[_TestFiles].files.to_list():
all_files.append(input.root.path)

# Expect that we have recieved multiple swiftmodules
asserts.true(env, len(all_files) > 1)

# Assert all swiftmodules have identical outputs ( and most importantly an
# identical output directory )
asserts.equals(env, 1, len(depset(all_files).to_list()))
return analysistest.end(env)

def _collect_transitive_outputs_impl(target, ctx):
# Collect trans swift_library outputs
out = []
if ctx.rule.kind == "swift_library":
out.extend(target[DefaultInfo].files.to_list())

if _TestFiles in target:
out.extend(target[_TestFiles].files.to_list())
if hasattr(ctx.rule.attr, "srcs"):
for d in ctx.rule.attr.srcs:
if _TestFiles in d:
out.extend(d[_TestFiles].files.to_list())
if hasattr(ctx.rule.attr, "deps"):
for d in ctx.rule.attr.deps:
if _TestFiles in d:
out.extend(d[_TestFiles].files.to_list())
return _TestFiles(files = depset(out))

_collect_transitive_outputs = aspect(
implementation = _collect_transitive_outputs_impl,
attr_aspects = ["deps", "srcs"],
)

_identical_outputs_test = analysistest.make(
_identical_outputs_test_impl,
expect_failure = False,
attrs = {
"deps": attr.label_list(
aspects = [_collect_transitive_outputs],
),
},
)

def make_tests():
# This test asserts that transitive dependencies have identical outputs for
Copy link
Contributor

Choose a reason for hiding this comment

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

To @gyfelton's question, also curious to know how we'll add more tests here. Should make_tests hold all of them? If that's the case (nit) should the test description be moved into _identical_outputs_test ?

Copy link
Contributor Author

@jerrymarino jerrymarino Jan 19, 2021

Choose a reason for hiding this comment

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

I added a: #196 (comment). If you follow the pattern I setup you can add a test for apps / test bundles like:

make_tests():

..... 
    _identical_outputs_test(
        name = "test_TestHostEquivilance",
        target_under_test = ":TestAppWithSelectableCopts",

        # These inputs *must* be passed seperately, in order to
        # have different transitions applied by Skyframe.
        deps = [":TestAppWithSelectableCopts", ":AppWithSelectableCopts"],
    )

but _identical_outputs_test cannot be extended as it uses Bazel's analyiss test analysistest.make

# different transition paths. In particular, a rules_apple ios_application and an a
# apple_framework that share a swift_library, :SwiftLib_swift. This test ensures
# that the actions in both builds have functionally equal transitions
# applied by normalizing their output directories into a set.
#
# For instance these tests will fail if there is any delta and requires both:
# - adding apple_common.multi_arch_split to apple_framework.deps - #188
# - the transition yields the same result when used w/rules_apple - #196

# Note:
# The gist of Bazel's configuration resolver is that it will apply
# relevant transitions to keys that are used by a given action. e.g. ios_multi_cpus.
# src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
#
# In order to get the same configuration for a rule, a given transition has
# to produce the same values for dependent keys for all possible combinations
# of edges

_identical_outputs_test(
name = "test_DependencyEquivilance",
target_under_test = ":AppWithSelectableCopts",

# These inputs *must* be passed seperately, in order to
# have different transitions applied by Skyframe.
deps = [":AppWithSelectableCopts", ":SwiftLib"],
)

native.test_suite(
name = "AnalysisTests",
tests = [
":test_DependencyEquivilance",
],
)
Loading