-
Notifications
You must be signed in to change notification settings - Fork 4k
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
incompatible_objc_linking_info_migration #17377
Comments
@keith @brentleyjones FYI. |
@googlewalt Is it expected that this is causing failures to link due to missing symbols from frameworks in an iOS application with Objective-C and Swift? I was testing something unrelated on latest Bazel's master and run into this failure. Setting |
I think it is since we don't have all the patches above in the rules, so some providers are missing the required info. |
Sounds likely. Please disable the migration flag until rules_apple is caught up with the patches. |
@keith @brentleyjones Do you have any ETAs for when rules_apple can get caught up with the required patches, so I can continue with the cleanup? |
The issue we have right now is that the cc_toolchain_forwarder is depended on by many upstream changes, but that change requires all users to have a platform_mappings file, which we're not sure we want to force on everyone. |
Thanks @keith for importing all the required changes. AFAICT rule_apple and rules_swift should now work with bazel at head which has |
I'm tracking the current updates needed for rules_apple here bazelbuild/rules_apple#1848, that list you gave was helpful but there are a few other fixes that I'm still debugging. As for your next few steps from the outside of google perspective:
|
It is no longer a requirement to wait an entire release to flip a flag, then an entire release to remove a flag. That requirement was back before we switched to the LTS model, when releases would happen ever few months. Part of the motivation for switching to the LTS model is to improve development velocity at head, while still providing stability with LTS releases. Taking 1-2 years to implement/flip/retire an incompatible flag would slow down development even more than it was before. Can we make a 6.x fork of rules_apple? I think in another thread @brentleyjones mentioned that this was already done for 5.x and seems like a good model to continue. |
@googlewalt It's correct that we forked for 6.x, but we don't want to do that again. Instead, we want to support both 6.x and 7.x in the same ruleset version. The forking makes it really hard for users, especially with Bzlmod version resolution, and I agree with @meteorcloudy that we shouldn't fork unless absolutely necessary: #17372 (comment) |
Ok IIUC these constraints don't necessarily block my plan, nor is a fork required? Once bazelbuild/rules_apple#1848 is done, rule_apple should be able to support both bazel 6.x and 7.x.
How does that sound? |
|
ok rules_apple now passes tests with the flag enabled as well |
That's great to hear. Thanks for unblocking me! |
If you want this flag to be tested by https://buildkite.com/bazel/bazelisk-plus-incompatible-flags, please add |
#17377 PiperOrigin-RevId: 548185001 Change-Id: Ibc44001666965845f2f5300ab1c507da179355e9
#17377 PiperOrigin-RevId: 548205363 Change-Id: I071cab13c549983627a303895775458d98af24f1
This flag is part of efforts to migrate
ObjcProvider
linking info toCcLinkingContext
. It controls whether native Objective-C/C++ rules will use linking info fromObjcProvider
orCcInfo
(which contains theCcLinkingContext
). If the flag is false, bazel will get its linking info fromObjcProvider
(pre-migration behavior). If the flag is true, bazel will get its linking info fromCcInfo
(post-migration behavior).See #16939 for more details on the migration, and general migration guide.
The flag was implemented on 12/15/2022, and enabled by default on 1/25/2023. It is not in Bazel 6.0. The reason for flipping the flag relatively soon is to because there have already been cleanups to internal versions of rules_apple/rules_swift that require the flag flip, so we wanted to try to keep the repos relatively consistent.
Below is more information on what the bazel community may need to do for the migration:
The change should mostly impact rules maintainers, and is expected to be mostly transparent to end users.
For rules maintainers:
All internal versions of the language rules were migrated by 12/8. Affected rules were rules_apple and rules_kotlin. Note that no changes were required for rules_swift. There was one change required for rules_kotlin but I'm not clear on how it is maintained; for now I'll provide info on rules_apple.
OSS maintainers of rules_apple need to update those rules. Here is a list of relevant changes if cherrypicking is required:
10/10: bazelbuild/rules_apple@254a4bb
10/13: bazelbuild/rules_apple@608ae26
11/9: bazelbuild/rules_apple@c13da60
11/10: bazelbuild/rules_apple@8e68a3c
11/10: bazelbuild/rules_apple@d8ab65d
11/11: bazelbuild/rules_apple@17f4ac4
12/8: bazelbuild/rules_apple@ed4e8c6
Migrating other rules:
Rules that generate linking info need to generate them in
CcInfo
. These are typically calls toapple_common.new_objc_provider()
. Only calls that produce linking info require migration.Rules that use linking info need use them from
CcInfo
. These are typically references to fields inObjcProviderApi
.Rules that produce
AppleDynamicFrameworkInfo
andAppleExecutableBinaryInfo
need to addCcInfo
s that provide the linking info to them.See Migrate ObjcProvider linking info to CcLinkingContext #16939 for details. For examples for how to migrate, see (1) above links, (2) rules_swift which has lots of examples of how the same linking info is represented in
ObjcProvider
andCcInfo
.End users need to make sure they are using compatible versions of Starlark rules. They may need to fix some build errors that were previously not reported, such as duplicate definition link errors.
The text was updated successfully, but these errors were encountered: