bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
23k stars 4.03k forks source link

incompatible_objc_provider_remove_linking_info #19000

Open googlewalt opened 1 year ago

googlewalt commented 1 year ago

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:

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 commented 1 year ago

@keith @brentleyjones FYI.

brentleyjones commented 1 year ago

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.

googlewalt commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Good point. The relevant bazel change is https://github.com/bazelbuild/bazel/commit/28f905675c98c652badbcbc7510134aab08dc4fb. Using an empty objc should suffice.