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_provider_remove_linking_info #19000

Open
googlewalt opened this issue Jul 19, 2023 · 7 comments
Open

incompatible_objc_provider_remove_linking_info #19000

googlewalt opened this issue Jul 19, 2023 · 7 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

This flag is part of the efforts to migrate ObjcProvider linking info to CcLinkingContext. It removes the Starlark APIs related to the definition and use of linking info in ObjcProvider. See #17377 for more details on the overall efforts.

Specifically, the flag does the following:

  • apple_common.new_objc_provider() will stop taking the following linking related arguments: cc_library, dynamic_framework_file, flag, force_load_library, imported_library, library, link_inputs, linkopt, sdk_dylib, sdk_framework, static_framework_file, weak_sdk_framework.

  • apple_common.new_dynamic_framework_provider() will no longer accept an objc argument. cc_info will be required, not optional.

  • apple_common.new_executable_binary_provider() will no longer accept an objc argument. cc_info will be required, not optional.

  • The following linking related APIs will be removed:

    • In ObjcProvider: cc_library, dynamic_framewrok_file, force_load_library, imported_library, library, link_inputs, linkopt, sdk_dylib, sdk_framework, static_framework_file, weak_sdk_framework, dynamic_framework_names, dynamic_framework_paths, static_framework_names, static_framework_paths.
    • In AppleDynamicFrameworkInfo: objc.
    • In AppleExecutableBinaryInfo: objc.

The flag will be available shortly, and will be kept disabled for 7.0. The flag will be flipped in coordination with OSS apple_rules.

@googlewalt
Copy link
Contributor Author

@keith @brentleyjones FYI.

@brentleyjones
Copy link
Contributor

brentleyjones commented Jul 19, 2023

Since all of these are removals, there is no way (afaict) to dynamically detect if this flag is flipped. So we will need a value on a fragment (or apple_common?) that we can check that reflects the value of this flag.

@sgowroji sgowroji added platform: apple incompatible-change Incompatible/breaking change team-Rules-ObjC Issues for Objective-C maintainers labels Jul 20, 2023
@googlewalt
Copy link
Contributor Author

brentleyjones@ can you clarify why you need dynamic detection of this flag?

The idea is that by the time we get to this stage of the migration, all the APIs guarded by this flag no longer does anything useful, and can safely be deleted. Given the progress of rules_apple as indicated in #17377, I believe the version of rules_apple of used for 6.x and 7.x can safely remove all the deprecated APIs.

This migration mirrors that of the compile info migration (#11359), and back then we did not need value of such a flag.

@brentleyjones
Copy link
Contributor

brentleyjones commented Aug 16, 2023

I believe the version of rules_apple of used for 6.x and 7.x can safely remove all the deprecated APIs.

I don't think we are there yet, but if that's the case, then yes, we won't need dynamic detection, we can just remove the APIs. I maybe misunderstood this purpose of the flag. Did rules_apple around the time of change maintain support for older Bazel versions? The key thing for us is that a single version of rules_apple needs to work for both Bazel 6 and 7.

@googlewalt
Copy link
Contributor Author

I'm no familiar with development of OSS version rules_apple, but our plan discussed in #17377 was to get rules_apple to a state so that the previous flag, --incompatible_objc_linking_info_migration, could be enabled. Now that we have done that, rules_apple should be able to support both 6.0 and 7.0. Any cleanups related to this new flag can go in without breaking compatibility with 6.0.

@keith
Copy link
Member

keith commented Aug 17, 2023

I noticed one case where we might have to conditionalize the rules for 6.x vs 7.x trying to land https://github.com/bazelbuild/rules_apple/pull/1872/files which removes uses of dynamic_framework_file. 6.x errors with:

Error in new_executable_binary_provider: new_executable_binary_provider() missing 1 required named argument: objc

Maybe I could provide an empty one in that case instead though

@googlewalt
Copy link
Contributor Author

Good point. The relevant bazel change is 28f9056. Using an empty objc should suffice.

copybara-service bot pushed a commit that referenced this issue Aug 22, 2023
#19000

PiperOrigin-RevId: 559141220
Change-Id: I76bc8e6dd764952f4ca2240c1c6c764a8e842494
@keith keith added the P2 We'll consider working on this in future. (Assignee optional) label Dec 18, 2023
copybara-service bot pushed a commit that referenced this issue Jan 5, 2024
#19000

PiperOrigin-RevId: 596060323
Change-Id: Ifbef6ea29918e4af7856a675b18f1ebfd1a14f37
copybara-service bot pushed a commit that referenced this issue Jan 25, 2024
#19000

This has been the default behavior.  Remove flag and support code for when flag
is false.

PiperOrigin-RevId: 601570796
Change-Id: Ied4bbbcf0087e551f99dcc914276d9b027290b6e
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

4 participants