Fix and assert swift imported header propagation (bazel-ios#333)

* Fix and assert swift imported header propagation

This PR fixes a bug in swift compilation with imported framework
transitive headers. Additionally it refactors the testing rules into a
package that can be used with `rules_ios` or other repositories. These
tests iclude existing tests for Bazel transitions being incorrect in `rules_ios`
and the new generalized header test. The headers test allows a user to
assert that tests are propagated to actions

For cases:

1. add a frameworks directory to the app. This represents the case of
   `apple_framework.vendored_static_frameworks` which is commonly used
   as a way to import frameworks

2. test framework import under test-imports-app. Asserts transitive headers
   are loaded from TensorFlow.

.github/workflows/tests.yml

@@ -16,7 +16,9 @@ jobs:
         run: sudo xcode-select -s /Applications/Xcode_12.2.app
       - name: Build and Test
         run: |
+          # Host config
           bazelisk test --local_test_jobs=1 -- //... -//tests/ios/...
+
           # `deleted_packages` is needed below in order to override the value of the .bazelrc file
           bazelisk test --local_test_jobs=1 --apple_platform_type=ios --deleted_packages='' -- //tests/ios/...
       - uses: actions/upload-artifact@v2
@@ -28,15 +30,24 @@ jobs:
     # Build the entire tree with this feature enabled. Longer term, we'll likely
     # consider merging this feature into the default behavior and can re-align
     # the CI job
-    name: Build ( Virtual Frameworks )
+    name: Build and Test ( Virtual Frameworks )
     runs-on: macos-latest
     steps:
       - uses: actions/checkout@v2
       - name: Select Xcode 12.2
         run: sudo xcode-select -s /Applications/Xcode_12.2.app
       - name: Build and Test
         run: |
-          bazelisk build //... --features apple.virtualize_frameworks
+          # Host config
+          bazelisk test --features apple.virtualize_frameworks --local_test_jobs=1 -- //... -//tests/ios/...
+
+          # `deleted_packages` is needed below in order to override the value of the .bazelrc file
+          bazelisk test  --features apple.virtualize_frameworks \
+              --local_test_jobs=1 \
+              --apple_platform_type=ios \
+              --deleted_packages='' \
+              -- //tests/ios/... \
+              -//tests/ios/frameworks/sources-with-prebuilt-binaries/... # Needs more work for pre-built binaries
       - uses: actions/upload-artifact@v2
         if: failure()
         with:

rules/analysis_tests/identical_outputs_test.bzl

@@ -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)
+
+    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 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
+
+# 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],
+        ),
+    },
+)

rules/analysis_tests/transitive_header_test.bzl

@@ -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":
+                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),
+    },
+)

rules/apple_patched.bzl

@@ -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,
     )
     framework_info = FrameworkInfo(
         vfsoverlay_infos = [vfs.vfs_info],

tests/ios/app/analysis-tests.bzl

@@ -1,89 +1,7 @@
-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],
-        ),
-    },
-)
+load("//rules/analysis_tests:identical_outputs_test.bzl", "identical_outputs_test")
 
 def make_tests():
-    # 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, :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(
+    identical_outputs_test(
         name = "test_DependencyEquivilance",
         target_under_test = ":AppWithSelectableCopts",
 

tests/ios/unit-test/test-imports-app/BUILD.GoogleMobileAds

@@ -1,12 +1,18 @@
-
 load(
     "@build_bazel_rules_ios//rules:apple_patched.bzl",
-    "apple_static_framework_import"
+    "apple_static_framework_import",
 )
 
 apple_static_framework_import(
     name = "GoogleMobileAds",
-    framework_imports = glob(["Frameworks/GoogleMobileAdsFramework-Current/GoogleMobileAds.xcframework/ios-arm64_x86_64-simulator/GoogleMobileAds.framework/**"], allow_empty = False),
+    framework_imports = glob(
+        ["Frameworks/GoogleMobileAdsFramework-Current/GoogleMobileAds.xcframework/ios-arm64_x86_64-simulator/GoogleMobileAds.framework/**"],
+        allow_empty = False,
+    ),
+    sdk_dylibs = [
+        "libsqlite3",
+        "libz",
+    ],
     sdk_frameworks = [
         "AudioToolbox",
         "AVFoundation",
@@ -21,17 +27,13 @@ apple_static_framework_import(
         "QuartzCore",
         "Security",
         "StoreKit",
-        "SystemConfiguration"
+        "SystemConfiguration",
     ],
+    visibility = ["//visibility:public"],
     weak_sdk_frameworks = [
         "AdSupport",
         "JavaScriptCore",
         "SafariServices",
-        "WebKit"
+        "WebKit",
     ],
-    sdk_dylibs = [
-        "libsqlite3",
-        "libz"
-    ],
-    visibility = ["//visibility:public"],
 )

tests/ios/unit-test/test-imports-app/BUILD.bazel

@@ -1,6 +1,7 @@
 load("//rules:app.bzl", "ios_application")
 load("//rules:framework.bzl", "apple_framework", "apple_framework_packaging")
 load("//rules:test.bzl", "ios_unit_test")
+load(":analysis-tests.bzl", "make_tests")
 
 apple_framework(
     name = "SomeFramework",
@@ -9,6 +10,7 @@ apple_framework(
         "ios": "12.0",
     },
     visibility = ["//visibility:public"],
+    deps = ["//tests/ios/unit-test/test-imports-app/frameworks/Basic"],
 )
 
 ios_application(
@@ -93,3 +95,5 @@ ios_unit_test(
         "//conditions:default": [],
     }),
 )
+
+make_tests()

tests/ios/unit-test/test-imports-app/analysis-tests.bzl

@@ -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",
+        ],
+    )

tests/ios/unit-test/test-imports-app/frameworks/Basic/BUILD.bazel

@@ -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",
+    # ]),
+    objc_copts = ["-fmodules-disable-diagnostic-validation"],
+    platforms = {"ios": "12.0"},
+    vendored_static_frameworks = ["ios/Basic.framework"],
+    visibility = ["//visibility:public"],
+)

tests/ios/unit-test/test-imports-app/frameworks/Basic/ios/Basic.framework/Headers/Basic.h

@@ -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";

tests/ios/unit-test/test-imports-app/frameworks/Basic/ios/Basic.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module Basic {
+    header "Basic.h"
+    export *
+}

tests/ios/unit-test/test-imports-app/frameworks/NestedHeaders/BUILD.bazel

@@ -0,0 +1,9 @@
+load("//rules:framework.bzl", "apple_framework")
+
+apple_framework(
+    name = "NestedHeaders",
+    objc_copts = ["-fmodules-disable-diagnostic-validation"],
+    platforms = {"ios": "12.0"},
+    vendored_static_frameworks = ["ios/NestedHeaders.framework"],
+    visibility = ["//visibility:public"],
+)

tests/ios/unit-test/test-imports-app/frameworks/NestedHeaders/ios/NestedHeaders.framework/Headers/Nested/Basic.h

@@ -0,0 +1,9 @@
+@import Foundation;
+
+typedef NS_ENUM(NSInteger, NestedHeadersVal) {
+  NestedHeadersVal_Unknown = 0,
+  NestedHeadersVal_FinishActivating = 1,
+  NestedHeadersVal_DownloadTheApp = 2,
+};
+
+static const NSString *NestedHeadersString = @"NestedHeadersString";

tests/ios/unit-test/test-imports-app/frameworks/NestedHeaders/ios/NestedHeaders.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+framework module NestedHeaders {
+    header "NestedHeaders.h"
+    export *
+}