Fix and assert swift imported header propagation #333

jerrymarino · 2021-10-07T21:06:44Z

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:

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
test framework import under test-imports-app. Asserts transitive headers
are loaded from TesnorFlow.

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 TesnorFlow.

gyfelton · 2021-10-07T21:58:35Z

rules/framework.bzl

+        if not CcInfo in dep:
+            continue
+        for h in dep[CcInfo].compilation_context.headers.to_list():
+            propagated_interface_headers.append(depset([h]))


will h.is_source be true for headers? Or true only to a set of headers? The original logic of continue if is_source is baffling me...

Here-in lies the problem: it needs to propagate all of the headers somehow, e.g. rather than the ones of is_source.. The documentation for is_source indicates True generated sources so if you use a generated source it would be true: https://docs.bazel.build/versions/main/skylark/lib/File.html#is_source ✅

congt · 2021-10-07T22:37:05Z

rules/framework.bzl

@@ -159,22 +160,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:


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

if CcInfo in dep: ... if FrameworkInfo in dep: ...

Could you explain why the order matters?

The code has a continue statement inside of the if block so it will skip ahead.

Could we change it to something like

I haven't tried that out yet but wouldn't be opposed to it. What are the benefits to changing the code to that?

The code has a continue statement inside of the if block so it will skip ahead.

I got that, but was wondering why we need the headers of imported frameworks.

What are the benefits to changing the code to that?

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.

I got that, but was wondering why we need the headers of imported frameworks.

The point of this PR is so the headers for imported frameworks are included in upstream actions.

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.

thiagohmcruz

Thx for adding these! A few questions inline 🙏

thiagohmcruz · 2021-10-08T14:50:19Z

.github/workflows/tests.yml

+              --apple_platform_type=ios \
+              --deleted_packages='' \
+              -- //tests/ios/... \
+              -//tests/ios/frameworks/sources-with-prebuilt-binaries/... # Needs more work for pre-built binaries


What work? Maybe more details here?

Want to file an issue to see what is wrong with it? I don't have an idea about what to do ATM

thiagohmcruz · 2021-10-08T14:51:24Z

rules/analysis_tests/identical_outputs_test.bzl

+        # 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


Awesome, thx for linking this 🙏

rules/analysis_tests/identical_outputs_test.bzl

thiagohmcruz · 2021-10-08T14:52:44Z

rules/analysis_tests/identical_outputs_test.bzl

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


Why > 1 and not > 0?

The test doesn't assert the problem to have 1 dev because it asserts identical outputs amongst multiple deps https://github.com/bazel-ios/rules_ios/pull/333/files/5138482e5b1fcd470f76b793ef40e6ebc930cc73#diff-fa2c728306ed72d291b632ffa2b0458ea872ba83445c911667ac90ff0c4263a0R56

This is the only way to ensure that bazelbuild/bazel#12171 isn't happening internal to a rule!

thiagohmcruz · 2021-10-08T14:55:23Z

rules/analysis_tests/identical_outputs_test.bzl

+# - adding apple_common.multi_arch_split to apple_framework.deps - #188
+# - the transition yields the same result when used w/rules_apple - #196


Nit: I'd put the links here and not just the number #123

thiagohmcruz · 2021-10-08T14:58:44Z

rules/analysis_tests/transitive_header_test.bzl

+        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":


Do we care about .hh or .hpp here?

We could by default have that and better expose this to the user to match other extensions if they'd like

rules/analysis_tests/transitive_header_test.bzl

thiagohmcruz · 2021-10-08T15:01:56Z

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,


I don't see this being used?

extra_search_paths is an argument to the rule framework_vfs_overlay so used internally of that rule

thiagohmcruz · 2021-10-08T15:04:31Z

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

+    # Note: it is totally possible that a user would write a glob like this..
+    # srcs = glob([
+    #    "sources/Basic/**/*.h",
+    # ]),


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 glob could be written?

Yeah it is possible a user to craft an redundant build file which duplicates srcs and framework_imports - probably better to have the comment but I can remove it doesn't make sense. The rule will handle it anyways.

I'm ok with keeping it. Was just curious to know the details of the edge case you're mentioning here 👍

Co-authored-by: Thiago Cruz <thiago.hmcruz@gmail.com>

* 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.

jerrymarino force-pushed the jmarino/fix_imported_framework_headers branch from 9416cae to b2a9946 Compare October 7, 2021 21:11

gyfelton reviewed Oct 7, 2021

View reviewed changes

congt reviewed Oct 7, 2021

View reviewed changes

jschear mentioned this pull request Oct 7, 2021

Tests fail when the swift.cacheable_swiftmodules feature is enabled #334

Open

Fix test suite

5138482

jerrymarino force-pushed the jmarino/fix_imported_framework_headers branch from 257397d to 5138482 Compare October 8, 2021 01:47

thiagohmcruz approved these changes Oct 8, 2021

View reviewed changes

congt approved these changes Oct 8, 2021

View reviewed changes

jerrymarino added 2 commits October 8, 2021 10:17

Buildifier

83b6f11

Bumps project

052e72a

gyfelton approved these changes Oct 8, 2021

View reviewed changes

jerrymarino and others added 4 commits October 8, 2021 11:58

Fixing problematic project again

e9a8a5c

Update rules/analysis_tests/identical_outputs_test.bzl

27aec62

Co-authored-by: Thiago Cruz <thiago.hmcruz@gmail.com>

Update rules/analysis_tests/transitive_header_test.bzl

d335037

Co-authored-by: Thiago Cruz <thiago.hmcruz@gmail.com>

Always update the FrameworkInfo

656bb93

jerrymarino enabled auto-merge (squash) October 8, 2021 21:12

jerrymarino merged commit c01b813 into master Oct 8, 2021

jerrymarino deleted the jmarino/fix_imported_framework_headers branch October 8, 2021 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and assert swift imported header propagation #333

Fix and assert swift imported header propagation #333

jerrymarino commented Oct 7, 2021

gyfelton Oct 7, 2021

jerrymarino Oct 7, 2021

congt Oct 7, 2021

jerrymarino Oct 7, 2021 •

edited

Loading

congt Oct 8, 2021

jerrymarino Oct 8, 2021 •

edited

Loading

jerrymarino Oct 8, 2021 •

edited

Loading

thiagohmcruz left a comment

thiagohmcruz Oct 8, 2021

jerrymarino Oct 8, 2021

thiagohmcruz Oct 8, 2021

thiagohmcruz Oct 8, 2021

jerrymarino Oct 8, 2021

thiagohmcruz Oct 8, 2021

thiagohmcruz Oct 8, 2021

jerrymarino Oct 8, 2021

thiagohmcruz Oct 8, 2021

jerrymarino Oct 8, 2021

thiagohmcruz Oct 8, 2021

jerrymarino Oct 8, 2021

thiagohmcruz Oct 8, 2021

		# - adding apple_common.multi_arch_split to apple_framework.deps - #188
		# - the transition yields the same result when used w/rules_apple - #196

Fix and assert swift imported header propagation #333

Fix and assert swift imported header propagation #333

Conversation

jerrymarino commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrymarino Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrymarino Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

jerrymarino Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

thiagohmcruz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrymarino Oct 7, 2021 •

edited

Loading

jerrymarino Oct 8, 2021 •

edited

Loading

jerrymarino Oct 8, 2021 •

edited

Loading