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

incompatible_objc_linking_info_migration #17377

Open
googlewalt opened this issue Feb 1, 2023 · 15 comments
Open

incompatible_objc_linking_info_migration #17377

googlewalt opened this issue Feb 1, 2023 · 15 comments
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) platform: apple team-Rules-ObjC Issues for Objective-C maintainers

Comments

@googlewalt
Copy link
Contributor

googlewalt commented Feb 1, 2023

This flag is part of efforts to migrate ObjcProvider linking info to CcLinkingContext. It controls whether native Objective-C/C++ rules will use linking info from ObjcProvider or CcInfo (which contains the CcLinkingContext). If the flag is false, bazel will get its linking info from ObjcProvider (pre-migration behavior). If the flag is true, bazel will get its linking info from CcInfo (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:

  • Migrating other rules:

    • Rules that generate linking info need to generate them in CcInfo. These are typically calls to apple_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 in ObjcProviderApi.

    • Rules that produce AppleDynamicFrameworkInfo and AppleExecutableBinaryInfo need to add CcInfos 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 and CcInfo.

  • 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.

@googlewalt
Copy link
Contributor Author

@keith @brentleyjones FYI.

@ShreeM01 ShreeM01 added platform: apple untriaged team-Rules-ObjC Issues for Objective-C maintainers labels Feb 1, 2023
@keertk keertk added incompatible-change Incompatible/breaking change and removed untriaged labels Feb 2, 2023
@BalestraPatrick
Copy link
Member

@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 --incompatible_objc_linking_info_migration=false fixed it.

@keith
Copy link
Member

keith commented Feb 13, 2023

I think it is since we don't have all the patches above in the rules, so some providers are missing the required info.

@googlewalt
Copy link
Contributor Author

Sounds likely. Please disable the migration flag until rules_apple is caught up with the patches.

@googlewalt
Copy link
Contributor Author

@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?

@keith
Copy link
Member

keith commented Feb 21, 2023

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.

@googlewalt
Copy link
Contributor Author

googlewalt commented Feb 27, 2023

Thanks @keith for importing all the required changes. AFAICT rule_apple and rules_swift should now work with bazel at head which has --incompatible_objc_linking_info_migration=true. Can I go ahead and proceed with the cleanup? The next steps are: 1. stop generating linking info in ObjcProvider in native rules and rules_apple (rules_swift did this already). 2. delete --incompatible_objc_linking_info_migration flag and native support for linking info in ObjcProvider. 3. implement --incompatible_objc_provider_remove_linking_info flag as prelude to removing the linking info APIs.

@keith
Copy link
Member

keith commented Feb 27, 2023

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:

  1. I don't think we'll be able to take these changes externally until we want to drop support for bazel 6.x, which we naturally want to delay as long as possible since users are mostly on LTS releases unfortunately
  2. I imagine this change to entirely delete this flag shouldn't be done until after bazel 7.x releases? That seems like a long time but otherwise the addition, flipping, and deletion of this would all have happened only on rolling releases?
  3. Same comment from 1 for our side, I don't think we'd be able to make the external rules work with that until we drop bazel 6.x

@googlewalt
Copy link
Contributor Author

googlewalt commented Feb 27, 2023

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.

@brentleyjones
Copy link
Contributor

brentleyjones commented Feb 27, 2023

@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)

@googlewalt
Copy link
Contributor Author

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.

  1. I can clean up the internal rules_apple with the understanding that the patches will not land land in OSS. To make things easier to track, I can try to minimize the number of patches to 1 (similar to what rules_swift already did).

  2. Deleting --incompatible_objc_linking_info_migration does not affect behavior of 7.x, as that flag is enabled in 7.x.

  3. --incompatible_objc_provider_remove_linking_info can be implemented, but it would need to be kept off to avoid breaking OSS rules_apple. Flipping the flag and final cleanup might have to wait till 7.x, but that's much better than being blocked earlier.

How does that sound?

@keith
Copy link
Member

keith commented Feb 28, 2023

  1. yep sounds fine. we're pretty behind already because of the other blockers i mentioned
  2. I think this is a bit quick, realistically i imagine next to no one has tested this externally, it working for google means a lot for sure, but i imagine there's something we might have missed that folks will be blocked on updating on if they can't turn off the flag anymore. hopefully it's fine but just mentioning that concern
  3. sounds good, there will be some point closer to 7.x where dropping 6.x will probably be fine, but it's a large maintenance burden for us to drop it so soon after the 6.x release, especially when most users will stay on 6.x until 7.x is fully released anyways.

@keith
Copy link
Member

keith commented Feb 28, 2023

ok rules_apple now passes tests with the flag enabled as well

@googlewalt
Copy link
Contributor Author

That's great to hear. Thanks for unblocking me!

@meteorcloudy
Copy link
Member

If you want this flag to be tested by https://buildkite.com/bazel/bazelisk-plus-incompatible-flags, please add migration-ready label.

copybara-service bot pushed a commit that referenced this issue Jul 14, 2023
#17377

PiperOrigin-RevId: 548185001
Change-Id: Ibc44001666965845f2f5300ab1c507da179355e9
copybara-service bot pushed a commit that referenced this issue Jul 14, 2023
#17377

PiperOrigin-RevId: 548205363
Change-Id: I071cab13c549983627a303895775458d98af24f1
@keith keith added the P2 We'll consider working on this in future. (Assignee optional) label Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change P2 We'll consider working on this in future. (Assignee optional) platform: apple team-Rules-ObjC Issues for Objective-C maintainers
Projects
None yet
Development

No branches or pull requests

7 participants