Skip to content
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

Update to rules_apple 3.0.0 #759

Closed

Conversation

jszumski
Copy link
Collaborator

@jszumski jszumski commented Aug 31, 2023

Changes

Remaining work

  • Update project generation rules
  • Run buildifier
  • Decide if we should try to make these changes backwards-compatible no
  • Decide if this is a good time to drop Bazel 5 support yes

@@ -25,6 +25,7 @@ load("@build_bazel_rules_apple//apple/internal:apple_toolchains.bzl", "AppleMacT
load("@build_bazel_rules_apple//apple/internal:swift_support.bzl", "swift_support")
load("@build_bazel_rules_apple//apple/internal/utils:clang_rt_dylibs.bzl", "clang_rt_dylibs")
load("@build_bazel_rules_apple//apple:providers.bzl", "AppleBundleInfo", "IosFrameworkBundleInfo")
load("@build_bazel_rules_apple//apple/internal:providers.bzl", "new_applebundleinfo")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to rely on yet another internal provider from rules_apple here? It seems like it would continue to work well on 5.x.x pretty easily w/o the change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the issue with this line is everyone will have to run a commit later than:
bazelbuild/rules_apple#2194

Copy link
Collaborator Author

@jszumski jszumski Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's incompatible because the initializer intentionally fails here in _make_banned_init:

AppleBundleInfo, new_applebundleinfo = provider(
    doc = ...,
    fields = {
        ...
    },
    init = _make_banned_init(provider_name = "AppleBundleInfo"),
)

Keeping rules_ios close to the head of rules_apple was intentional with this change.

Copy link
Contributor

@jerrymarino jerrymarino Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah what I'm wondering, is how to make it work backwards with older versions:

e.g.

# in rules_ios/framework.bzl or util module
def _wrapped_bundule_init(**kwargs):
    if old_version:
       return AppleBundleInfo(kwargs)
    else:
       return new_applebundleinfo(kwargs)

Though the initializer moving to a private API which used to be public is another story 😮‍💨

if (arch == "armv7s" or arch == "arm64e"):
# unsupported platform-arch by rules_apple
# unsupported platform-arch by rules_apple
unsupported_platforms = ["armv7", "armv7s", "i386"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping arm64e was intentional here since it exists in rules_apple

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this works on 2.5?

@jszumski jszumski force-pushed the jszumski/update-rules-apple-3.0.0 branch from 9009b1a to 7e1d2aa Compare September 6, 2023 18:48
@jszumski jszumski changed the base branch from master to jszumski/drop-bazel-5 September 6, 2023 18:49
@jszumski jszumski force-pushed the jszumski/update-rules-apple-3.0.0 branch from 7e1d2aa to 771a0cb Compare September 6, 2023 20:36
Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to review this update and see what it takes to support older versions.

Use new provider initialization pattern, use new apple_platform_split_transition, drop support for i386 slices in xcframeworks, passthrough minimum_os_version to all test rules.
@jszumski jszumski force-pushed the jszumski/update-rules-apple-3.0.0 branch from 771a0cb to 3b4e5ef Compare September 7, 2023 20:57
Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like with few quick changes there isn't a reason it wouldn't support 2.5.x by making some of these conditional. Likely this can be done with wrappers to further centralize the calling of rules_apple APIs here

if (arch == "armv7s" or arch == "arm64e"):
# unsupported platform-arch by rules_apple
# unsupported platform-arch by rules_apple
unsupported_platforms = ["armv7", "armv7s", "i386"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this works on 2.5?

@@ -664,8 +675,11 @@ def _bundle_dynamic_framework(ctx, is_extension_safe, avoid_deps):
resource_deps = ctx.attr.deps + ctx.attr.transitive_deps + ctx.attr.data
current_apple_platform = transition_support.current_apple_platform(apple_fragment = ctx.fragments.apple, xcode_config = ctx.attr._xcode_config)
platform_type = str(current_apple_platform.platform.platform_type)
rule_descriptor = rule_support.rule_descriptor_no_ctx(platform_type, apple_product_type.framework)
platform_prerequisites = _platform_prerequisites(ctx, rule_descriptor, platform_type)
rule_descriptor = rule_support.rule_descriptor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to plumb in the rule_descriptor from above?

@@ -694,6 +708,7 @@ def _bundle_dynamic_framework(ctx, is_extension_safe, avoid_deps):
ctx,
avoid_deps = avoid_deps,
entitlements = None,
exported_symbols_lists = ctx.files.exported_symbols_lists,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# If this isn't backwards compatible with 2.5.0 
if rules_apple_api_version_3:
    kwargs["exported_symbols_lists"] = ctx.files.exported_symbols_lists
else:
    kwargs[" executable_name"] = bundle_name

@@ -1053,23 +1073,23 @@ apple_framework_packaging = rule(
),
"deps": attr.label_list(
mandatory = True,
cfg = apple_common.multi_arch_split,
cfg = rules_apple_transition_support.apple_rule_transition,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible to add conditional assignment at the top of framework.bzl or in some other file? e.g. :

if rules_apple_api_version_3:
    apple_dep_transition = rules_apple_transition_support.apple_rule_transition
else:
    apple_dep_transition = apple_common.multi_arch_split

Then - consuming it here:

cfg = apple_dep_transition

@@ -52,6 +53,7 @@ def _precompiled_apple_resource_bundle_impl(ctx):

platform_prerequisites = platform_support.platform_prerequisites(
apple_fragment = ctx.fragments.apple,
build_settings = None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May or many not need fixing if it passes CI with 2.5 / LTS enabled

@@ -19,7 +19,7 @@
"""iOS test runner rule."""

load(
"@build_bazel_rules_apple//apple/testing:apple_test_rules.bzl",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need other way to load it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you conditionalize load statements? I didn't think so but maybe I'm wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure - we can unify the sims in a starlark file we load in “rules ios dependencies” - based on the api version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shims*

@jerrymarino
Copy link
Contributor

@jszumski last night I pulled this down and looked at at bit. It seems like there are 2 distinct issues here:

  1. making rules_ios work correctly with rules_apple 3.0.0 - this is easier said than done and the errors you have in the CI job are the tip of the iceberg. I'm trying to find the simplest way for to unblock you and others.

  2. cleanup and refactoring some stuff to better support pt1 and also consequently older rules_apple.

@jerrymarino
Copy link
Contributor

Submitting and end to end implementation of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants