Build frameworks/resource_bundles behind a transition #73

segiddins · 2020-06-09T02:24:22Z

This way, their artifacts can be shared across multiple apps/test targets, regardless of the deployment targets used in the final packaged product

# Conflicts: # rules/framework.bzl # rules/xcodeproj.bzl

rules/library.bzl

rules/precompiled_apple_resource_bundle.bzl

tests/framework/platforms/BUILD.bazel

segiddins · 2020-06-09T02:28:06Z

...ation-Project-AllTargets.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist

@@ -1,8 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>


Deleted since I don't think these files actually get generated anywhere (except maybe by Xcode)

cc @amberdixon

segiddins · 2020-06-09T02:28:45Z

tools/xcodeproj_shims/xcodeproj-installer.sh

+cp -r "${project_path}" "$tmp_dest"
+chmod -R +w "${tmp_dest}"
+
+readonly stubs_dir="${tmp_dest}/bazelstubs"


installing the stubs into the tmp dir means they will get chmod'd, allowing them to be overwritten by subsequent invocations

cc @amberdixon this is the change I showed you on Tuesday

ob

Do we have concerns about blowing up the build graph by using transitions?

I also hit a strange problem where I had to remove shallow_since form the rules_apple repository or git wouldn't find the commit specified in repositories.bzl. Unrelated but I thought it might be worth mentioning it.

ob · 2020-06-11T20:10:38Z

rules/framework.bzl

-        visibility = kwargs.get("visibility", None),
-        tags = kwargs.get("tags", None),
+        platforms = library.platforms,
+        **framework_packaging_kwargs


ob · 2020-06-11T20:25:35Z

rules/precompiled_apple_resource_bundle.bzl

+    output_plist = ctx.actions.declare_file(
+        paths.join("%s-intermediates" % ctx.label.name, "Info.plist"),
+    )
+    bundle_id = ctx.attr.bundle_id or "com.cocoapods." + bundle_name


Would it make sense to make the bundle_id required instead of providing a default?

Maybe? We could push the default out to the apple_library macro if you think that's better, but we do need a default at some layer

ob · 2020-06-11T20:27:17Z

rules/precompiled_apple_resource_bundle.bzl

+        ),
+        bundle_id = attr.string(
+            mandatory = False,
+            doc = "The bundle identifier of the resource bundle.",


Or if we are providing a default (which I suggest we don't) it should be documented here.

ob · 2020-06-11T20:30:11Z

rules/precompiled_apple_resource_bundle.bzl

+_FAKE_BUNDLE_PRODUCT_TYPE_BY_PLATFORM_TYPE = {
+    "ios": apple_product_type.application,
+    "tvos": apple_product_type.application,
+    "watchos": apple_product_type.watch2_application,


Should we add macos here since why not?

macos is tested, it just doesnt need a fake bundle product type since the rule descriptors have one for a macos resource bundle

ob · 2020-06-11T20:33:29Z

...ation-Project-AllTargets.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist

@@ -1,8 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>


segiddins · 2020-06-11T21:44:00Z

rules/library.bzl

-            tags = _MANUAL,
-        )
-        apple_resource_bundle(
+        precompiled_apple_resource_bundle(
            name = target_name,
            bundle_name = bundle_name,
            resources = [
                library_tools["wrap_resources_in_filegroup"](name = target_name + "_resources", srcs = resource_bundles[bundle_name]),


In a follow-up, it would be nice to get rid of this extra target as well

segiddins · 2020-06-11T21:50:04Z

Do we have concerns about blowing up the build graph by using transitions?

I think that this will actually shrink the build graph, because it's an incoming edge transition rather than an outgoing one. Before, we could build a framework (and therefore its library targets) N times, depending on what app/test binary was linking it in. Now, it's guaranteed to only be built once per platform when an explicit list of platform/deployment target pairs is passed in.

amberdixon · 2020-06-11T23:07:56Z

rules/framework.bzl

+            minimum_os_version = str(current_apple_platform.target_os_version),
+            platform_type = str(current_apple_platform.platform.platform_type),
+            product_type = ctx.attr._product_type,
+            uses_swift = swiftmodule_out != None,


nit, can you add parens here to keep the order of operations obvious?
suggestion:
uses_swift = (swiftmodule_out != None),

I hope the RHS doesn't turn into an array/tuple if you do that!

amberdixon

For other reviewers, I found the following bazel docs helpful when reviewing this PR:
https://docs.bazel.build/versions/master/skylark/lib/transition.html
https://docs.bazel.build/versions/master/skylark/config.html
https://docs.bazel.build/versions/master/skylark/config.html#incoming-edge-transitions

amberdixon · 2020-06-12T03:42:39Z

tests/framework/platforms/BUILD.bazel

+    resources_to_bundle = glob(["Resources/**/*"]),
+)
+
+make_applications(


Did you confirm that this application would fail to run, if this transition was not applied?

amberdixon

LGTM! Looks like this PR does 3 things, correct my understanding if I'm wrong:

adding a transition to change configs during dep graph traversal. Specifically, changes the configuration when traversing from a dependency of a framework to the framework packaging target.
resource bundle rules change to avoid unnecessarily rebuilding resource bundles
light xcodeproj changes to consume BundleInfo for frameworks through the framework rule's providers and fix up the installer to write the generated project to a tmpdir.

segiddins added 2 commits June 8, 2020 18:54

Spike on framework transition

d59da9f

# Conflicts: # rules/framework.bzl # rules/xcodeproj.bzl

Allow incremental regeneration of rcode projects

f6621f1

segiddins commented Jun 9, 2020

View reviewed changes

segiddins mentioned this pull request Jun 9, 2020

Serialize framework platforms bazel-ios/cocoapods-bazel#14

Merged

segiddins added 2 commits June 9, 2020 15:46

Pass platforms through for apps/tests

cf87e73

Add docs for new attrs

8d82570

segiddins requested a review from ob June 10, 2020 00:09

segiddins marked this pull request as ready for review June 10, 2020 00:09

Generate docs for new bzl files

6964eca

segiddins force-pushed the segiddins/precomp-resources-shared-frameworks branch from 5a3ff81 to 6964eca Compare June 10, 2020 00:17

ob approved these changes Jun 11, 2020

View reviewed changes

ob added 2 commits June 11, 2020 14:09

Merge branch 'master' into segiddins/precomp-resources-shared-frameworks

002721c

Fix a coupld of CI failures

e025575

segiddins commented Jun 11, 2020

View reviewed changes

amberdixon reviewed Jun 11, 2020

View reviewed changes

amberdixon reviewed Jun 12, 2020

View reviewed changes

amberdixon approved these changes Jun 12, 2020

View reviewed changes

segiddins merged commit 70a5b7a into master Jun 12, 2020

segiddins deleted the segiddins/precomp-resources-shared-frameworks branch June 12, 2020 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build frameworks/resource_bundles behind a transition #73

Build frameworks/resource_bundles behind a transition #73

segiddins commented Jun 9, 2020

segiddins Jun 9, 2020

segiddins Jun 11, 2020

ob Jun 11, 2020

segiddins Jun 9, 2020

segiddins Jun 11, 2020

ob left a comment

ob Jun 11, 2020

ob Jun 11, 2020

segiddins Jun 11, 2020

ob Jun 11, 2020

ob Jun 11, 2020

segiddins Jun 11, 2020

ob Jun 11, 2020

segiddins Jun 11, 2020

segiddins commented Jun 11, 2020

amberdixon Jun 11, 2020

amberdixon Jun 11, 2020

amberdixon left a comment

amberdixon Jun 12, 2020

segiddins Jun 12, 2020

amberdixon left a comment

Build frameworks/resource_bundles behind a transition #73

Build frameworks/resource_bundles behind a transition #73

Conversation

segiddins commented Jun 9, 2020

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

ob 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

segiddins commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amberdixon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amberdixon left a comment

Choose a reason for hiding this comment