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

Support aspect_hints for improved Swift interop compatibility #1191

Conversation

aaronsky
Copy link
Contributor

@aaronsky aaronsky commented Apr 3, 2024

This PR is a rebase of #691 as I am personally motivated to see it merged, with tests updated to minimize breakage and docs update to reflect the current state of things. Credit for the original PR goes to @keith and @allevato.

Notably, it introduces a swift_interop_hint rule and new fields on swift_common.create_swift_interop_info to allow for greater customization of swift_clang_module_aspect behavior when running against objc_* rules. It also removes swift_c_module, as its behavior is now redundant. It introduces a breaking change, where the swift_module= tag no longer does anything. The swift_interop_hint aspect hint is expected its place.

This PR requires --experimental_enable_aspect_hints to take advantage of. If you are using Bazel 7, this is enabled by default. If you are using Bazel 6, this should be provided explicitly in a bazelrc or CLI invocation.

Closes #691. Fixes #1184.

Copy link

google-cla bot commented Apr 3, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@aaronsky aaronsky force-pushed the aaronsky/add-the-swift_interop_hint-rule-to-be-used-with-aspect_hints branch from 01e3255 to a02747a Compare April 3, 2024 13:38
@aaronsky aaronsky marked this pull request as draft April 3, 2024 13:39
@aaronsky aaronsky marked this pull request as ready for review April 3, 2024 14:21
.bazelrc Outdated Show resolved Hide resolved
test/interop_hints_tests.bzl Outdated Show resolved Hide resolved
test/interop_hints_tests.bzl Outdated Show resolved Hide resolved
swift/internal/compiling.bzl Outdated Show resolved Hide resolved
swift/internal/swift_clang_module_aspect.bzl Outdated Show resolved Hide resolved
.bazelci/presubmit.yml Outdated Show resolved Hide resolved
test/explicit_modules_test.bzl Outdated Show resolved Hide resolved
test/rules/provider_test.bzl Show resolved Hide resolved
@brentleyjones brentleyjones force-pushed the aaronsky/add-the-swift_interop_hint-rule-to-be-used-with-aspect_hints branch from 11e1be0 to b40d59a Compare April 5, 2024 16:14
@brentleyjones
Copy link
Collaborator

brentleyjones commented Apr 5, 2024

I'm going to clean up the commits a little bit, so that our adjustments are included in the google ones. Then I'll merge it without a squash.

allevato and others added 10 commits April 5, 2024 14:11
The new `aspect_hints` attribute in Bazel, available on all rules, lets us attach arbitrary providers to targets that `swift_clang_module_aspect` can read. The `swift_interop_hint` rule uses this to provide the module name for whichever target references it in its `aspect_hints`.

A canonical auto-deriving hint is also provided as part of the Swift build rules (in `.../swift:auto_module`), so that the default/recommended behavior of deriving a module name from the target label can be easily obtained without having to declare one's own `swift_module_hint` targets.

This is a more principled approach to handling Swift interop than the current `tags` implementation (which will be removed in a future change), and lets us associate additional metadata easily in the future, including files (for example, custom module maps or APINotes).

PiperOrigin-RevId: 387147846
(cherry picked from commit c42a37a)
This allows rules like `cc_library` to associate a custom module map if needed (however, this should be rare and used sparingly). It also eliminates the need for the `swift_c_module`, which will be removed.

Added analysis tests around the propagation of the module map artifacts.

PiperOrigin-RevId: 387195026
(cherry picked from commit 1356365)
…using `swift_interop_hint`.

PiperOrigin-RevId: 387355219
(cherry picked from commit 81c3074)
Its functionality has been replaced by `swift_import_hint` and `aspect_hints`.

PiperOrigin-RevId: 387358449
(cherry picked from commit 553f697)
…t_interop_hint`.

PiperOrigin-RevId: 388242317
(cherry picked from commit 6ded44e)
Interop for non-Obj-C rules should now exclusively use `aspect_hints` (see the documentation for `swift_interop_hint`).

PiperOrigin-RevId: 388940287
(cherry picked from commit a67043f)
… that generate them by default, like `objc_library`.

PiperOrigin-RevId: 391087374
(cherry picked from commit de0f604)
This attribute can be used to exclude a list of headers from the Swift-generated module map (via `exclude header` declarations) without removing them from the hinted target completely. This is often helpful in cases where some subset of headers are not Swift-compatible but still needed as part of the library for other reasons (e.g., they are private headers used by implementation source files, or still used by other non-Swift dependents).

PiperOrigin-RevId: 398076709
(cherry picked from commit ef6662c)
…t_interop_hint.exclude_hdrs` on a `cc_library` that has `include_prefix` and/or `strip_include_prefix` set.

PiperOrigin-RevId: 412917671
(cherry picked from commit 6b13232)
@brentleyjones brentleyjones force-pushed the aaronsky/add-the-swift_interop_hint-rule-to-be-used-with-aspect_hints branch from 395c58c to 318172f Compare April 5, 2024 19:17
@brentleyjones brentleyjones enabled auto-merge (rebase) April 5, 2024 19:18
@brentleyjones brentleyjones merged commit 1bb582d into bazelbuild:master Apr 5, 2024
14 of 15 checks passed
brentleyjones referenced this pull request Apr 5, 2024
The new `aspect_hints` attribute in Bazel, available on all rules, lets us attach arbitrary providers to targets that `swift_clang_module_aspect` can read. The `swift_interop_hint` rule uses this to provide the module name for whichever target references it in its `aspect_hints`.

A canonical auto-deriving hint is also provided as part of the Swift build rules (in `.../swift:auto_module`), so that the default/recommended behavior of deriving a module name from the target label can be easily obtained without having to declare one's own `swift_module_hint` targets.

This is a more principled approach to handling Swift interop than the current `tags` implementation (which will be removed in a future change), and lets us associate additional metadata easily in the future, including files (for example, custom module maps or APINotes).

PiperOrigin-RevId: 387147846
brentleyjones referenced this pull request Apr 5, 2024
This allows rules like `cc_library` to associate a custom module map if needed (however, this should be rare and used sparingly). It also eliminates the need for the `swift_c_module`, which will be removed.

Added analysis tests around the propagation of the module map artifacts.

PiperOrigin-RevId: 387195026
brentleyjones referenced this pull request Apr 5, 2024
…using `swift_interop_hint`.

PiperOrigin-RevId: 387355219
brentleyjones referenced this pull request Apr 5, 2024
Its functionality has been replaced by `swift_import_hint` and `aspect_hints`.

PiperOrigin-RevId: 387358449
brentleyjones referenced this pull request Apr 5, 2024
brentleyjones referenced this pull request Apr 5, 2024
Workaround for not being able to merge the change that adds
`swift_interop_hint` rule.
brentleyjones referenced this pull request Apr 5, 2024
Interop for non-Obj-C rules should now exclusively use `aspect_hints` (see the documentation for `swift_interop_hint`).

PiperOrigin-RevId: 388940287
brentleyjones referenced this pull request Apr 5, 2024
… that generate them by default, like `objc_library`.

PiperOrigin-RevId: 391087374
brentleyjones referenced this pull request Apr 5, 2024
This attribute can be used to exclude a list of headers from the Swift-generated module map (via `exclude header` declarations) without removing them from the hinted target completely. This is often helpful in cases where some subset of headers are not Swift-compatible but still needed as part of the library for other reasons (e.g., they are private headers used by implementation source files, or still used by other non-Swift dependents).

PiperOrigin-RevId: 398076709
brentleyjones referenced this pull request Apr 5, 2024
…t_interop_hint.exclude_hdrs` on a `cc_library` that has `include_prefix` and/or `strip_include_prefix` set.

PiperOrigin-RevId: 412917671
brentleyjones referenced this pull request Apr 5, 2024
… J2ObjC

Targets processed by J2ObjC's aspects have a new compilation context created that treats the generated J2ObjC headers as textual headers, and the "umbrella" header as a regular header. The root path of the generated headers is added to the compilation context as an include path so the headers can be found by dependents.

To handle `java_libary`'s `exports` attribute (this lists targets that should be treated as a direct dependency when depending on the `java_libary` in question), the `SwiftInfos` from dependencies are split into direct and indirect. These are then propagated as `direct_swift_infos` vs `swift_infos` respectively to provide the semantics of `exports`.

The modular import header rewriter was updated to accommodate the fact that the J2ObjC "umbrella" header is no longer marked as being an umbrella header in the module map. It still needs to be rewritten for the same reason as before, but must now be detected by a portion of the file path (🤮).

PiperOrigin-RevId: 416355827
brentleyjones referenced this pull request Apr 5, 2024
…o that

layering checks are still satisfied correctly.

PiperOrigin-RevId: 416824355
brentleyjones referenced this pull request Apr 5, 2024
The direct_headers field in ObjcProvider is being deprecated and will
be removed.  The same information can be found in CcInfo.
J2ObjcAspect has been modified so that the CcInfo is accessible via
Starlark.

PiperOrigin-RevId: 417648590
brentleyjones referenced this pull request Apr 5, 2024
…iptor in `swift_clang_module_aspect`.

This makes it so that the `apple_common.Objc` provider no longer needs to be handled separately for compilation by Swift build APIs. (It is still used for linking until that is migrated entirely onto `CcInfo`.)

PiperOrigin-RevId: 423822059
brentleyjones referenced this pull request Apr 5, 2024
RELNOTES: None
PiperOrigin-RevId: 430276243
@aaronsky aaronsky deleted the aaronsky/add-the-swift_interop_hint-rule-to-be-used-with-aspect_hints branch April 7, 2024 17:46
BalestraPatrick added a commit to bazelbuild/rules_apple that referenced this pull request Jun 3, 2024
…2465)

This was recently enabled in rules_swift (with
bazelbuild/rules_swift#1191) and it seems to be
required in the next `rules_swift` bump. I saw the failure initially
when testing a rules_swift bump in #2418 which was
[failing](https://buildkite.com/bazel/rules-apple-darwin/builds/9191#018fdd69-abfb-451a-a454-4b0771e26b77)
with the following error only in the Bazel 6.x configuration as expected
(since it's enabled by default on Bazel 7):
```
Error: No attribute 'aspect_hints' in attr. Make sure you declared a rule attribute with this name.
```
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jun 27, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in: bazelbuild/rules_swift#1191
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jun 27, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in: bazelbuild/rules_swift#1191
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jun 27, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in: bazelbuild/rules_swift#1191
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jul 3, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in: bazelbuild/rules_swift#1191
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jul 3, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in: bazelbuild/rules_swift#1191
luispadron added a commit to bazel-ios/rules_ios that referenced this pull request Jul 3, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in:
bazelbuild/rules_swift#1191

Fixes #875
nataliejameson pushed a commit to discord/rules_ios that referenced this pull request Aug 13, 2024
This fixes a missing `_swift_vfs.yaml` error related to the changes in:
bazelbuild/rules_swift#1191

Fixes #875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Umbrella objc_library in experimental_mixed_language_library produces malformed .swift.modulemap
7 participants