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

Remove references to objc_provider and remove force_load_direct_deps to support rules_apple > 3.15.0 #934

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ssarad
Copy link

@ssarad ssarad commented Dec 21, 2024

  1. With rules_apple removing legacy objc_providers, rules_apple > 3.15.0 is no longer compatible with rules_ios due to Remove legacy Objc provider linking support bazelbuild/rules_apple#2611

  2. Removing force load direct deps as its no longer needed in Bazel 7 Consider removing force_load_direct_deps_rule in Bazel 7+ #862

ERROR: /Users/ssarad/Bazel/BUILD.bazel:3:15: in apple_framework_packaging rule 
Traceback (most recent call last):
	File "/Users/ssarad/.bazel_cache/output/external/rules_ios~/rules/framework.bzl", line 1109, column 48, in _apple_framework_packaging_impl
		bundle_outs = _bundle_dynamic_framework(ctx, is_extension_safe = is_extension_safe, avoid_deps = avoid_deps)
	File "/Users/ssarad/.bazel_cache/output/external/rules_ios~/rules/framework.bzl", line 888, column 44, in _bundle_dynamic_framework
		partials.framework_provider_partial(
	File "/Users/ssarad/.bazel_cache/output/external/rules_apple~/apple/internal/partials/framework_provider.bzl", line 94, column 5, in framework_provider_partial
		def framework_provider_partial(
Error: framework_provider_partial() got unexpected keyword argument: objc_provider

This PR is attempt to make rules_ios compatible with rules_apple + remove the legacy code to force load direct deps as with Bazel 7, --incompatible_objc_alwayslink_by_default does the same thing.

@ssarad ssarad marked this pull request as ready for review December 24, 2024 17:42
@ssarad
Copy link
Author

ssarad commented Dec 24, 2024

@luispadron If I can get your review that'd be fantastic 🙏

@@ -29,7 +29,7 @@ jobs:
XCODE_VERSION: ${{ matrix.xcode_version }}
USE_BAZEL_VERSION: ${{ matrix.bazel_version }}
LATEST_RULES_SWIFT_VERSION: 2.1.1
LATEST_RULES_APPLE_VERSION: 3.8.0
LATEST_RULES_APPLE_VERSION: 3.15.0
Copy link
Author

Choose a reason for hiding this comment

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

Added this line because CI was complaining about this (do let me know if this isnt right)

@luispadron
Copy link
Collaborator

Thanks for getting the work started here! I think theres some left-over usage:

Error: <target //tests/ios/in-tree-vendor-prebuilt-deps/Pods/MiSnap:MiSnap-MiSnap.framework-import> (rule 'apple_dynamic_framework_import') doesn't contain declared provider 'objc'

Do we need to upgrade rules_apple in MODULE.bazel, if so, we'll need to drop Bazel 6 support (probably in it's own PR).

@ssarad
Copy link
Author

ssarad commented Dec 28, 2024

Thanks for getting the work started here! I think theres some left-over usage:

Error: <target //tests/ios/in-tree-vendor-prebuilt-deps/Pods/MiSnap:MiSnap-MiSnap.framework-import> (rule 'apple_dynamic_framework_import') doesn't contain declared provider 'objc'

Do we need to upgrade rules_apple in MODULE.bazel, if so, we'll need to drop Bazel 6 support (probably in it's own PR).

@luispadron I think when I didnt change rules_apple to 3.15.0, I got build errors that rules_apple was expecting the objc_provider parameter.

I would vote to drop Bazel 6 support mainly because

  1. Your recent survey showed that most of us are on Bazel 7
  2. rules_apple has dropped support for Bazel 6

What do you think? If you’d think its fine to drop support for Bazel 6, I’m happy to start that PR before we let this in.

@luispadron
Copy link
Collaborator

Yeah I think it makes sense! @thiagohmcruz @gyfelton FYI in case we have some reason for keeping Bazel 6

@ssarad
Copy link
Author

ssarad commented Dec 28, 2024

Yeah I think it makes sense! @thiagohmcruz @gyfelton FYI in case we have some reason for keeping Bazel 6

Okay perfect. I can start that PR and will tag you along. I’ll likely complete it after New Years when I am back from vacation.

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.

3 participants