Include all framework search paths for all configurations for targets #129

amberdixon · 2020-10-02T17:39:33Z

It is possible for the same target to be built with different configurations, depending on the build settings that are applied to it. In this repro, it is possible to build FW alone. It is also possible to build it as a transitive dep of App:
App (applies build settings, like cpu, ios_minimum_os, etc.) -> FW2 -> FW
FW (can be built by itself without those build settings

Since the project depends on both FW and App directly, there are multiple paths through the dependency graph to FW:

~/Development/rules_ios amber/use-all-search-paths * bazelisk cquery 'allpaths(tests/ios/xcodeproj:Test-MultipleConfigs-Project, tests/ios/app:FW)' | grep "FW "
Loading: 
Loading: 0 packages loaded
Analyzing: 2 targets (0 packages loaded, 0 targets configured)
INFO: Analyzed 2 targets (0 packages loaded, 0 targets configured).
INFO: Found 2 targets...
//tests/ios/app:FW (8ad97c7118ac41df2bd7a3b28b08ab59b312b67df3be8939055f4bd091992cca)
//tests/ios/app:FW (c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53)
INFO: Elapsed time: 0.094s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions
INFO: Build completed successfully, 0 total actions

Diffing the configs demonstrates the different build settings, depending on how FW is being built:

~/Development/rules_ios amber/use-all-search-paths * bazelisk config c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53 8ad97c7118ac41df2bd7a3b28b08ab59b312b67df3be8939055f4bd091992cca
INFO: Displaying diff between configs c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53 and 8ad97c7118ac41df2bd7a3b28b08ab59b312b67df3be8939055f4bd091992cca
Displaying diff between configs c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53 and 8ad97c7118ac41df2bd7a3b28b08ab59b312b67df3be8939055f4bd091992cca
FragmentOptions com.google.devtools.build.lib.analysis.config.CoreOptions {
  affected by starlark transition: [apple_split_cpu, ios_minimum_os], [apple configuration distinguisher, cpu]
  cpu: ios_x86_64, darwin_x86_64
  transition directory name fragment: ST-0010df40fd2e, ST-2b46341d9197
}
FragmentOptions com.google.devtools.build.lib.rules.apple.AppleCommandLineOptions {
  apple configuration distinguisher: APPLEBIN_IOS, APPLEBIN_MACOS
  apple_platform_type: ios, macos
}

If the developer builds App, then we would expect autocompletion operations for FW's methods to work when editing FW2 source files. Therefore the FW2 framework search paths needs to point to the output directory of FW's artifacts (even if FW is built transitively from App.)

For this PR, we have generated projects that depend directly on both App and FW. In both cases, we now see that the framework search paths in the FW2 xcode build settings point to the output directories for both configurations of FW. That way, when xcode's indexer tries to compile FW2's source files, the compiler will be able to find where the FW framework is defined.

segiddins

Nice!

segiddins · 2020-10-02T18:19:51Z

rules/xcodeproj.bzl

-        )
+        all_hmaps = []
+        for at in all_transitive_targets:
+          if at.name == target_name:


this feels a tad weird to me: can you drop a comment in here explaining what this is doing?

I'll drop a comment here.

The all_transitive_targets list returned by the aspect actually has all the targets built with their different configurations. Therefore we need to iterate through this list to identify all the ways that a target can be configured.

segiddins · 2020-10-02T18:20:19Z

rules/xcodeproj.bzl

+        for at in all_transitive_targets:
+          if at.name == target_name:
+            all_hmaps.extend(at.hmap_paths.to_list())
+        target_settings["HEADER_SEARCH_PATHS"] = _joined_header_search_paths(all_hmaps)

        # Ensure Xcode will resolve references to the XCTest framework.
        framework_search_paths = ["$(PLATFORM_DIR)/Developer/Library/Frameworks"]


this is a bit of a "pyramid of doom", maybe extract into a method?

segiddins · 2020-10-02T18:20:52Z

tests/ios/app/FW2/FW.m

@@ -0,0 +1,5 @@
+#import <FW/FW.h>
+
+@implementation FW


nit: change class name to FW2

(to avoid symbol name clashes with the definition in FW)

gyfelton

damn should have done debugging script with this change

gyfelton · 2020-10-05T16:22:56Z

rules/xcodeproj.bzl

@@ -116,6 +116,7 @@ def _xcodeproj_aspect_impl(target, ctx):
            if test_host_target:
                test_host_appname = test_host_target[_TargetInfo].direct_targets[0].name

+        framework_includes = depset([], transitive = _get_attr_values_for_name(deps, _SrcsInfo, "framework_includes"))


just curious if moving here makes any diff. or it's just to make code clenaer

That did not make a difference. I think it does make code slightly cleaner.

gyfelton · 2020-10-05T16:25:23Z

rules/xcodeproj.bzl

@@ -252,7 +253,22 @@ def _exclude_swift_incompatible_define(define):
        return token
    return None

-def _joined_header_search_paths(hmap_paths):
+def _fw_search_paths_for_target(target_name, all_transitive_targets):


what's fw stands for again... sorry i totally forgot

no problem. fw = framework. I'll rename the method.

gyfelton · 2020-10-05T16:27:07Z

rules/xcodeproj.bzl

+    framework_search_paths = ["$(PLATFORM_DIR)/Developer/Library/Frameworks"]
+
+    # all_transitive_targets includes all the targets built with their different configurations.
+    # Some configurations are only applied when the target is reached transitively


reached transitively.... this is so new to me :)

i feel like we should start using subway metaphor, ie: two station can be connected directly or transitively ;)

lol! It has to do with traversal from on target to another through the dependency graph. I hoped this explanation was clear but feel free to suggest a re-wording :)

amberdixon · 2020-10-05T20:21:10Z

tests/ios/xcodeproj/Single-Static-Framework-Project.xcodeproj/project.pbxproj

@@ -282,7 +282,7 @@
 				CLANG_ENABLE_OBJC_ARC = YES;
 				FRAMEWORK_SEARCH_PATHS = "$(PLATFORM_DIR)/Developer/Library/Frameworks";
 				GCC_PREPROCESSOR_DEFINITIONS = "$(inherited)";
-				HEADER_SEARCH_PATHS = "\"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_public_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_private_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_private_angled_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT\"";
+				HEADER_SEARCH_PATHS = "\"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-e85ff352ea2d/bin/tests/ios/frameworks/objc/ObjcFramework_public_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-e85ff352ea2d/bin/tests/ios/frameworks/objc/ObjcFramework_private_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-e85ff352ea2d/bin/tests/ios/frameworks/objc/ObjcFramework_private_angled_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_public_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_private_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT/bazel-out/applebin_ios-ios_x86_64-dbg-ST-f75cadb68314/bin/tests/ios/frameworks/objc/ObjcFramework_private_angled_hmap.hmap\" \"$BAZEL_WORKSPACE_ROOT\"";


This change makes sense, because ObjcFramework can be built one of two ways:

~/Development/rules_ios amber/use-all-search-paths bazelisk cquery 'allpaths(tests/ios/xcodeproj:Single-Static-Framework-Project, tests/ios/frameworks/objc:ObjcFramework)' INFO: Analyzed 2 targets (0 packages loaded, 79 targets configured). INFO: Found 2 targets... //tests/ios/frameworks/objc:ObjcFramework (b825178b8896d4f990944aa10179e51f20dabcefac3ab234bf5d4da3df2131df) //tests/ios/frameworks/objc:ObjcFramework (HOST) //tests/ios/frameworks/objc:ObjcFramework (d9a500664a14607c0e7a1112f9a284574e55f557c6fc99cbbe26a2d6a972f231) //tests/ios/xcodeproj:Single-Static-Framework-Project (756ab3ce6f028ea563926bee2bd9c47c1dbcd5b3f5aeee8d3d12d1c605038aa3) //tests/ios/xcodeproj:Single-Static-Framework-Project (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTestLib (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTestLib_objc (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTestLib (c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53) //tests/ios/frameworks/objc:ObjcFrameworkTestLib_objc (c54ba92df8de88568a6191030eafabda830708ac76941ab9cdb7a2360ee2ea53) //tests/ios/frameworks/objc:ObjcFrameworkTests_objc (3713a275cd8fa69dc716f35427b8238fe8b88e687b11e8993bece6bf66a8743d) //tests/ios/frameworks/objc:ObjcFrameworkTests_objc (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTests.__internal__.__test_bundle (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTests.__internal__.__test_bundle.swift_runtime_linkopts (HOST) //tests/ios/frameworks/objc:ObjcFrameworkTests.__internal__.__test_bundle (756ab3ce6f028ea563926bee2bd9c47c1dbcd5b3f5aeee8d3d12d1c605038aa3) //tests/ios/frameworks/objc:ObjcFrameworkTests.__internal__.__test_bundle.swift_runtime_linkopts (3713a275cd8fa69dc716f35427b8238fe8b88e687b11e8993bece6bf66a8743d) //tests/ios/frameworks/objc:ObjcFrameworkTests (756ab3ce6f028ea563926bee2bd9c47c1dbcd5b3f5aeee8d3d12d1c605038aa3) //tests/ios/frameworks/objc:ObjcFrameworkTests (HOST) INFO: Elapsed time: 0.239s INFO: 0 processes. INFO: Build completed successfully, 0 total actions ~/Development/rules_ios amber/use-all-search-paths bazelisk config b825178b8896d4f990944aa10179e51f20dabcefac3ab234bf5d4da3df2131df d9a500664a14607c0e7a1112f9a284574e55f557c6fc99cbbe26a2d6a972f231 INFO: Displaying diff between configs b825178b8896d4f990944aa10179e51f20dabcefac3ab234bf5d4da3df2131df and d9a500664a14607c0e7a1112f9a284574e55f557c6fc99cbbe26a2d6a972f231 Displaying diff between configs b825178b8896d4f990944aa10179e51f20dabcefac3ab234bf5d4da3df2131df and d9a500664a14607c0e7a1112f9a284574e55f557c6fc99cbbe26a2d6a972f231 FragmentOptions com.google.devtools.build.lib.analysis.config.CoreOptions { affected by starlark transition: [apple configuration distinguisher, apple_platform_type, cpu, ios_minimum_os], [apple_split_cpu, ios_minimum_os] transition directory name fragment: ST-e85ff352ea2d, ST-f75cadb68314 }

gyfelton · 2020-10-06T19:36:30Z

@amberdixon From what I saw the path under bazel-out started with ios- will never end with ST-some_hash but with -dbg, when will a bazel out folder not having ST-some_hash as the suffix?
And I am seeing this on my side as well

amberdixon · 2020-10-07T22:53:20Z

@amberdixon From what I saw the path under bazel-out started with ios- will never end with ST-some_hash but with -dbg, when will a bazel out folder not having ST-some_hash as the suffix?
And I am seeing this on my side as well

Discussed IRL but pull request 133 fixes that: #133

amberdixon · 2020-10-08T01:00:20Z

@segiddins or @gyfelton can one of you take a look at my changes to hmap.bzl and confirm that it's OK? Thanks!

segiddins · 2020-10-08T01:03:39Z

rules/hmap.bzl

-            files = depset([ctx.outputs.headermap]),
-        ),
+    contains_headers = False
+    for h in hdrs_lists:


hdrs_lists =[l for l in hdrs_lists if l]

And then the conditional boils down to if hdrs_lists: ?

amberdixon requested review from gyfelton and segiddins October 2, 2020 17:39

segiddins reviewed Oct 2, 2020

View reviewed changes

amberdixon force-pushed the amber/use-all-search-paths branch from dc49f18 to 8c6e88b Compare October 2, 2020 20:56

segiddins approved these changes Oct 2, 2020

View reviewed changes

gyfelton approved these changes Oct 5, 2020

View reviewed changes

amberdixon commented Oct 5, 2020

View reviewed changes

amberdixon added 7 commits October 7, 2020 15:54

include all framework search paths for all configurations for targets

f2a05e4

Fix to point to correct symbol name

bce67d2

Address build failures and PR feedback

7eadd51

fix build errors

51acff7

Address more build issues

e86a97c

rename method name

d2d2c3d

Filter out headermaps that do not take any inputs

3cee054

amberdixon force-pushed the amber/use-all-search-paths branch from 289dae2 to 3cee054 Compare October 8, 2020 00:09

buildifier fixes

c9a7d4f

segiddins approved these changes Oct 8, 2020

View reviewed changes

change the way we iterate over headers

a08ec57

amberdixon merged commit c5a87dd into master Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include all framework search paths for all configurations for targets #129

Include all framework search paths for all configurations for targets #129

amberdixon commented Oct 2, 2020 •

edited

Loading

segiddins left a comment

segiddins Oct 2, 2020

amberdixon Oct 2, 2020

segiddins Oct 2, 2020

segiddins Oct 2, 2020

segiddins Oct 2, 2020

gyfelton left a comment

gyfelton Oct 5, 2020

amberdixon Oct 7, 2020

gyfelton Oct 5, 2020

amberdixon Oct 5, 2020

gyfelton Oct 5, 2020

gyfelton Oct 5, 2020

amberdixon Oct 5, 2020

amberdixon Oct 5, 2020

gyfelton commented Oct 6, 2020 •

edited

Loading

amberdixon commented Oct 7, 2020

amberdixon commented Oct 8, 2020

segiddins Oct 8, 2020

Include all framework search paths for all configurations for targets #129

Include all framework search paths for all configurations for targets #129

Conversation

amberdixon commented Oct 2, 2020 • edited Loading

segiddins 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

gyfelton 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

gyfelton commented Oct 6, 2020 • edited Loading

amberdixon commented Oct 7, 2020

amberdixon commented Oct 8, 2020

Choose a reason for hiding this comment

amberdixon commented Oct 2, 2020 •

edited

Loading

gyfelton commented Oct 6, 2020 •

edited

Loading