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

Merged
merged 15 commits into from
Jan 17, 2025

Conversation

ssarad
Copy link
Contributor

@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
Copy link
Contributor Author

ssarad commented Dec 24, 2024

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

@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
Contributor 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
Contributor 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.

@ssarad ssarad mentioned this pull request Dec 29, 2024
jszumski pushed a commit that referenced this pull request Jan 9, 2025
⚠️ Bazel 6 support is being dropped from rules_ios starting release >
5.3.0

With new changes in rules_apple such as removal of legacy objc_provider,
latest rules_apple and latest rules_ios are no longer compatible, the PR
is up to fix but we need to drop support for Bazel 6 to move on with
that PR due to rules_apple dropping support for Bazel 6

Additionally, @luispadron 's survey showed most of us are on Bazel 7. 

## Related PRs

- #934

Co-authored-by: Sarad <sarad.pant@capitalone.com>
@ssarad ssarad force-pushed the master branch 2 times, most recently from e2e2d79 to 94f6914 Compare January 10, 2025 04:27
@ssarad
Copy link
Contributor Author

ssarad commented Jan 10, 2025

@luispadron @jszumski

When you have some time, can I get your reviews on this PR please? All the checks should pass so its ready to review.

Thank you and much appreciated! 🙏

nataliejameson added a commit to discord/rules_ios that referenced this pull request Jan 13, 2025
nataliejameson added a commit to discord/rules_ios that referenced this pull request Jan 13, 2025
@thiagohmcruz
Copy link
Contributor

I don't think this was being exercised in OSS properly, I'm running CI internally pointing to this PR since we rely on the features being touched here. My first test has failed with a bunch of ld: symbol(s) not found for architecture arm64. Digging more to see if it's related to this PR or not and why.

nataliejameson added a commit to discord/rules_ios that referenced this pull request Jan 13, 2025
This patches in bazel-ios/rules_ios#934, which
has not been merged just yet, to handle the fact that some link options
were removed in bazel 8 / rules_apple 3.15+ .

This also removes most properties in objc provider merging, as they were
removed upstream/ in newer versions of rules_apple.
rules/library.bzl Outdated Show resolved Hide resolved
rules/library.bzl Outdated Show resolved Hide resolved
@jszumski jszumski linked an issue Jan 14, 2025 that may be closed by this pull request
@ssarad
Copy link
Contributor Author

ssarad commented Jan 16, 2025

I think we got all the stuffs covered here. Anything pending to merge this? @jschear @jszumski @thiagohmcruz

Thank you all again for all the reviews. I appreciate it! 🙏

@jszumski jszumski enabled auto-merge (squash) January 17, 2025 13:42
@jszumski jszumski merged commit 6aa4540 into bazel-ios:master Jan 17, 2025
12 checks passed
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.

Consider removing force_load_direct_deps_rule in Bazel 7+
7 participants