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

Migrate ObjcProvider compile info to CcCompilationContext #10674

Closed
googlewalt opened this issue Jan 29, 2020 · 6 comments
Closed

Migrate ObjcProvider compile info to CcCompilationContext #10674

googlewalt opened this issue Jan 29, 2020 · 6 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@googlewalt
Copy link
Contributor

googlewalt commented Jan 29, 2020

Overview

This issue describes the plans to migrate compile info from ObjcProvider to CcCompilationContext. It will focus on the main migration step, which moves the compilation info in ObjcProvider into an embedded CcCompilationContext. Note this step is Starlark API compatible (except for one secondary feature). The issue will also describe any expected changes in behavior and the followup cleanup work.

Scope

What is being migrated:

  • Headers, defines, all four flavors of include paths (regular, quote, system, framework).

What is not being migrated:

  • Module maps, objc_proto strict dependency include.
  • Any non-compile info.

Initial rollout is Starlark API compatible, except for the strict dependency propagation feature. Later cleanups may break compatibility.

Changes to ObjcProvider

The migration will make the following changes to ObjcProvider:

  • The migrated fields will be stored in an embedded CcCompilationContext, instead of directly as items in ObjcProvider. The items for those fields in ObjcProvider will become empty.
  • Strict dependency and non-propagated items will no longer be supported generically. Instead, the only intended remaining use of strict dependency is for objc_proto_library includes (which is only used within Google), and that will be moved to a special field in ObjcProvider.
  • Strict dependency includes will no longer be propagated to a dependent ObjcProvider. Instead, to get the strict dependency behavior, a rule should fetch that information directly from its dependencies. For an example of how to do this, see swift_library which is already set up this way.
  • The ObjcProvider Starlark API will have a new accessor field (compilation_context) to access its CcCompilationContext. This field is a temporary measure to ease migration, and will be deleted once all the cleanup tasks are completed.
  • Otherwise, the Starlark API will remain the same.

Changes to native rules

The migration will make the following changes the native rules:

  • Previously, objc_library exports a CcInfo with an incomplete CcCompilationContext that only has header information. Now it will export the complete CcCompilationContext, identical to the one embedded in its ObjcProvider.

    Note this change can expose bugs/issues in rules that depend on objc_library and consume its CcInfo.

  • Rules that produce final library or executable artifacts (e.g. apple_binary, apple_static_libary) generate ObjcProviders for linking. These ObjcProviders will no longer carry compilation information.

Other changes in behavior that may impact users

  • Private headers (those in srcs attribute) of objc_library are now propagated to dependencies. This behavior makes objc_library more consistent with that for cc_library.
  • The quote include paths now contain ".", the root of the source directory. Previously this path was added implicitly to every compile, but never put into the provider.
  • The ordering of defines and include paths will likely change, in ways that don't affect correctness. Furthermore, the Starlark APIs for accessing these fields only provide stable order, so with the change in underlying representation that is even more likely to change. Tulsi unit tests, which assumes a specific ordering, would need to be remastered.

Internal changes

This section describes the changes made to the blaze internals for the migration. It is mainly of interest to code reviewers, or those who are interested in the inner workings of Objective C/C++ rules. It may be skipped by most users.

Migrating internal uses of ObjcProvider

ObjcProvider is used not only for information exchange between rules, it is used internally as well. The migration covers these uses. There are three issues in migrating them:

  1. objc_library's compile action depends on its own provider.

    Currently, a compilation rule such as objc_library generates its ObjcProvider, then uses it to generate its compile action. This flow is problematic for the migration, because the compile action generates a CcCompilationContext that we want to use to create the ObjcProvider -- so the migration would cause a circular dependency in this flow.

    The migration requires breaking this circular dependency. A compilation rule is modified so that it collects all the compilation info into a new object called ObjcCompilationContext, which serves as a substitute for the way ObjcProvider was used before in its own compilation. Whereas ObjcCommon used to create an ObjcProvider up front, it now creates an ObjcCompilationContext and an ObjcProvider.Builder, the latter of which only contains non-compile information. The creation of the final ObjcProvider is postponed until CompilationSupport.compile() is called.

  2. Distinguish between two kinds of uses of ObjcCommon.

    Because of the the aforementioned change, it is useful to distinguish between two use cases of ObjcCommon: those that are used to create compilation actions and those that are not.

    • ObjcCommon used to create compilation actions needs to export an ObjcProvider.Builder, and rely on CompilationSupport.compile() to create the final ObjcProvider that has the CcCompilationContext from the compile action.
    • ObjcCommon that do not create compilation actions can export an ObjcProvider directly, as before.

    We define a enum ObjcCommon.Purpose, set during the creation of ObjcCommon, to distinguish the two use cases.

  3. Implementation of strict dependency propagation

    Changing objc_library so that it doesn't depend on its own provider also affects how strict dependency propagation is implemented. Because we fetch strict dependency items directly from the dependent providers, rather than indirectly through a new provider, we no longer need to propagate those items. This simplifies implementation and allows us to get rid of the wonky propagation mechanism altogether.

    The only remaining use of strict dependency propagation is for objc proto library include paths, so we just store that in a normal field in ObjcProvider that is not propagated.

Clean up Objc to Cc hooks

The migration to CcCompilationContext allows us to get rid of a bunch of existing hacky hooks used to get Objective-C compilation to work properly with underlying C++ infrastructure. The following hooks are used to work around include scanning issues, which are obsoleted by the migration and can be deleted:

  • The concepts of additional prunable includes and alternate include scanning data input (see ObjcCppSemantics and CppCompilationAction).
  • The hack in CompilationSupport.ccCompileAndLink().

Followup plan

The followup work can be divided into three phases:

  1. Migrate all native and Starlark rules to generate CcInfo / CcCompilationContext for compile info. This info will be identical to information stored in the CcCompilationContext embedded in ObjcProvider.
  2. Migrate all native and Starlark rules to use CcInfo for compile info.
  3. Migrate all rules to stop generating compile info in ObjcProvider. Remove the ObjcProvider compile-info related Starlark APIs.

I will migrate the native rules, apple rules, swift rules, and tulsi as described above. Users also need to migrate any Starlark rules they own that use ObjcProvider for compile information. We will provide two incompatible flags to aid the migration:

--incompatible_objc_compile_info_migration: This flag controls whether native rules will use compile info from ObjcProvider or CcInfo. If the flag is false, bazel will get its compile info from ObjcProvider (pre-migration behavior). If the flag is true, bazel will get its compile info from CcInfo (post-migration behavior).

Tentative time-frame: available and default false in 3.0, default true the next release (i.e. 3.1), removed the following release (i.e. 3.2).

--incompatible_objc_provider_remove_compile_info: This flag deletes the Starlark APIs to put compile info in an ObjcProvider.

Tentative time-frame: available and default false in 3.2, default true next release (i.e. 4.0), removed in the following release (i.e. 4.1).

We recommend that rules be migrated as follows:

  1. Migrate Starlark rules that define ObjcProvider with compile info.

    These are rules that insert any of the following fields into ObjcProvider: define, framework_search_paths, header, include, include_system, iquote. For each such rule, generate a CcInfo containing a CcCompilationContext with the same information, and add it to list of providers emitted by that rule.

    There is a 1:1 correspondence between these fields and where they should land in CcCompilationContext:

    • define -> defines
    • framework_search_paths -> framework_includes
    • header -> headers
    • include -> includes
    • include_system -> system_includes
    • iquote -> quote_includes
  2. Make sure all bazel Starlark rules you are using have been migrated up to this step (you may have to update your rules_apple, etc.). Flip --incompatible_objc_compile_info_migration to true, which will help test whether you have migrated all your rules in step (1) properly.

  3. Migrate Starlark rules that use compile info in ObjcProvider. These rules should be migrated to use compile info from Ccinfo instead. This migration may include migrating rules that propagate compile info from its deps, which can be done using cc_common.merge_cc_infos.

  4. Migrate Starlark rules so that they stop adding compile info to ObjcProvider.

  5. Make sure all bazel Starlark rules you are using have been migrated up to this step (you may have to update your rules_apple, etc.) Flip --incompatible_objc_provider_remove_compile_info to true. This will help test whether you have completed the migration.

I will attach examples of the migrations as they become available, in rules_apple/rules_swift/tulsi.

@iirina iirina added team-Rules-CPP Issues for C++ rules untriaged labels Jan 29, 2020
@oquenchil oquenchil added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Feb 18, 2020
@aiuto aiuto assigned aiuto and googlewalt and unassigned aiuto Feb 18, 2020
@aiuto aiuto added the z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple label Feb 18, 2020
bazel-io pushed a commit that referenced this issue Feb 19, 2020
See #10674.

RELNOTES: None
PiperOrigin-RevId: 295898085
swiple-rules-gardener pushed a commit to bazelbuild/rules_swift that referenced this issue Feb 28, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297752500
swiple-rules-gardener pushed a commit to bazelbuild/rules_apple that referenced this issue Feb 28, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297752932
swiple-rules-gardener pushed a commit to bazelbuild/rules_swift that referenced this issue Feb 28, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297752500
swiple-rules-gardener pushed a commit to bazelbuild/rules_apple that referenced this issue Feb 28, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297752932
swiple-rules-gardener pushed a commit to bazelbuild/rules_apple that referenced this issue Feb 28, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297920880
swiple-rules-gardener pushed a commit to bazelbuild/rules_swift that referenced this issue Feb 29, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 297752500
swiple-rules-gardener pushed a commit to bazelbuild/rules_swift that referenced this issue Feb 29, 2020
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 298008786
bazel-io pushed a commit that referenced this issue Mar 2, 2020
Part of overall migration of compile info to CcInfo.  See
#10674.

RELNOTES: None
PiperOrigin-RevId: 298466963
copybara-service bot pushed a commit to bazelbuild/rules_apple that referenced this issue Apr 22, 2020
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 296338258
copybara-service bot pushed a commit to bazelbuild/rules_swift that referenced this issue Apr 22, 2020
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 296338258
copybara-service bot pushed a commit to bazelbuild/rules_swift that referenced this issue Apr 22, 2020
Previous CL missed an include path.

Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 307730265
copybara-service bot pushed a commit to bazelbuild/rules_swift that referenced this issue Apr 22, 2020
Previous CL missed an include path.

Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 307730265
copybara-service bot pushed a commit to bazelbuild/rules_swift that referenced this issue Apr 22, 2020
Previous CL missed an include path.

Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 307730265
copybara-service bot pushed a commit to bazelbuild/rules_swift that referenced this issue Apr 22, 2020
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 294567121
@googlewalt
Copy link
Contributor Author

The migration is complete. The migration incompatible flags were flipped in Bazel 4.0, and subsequently deleted.

siddharthab pushed a commit to grailbio/bazel-compilation-database that referenced this issue Sep 8, 2021
These APIs were deprecated a while ago, but Bazel 4.0.0 has removed them completely. The information in `ObjCProvider` has been migrated to `CcCompilationContext`: bazelbuild/bazel#10674

I've replaced the usages of the deprecated APIs with the equivalents from `CcCompilationContext` and can confirm that it now works with Bazel 4.0.0.
jparise pushed a commit to pinterest/tulsi that referenced this issue Sep 8, 2021
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

PiperOrigin-RevId: 307870523
jparise pushed a commit to pinterest/tulsi that referenced this issue Sep 8, 2021
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

PiperOrigin-RevId: 307870523
jparise added a commit to jparise/xchammer that referenced this issue Sep 8, 2021
This represents the minimal set of changes to get this project through
the upstream Bazel project's ObjcProvider migration:

bazelbuild/bazel#10674

In addition to updating our direct WORKSPACE dependencies, it also bumps
the commit reference to our (quite stale) Tulsi fork that now contains
these cherry-picked upstream commits:

- pinterest/tulsi@3d4f37a
- pinterest/tulsi@a44bc1c
- pinterest/tulsi@0c7ae95
jparise added a commit to jparise/xchammer that referenced this issue Sep 8, 2021
This represents the minimal set of changes to get this project through
the upstream Bazel project's ObjcProvider migration:

bazelbuild/bazel#10674

In addition to updating our direct WORKSPACE dependencies, it also bumps
the commit reference to our (quite stale) Tulsi fork that now contains
these cherry-picked upstream commits:

- pinterest/tulsi@3d4f37a
- pinterest/tulsi@a44bc1c
- pinterest/tulsi@0c7ae95
jparise added a commit to jparise/xchammer that referenced this issue Sep 8, 2021
This represents the minimal set of changes to get this project through
the upstream Bazel project's ObjcProvider migration:

bazelbuild/bazel#10674

In addition to updating our direct WORKSPACE dependencies, it also bumps
the commit reference to our (quite stale) Tulsi fork that now contains
these cherry-picked upstream commits:

Branch: xchammer-0.28.1
- pinterest/tulsi@432cdb6
- pinterest/tulsi@62cfb95
- pinterest/tulsi@131e87b

Branch: xchammer-3.4.1
- pinterest/tulsi@3d4f37a
- pinterest/tulsi@a44bc1c
- pinterest/tulsi@0c7ae95
jparise added a commit to jparise/xchammer that referenced this issue Sep 8, 2021
This represents the minimal set of changes to get this project through
the upstream Bazel project's ObjcProvider migration:

bazelbuild/bazel#10674

In addition to updating our direct WORKSPACE dependencies, it also bumps
the commit reference to our (quite stale) Tulsi fork that now contains
these cherry-picked upstream commits:

Branch: xchammer-0.28.1
- pinterest/tulsi@432cdb6
- pinterest/tulsi@62cfb95
- pinterest/tulsi@131e87b

Branch: xchammer-3.4.1
- pinterest/tulsi@3d4f37a
- pinterest/tulsi@a44bc1c
- pinterest/tulsi@0c7ae95
jparise added a commit to bazel-xcode/xchammer that referenced this issue Sep 8, 2021
This represents the minimal set of changes to get this project through
the upstream Bazel project's ObjcProvider migration:

bazelbuild/bazel#10674

In addition to updating our direct WORKSPACE dependencies, it also bumps
the commit reference to our (quite stale) Tulsi fork that now contains
these cherry-picked upstream commits:

Branch: xchammer-0.28.1
- pinterest/tulsi@432cdb6
- pinterest/tulsi@62cfb95
- pinterest/tulsi@131e87b

Branch: xchammer-3.4.1
- pinterest/tulsi@3d4f37a
- pinterest/tulsi@a44bc1c
- pinterest/tulsi@0c7ae95
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Step 3 of 3-step plan:
    1. Migrate the definitions.
    2. Migrate the uses.
    3. Delete old ObjcProvider definitions.

    See bazelbuild/bazel#10674.

    Now that compilation info has been migrated from ObjcProvider to
    CcCompilationContext, get rid of some logic used for the migration.

    Before the migration, ObjcCommon.build() produced an ObjcProvider.

    For the migration, I added a CcCompilationContext inside the
    ObjcProvider.  This caused two issues:

    1. the CcCompilationContext may not exist during ObjcCommon.build(),
       so it would not be possible to fully construct ObjcProvider then.

    2. Depending on the use case, CcCompilationContext may be produced in
       two different ways.

    So I changed ObjcCommon to produce a Builder for ObjcProvider instead,
    with its CcCompilationContext to be set later when it is available.

    Now that the migration is complete, we no longer need the
    CcCompilationContext in ObjcProvider, so delete all this logic, and
    revert to having ObjcCommon generate an ObjcProvider with an empty
    CcCompilationContext.  I will delete ObjcProvider's
    CcCompilationContext in a subsequent CL.

    I also had to update the tests so that they use CcInfo instead of
    ObjcProvider to find objc rules' compile info.

    PiperOrigin-RevId: 357642600
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Part of overall migration of compile info to CcInfo.  See
    bazelbuild/bazel#10674.

    RELNOTES: None
    PiperOrigin-RevId: 298466963
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 298008786
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
Previous CL missed an include path.

Step 1 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 307835766
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
Note strict includes still come from ObjcProvider.

Step 2 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

RELNOTES: None
PiperOrigin-RevId: 307965879
tymurmustafaiev pushed a commit to tymurmustafaiev/rules_swift that referenced this issue Jul 19, 2023
Step 3 of 3-step plan:
1. Migrate the definitions.
2. Migrate the uses.
3. Delete old ObjcProvider definitions.

See bazelbuild/bazel#10674.

A couple extra notes:

- Also migrated a couple remaining uses in
  swift_clang_module_aspect.bzl.  Some Starlark rules still export a
  (empty) ObjcProvider but not a CcInfo, so we still need to handle
  that case.

- Use of "header" is not removed, because it still populates direct
  fields in ObjcProvider.

RELNOTES: None
PiperOrigin-RevId: 320402849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

No branches or pull requests

7 participants