-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fix and assert swift imported header propagation #333
Changes from all commits
b2a9946
5138482
83b6f11
052e72a
e9a8a5c
27aec62
d335037
656bb93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test doesn't assert the problem to have 1 dev because it asserts identical outputs amongst multiple This is the only way to ensure that bazelbuild/bazel#12171 isn't happening internal to a rule! |
||
|
||
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 | ||
Comment on lines
+18
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, thx for linking this 🙏 |
||
for input in dep[_TestFiles].files.to_list(): | ||
all_files.append(input.root.path) | ||
|
||
# Expect that we have received 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"], | ||
) | ||
|
||
# This test asserts that transitive dependencies have identical outputs for | ||
# different transition paths. In particular, a rules_apple ios_application and | ||
# an a apple_framework that share a swift_library, | ||
# //tests/ios/app: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 | ||
Comment on lines
+64
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd put the links here and not just the number |
||
|
||
# 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 | ||
identical_outputs_test = analysistest.make( | ||
_identical_outputs_test_impl, | ||
expect_failure = False, | ||
attrs = { | ||
"deps": attr.label_list( | ||
aspects = [_collect_transitive_outputs], | ||
), | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
load("@bazel_skylib//lib:unittest.bzl", "analysistest", "asserts") | ||
|
||
def _transitive_header_test_impl(ctx): | ||
env = analysistest.begin(ctx) | ||
asserts.true(env, len(ctx.attr.deps) > 0) | ||
target_under_test = analysistest.target_under_test(env) | ||
target_headers = target_under_test[CcInfo].compilation_context.headers.to_list() | ||
for dep in ctx.attr.deps: | ||
asserts.true(env, CcInfo in dep) | ||
dep_headers = dep[CcInfo].compilation_context.headers.to_list() | ||
for dep_header in dep_headers: | ||
# Assert that all of the dep headers are in the target | ||
if not dep_header.extension == "h": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we care about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could by default have that and better expose this to the user to match other extensions if they'd like |
||
continue | ||
|
||
has_header = dep_header in target_headers | ||
if not has_header: | ||
print("Missing header", dep_header, target_headers) | ||
asserts.true(env, has_header) | ||
return analysistest.end(env) | ||
|
||
# The headers test allows a user to assert that tests are propagated to actions | ||
# from arbitrary deps. Given a target_under_test, supply transitive deps or | ||
# virtually any file group. | ||
transitive_header_test = analysistest.make( | ||
_transitive_header_test_impl, | ||
expect_failure = False, | ||
attrs = { | ||
"deps": attr.label_list(allow_empty = False), | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,7 @@ def _get_framework_info_providers(ctx, old_cc_info, old_objc_provider): | |
private_hdrs = [], | ||
has_swift = False, | ||
framework_name = imported_framework_name, | ||
extra_search_paths = imported_framework_name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this being used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
) | ||
framework_info = FrameworkInfo( | ||
vfsoverlay_infos = [vfs.vfs_info], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
load("//rules/analysis_tests:transitive_header_test.bzl", "transitive_header_test") | ||
|
||
def make_tests(): | ||
transitive_header_test( | ||
name = "test_SomeFramework_SwiftCompilationHeaders", | ||
target_under_test = ":SomeFramework_swift", | ||
deps = ["//tests/ios/unit-test/test-imports-app/frameworks/Basic"], | ||
) | ||
|
||
transitive_header_test( | ||
name = "test_App_SwiftCompilationHeaders", | ||
target_under_test = ":TestImports-App_swift", | ||
deps = ["//tests/ios/unit-test/test-imports-app/frameworks/Basic", "@TensorFlowLiteC//:TensorFlowLiteC"], | ||
) | ||
|
||
native.test_suite( | ||
name = "AnalysisTests", | ||
tests = [ | ||
":test_App_SwiftCompilationHeaders", | ||
":test_SomeFramework_SwiftCompilationHeaders", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
load("//rules:framework.bzl", "apple_framework") | ||
|
||
apple_framework( | ||
name = "Basic", | ||
# Note: it is totally possible that a user would write a glob like this.. | ||
# srcs = glob([ | ||
# "sources/Basic/**/*.h", | ||
# ]), | ||
Comment on lines
+5
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the ideal is to keep this as a note for someone trying to understand what is under test right? Why is it relevant to know that such a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah it is possible a user to craft an redundant build file which duplicates There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with keeping it. Was just curious to know the details of the edge case you're mentioning here 👍 |
||
objc_copts = ["-fmodules-disable-diagnostic-validation"], | ||
platforms = {"ios": "12.0"}, | ||
vendored_static_frameworks = ["ios/Basic.framework"], | ||
visibility = ["//visibility:public"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
@import Foundation; | ||
|
||
typedef NS_ENUM(NSInteger, BasicVal) { | ||
BasicVal_Unknown = 0, | ||
BasicVal_FinishActivating = 1, | ||
BasicVal_DownloadTheApp = 2, | ||
}; | ||
|
||
static const NSString *BasicString = @"BasicString"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What work? Maybe more details here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to file an issue to see what is wrong with it? I don't have an idea about what to do ATM