-
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 4 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 recieved multiple swiftmodules | ||
jerrymarino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 | ||
# virutally any file group. | ||
jerrymarino marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 |
---|---|---|
|
@@ -159,22 +159,20 @@ def _get_virtual_framework_info(ctx, framework_files, compilation_context_fields | |
# We need to map all the deps here - for both swift headers and others | ||
fw_dep_vfsoverlays = [] | ||
for dep in transitive_deps + deps: | ||
# Collect transitive headers. For now, this needs to include all of the | ||
# transitive headers | ||
if not CcInfo in dep: | ||
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. Could you explain why the order matters? Is it because an dep (except apple_framework_packaging targets) may return CcInfo, but won't return FrameworkInfo? Could we change it to something like
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 code has a
I haven't tried that out yet but wouldn't be opposed to it. What are the benefits to changing the code to that? 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 got that, but was wondering why we need the headers of imported frameworks.
It might just be me, but the current logic is a little hard to understand. I had to know that if a dep doesn't return CcInfo, it won't return FrameworkInfo, either. 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 point of this PR is so the headers for imported frameworks are included in upstream actions. 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. Also we may be better off just checking the providers like here https://github.com/bazel-ios/rules_ios/pull/333/files/5138482e5b1fcd470f76b793ef40e6ebc930cc73#r724580342 I don't believe there's exclusivity here but need to test it a bit more. |
||
continue | ||
compilation_context = dep[CcInfo].compilation_context | ||
propagated_interface_headers.append(compilation_context.headers) | ||
|
||
if not FrameworkInfo in dep: | ||
continue | ||
framework_info = dep[FrameworkInfo] | ||
fw_dep_vfsoverlays.extend(framework_info.vfsoverlay_infos) | ||
framework_headers = depset(framework_info.headers + framework_info.modulemap + framework_info.private_headers) | ||
propagated_interface_headers.append(framework_headers) | ||
|
||
# Collect generated headers. consider exposing all required generated | ||
# headers in respective providers: -Swift, modulemap, -umbrella.h | ||
if not CcInfo in dep: | ||
continue | ||
for h in dep[CcInfo].compilation_context.headers.to_list(): | ||
if h.is_source: | ||
continue | ||
propagated_interface_headers.append(depset([h])) | ||
|
||
outputs = framework_files.outputs | ||
vfs = make_vfsoverlay( | ||
ctx, | ||
|
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"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
framework module Basic { | ||
header "Basic.h" | ||
export * | ||
} |
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