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

Add header_map rule #2352

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Add header_map rule #2352

merged 1 commit into from
Jan 19, 2024

Conversation

luispadron
Copy link
Contributor

@luispadron luispadron commented Jan 1, 2024

This adds a new header_map rule for creating binary .hmap files which are required to support #import <Foo/Foo.h> semantics without restructuring an entire project.

Header maps are generated by default when using Xcode, so projects built in Xcode tend to end up relying on them. When migrating an existing project to use Bazel this lack of header map support has been a source of confusion in several threads on the Bazel Slack. Users either end up using rules_ios, which this implementation comes from, or they end up copying the header map rule locally.

By adding it to rules_apple we can help folks who are migrating existing projects. Along with documentation to make this less confusing.

My goal here is to start a discussion around the addition of the rule and it's implementation and eventually extend experimental_mixed_language_library to support generating these header maps (with this rule) when requested.

Closes #1757

Open questions

- Do we want to use the C implementation from rules_ios? There is a Swift implementation we could look at taking from
- If we do want to use the C implementation, should we spend some time getting the dependencies (lines/uthash) as Bazel modules (or at least consumed via download) vs. copying the sources into the source tree?

Decided on using Swift implementation

@luispadron luispadron force-pushed the luis/add-header-map-rule branch 2 times, most recently from 2d924ee to 5b43171 Compare January 1, 2024 22:42
@luispadron luispadron force-pushed the luis/add-header-map-rule branch 9 times, most recently from 894cf5b to 6303159 Compare January 2, 2024 23:20
@luispadron
Copy link
Contributor Author

luispadron commented Jan 2, 2024

I added a Swift implementation to see the difference between the two. Either works for me but Swift requires a bit less LOC to maintain.

@luispadron
Copy link
Contributor Author

@keith @brentleyjones for when you two have a chance to review. I can add tests and such when we align on the hmap implementation

apple/header_map.bzl Show resolved Hide resolved
tools/hmaptool/BUILD Outdated Show resolved Hide resolved
@thii
Copy link
Member

thii commented Jan 11, 2024

Do we want to use the C implementation from rules_ios? There is a Swift implementation we could look at taking from

No strong opinion on this—I think this would rarely change, so either is fine. It would be great if you could do a perf test on a large project and/or add a Starlark flag to allow switching.

If we do want to use the C implementation, should we spend some time getting the dependencies (lines/uthash) as Bazel modules (or at least consumed via download) vs. copying the sources into the source tree?

Yes, I think we should (as a non-module dependency is fine). (Same if we use the Swift implementation)

@luispadron
Copy link
Contributor Author

Thanks for the review @thii, some thoughts:

No strong opinion on this—I think this would rarely change, so either is fine. It would be great if you could do a perf test on a large project and/or add a Starlark flag to allow switching.

Lets go with Swift then, it's less code and a bit easier to keep maintained IMO.

If we do want to use the C implementation, should we spend some time getting the dependencies (lines/uthash) as Bazel modules (or at least consumed via download) vs. copying the sources into the source tree?

Yes, I think we should (as a non-module dependency is fine). (Same if we use the Swift implementation)

I actually changed the Swift one up a bit and had to make my own CLI around the headermap code so we'd just keep the two files in here for that one.

ghost

This comment was marked as resolved.

Copy link
Contributor

@mattrobmattrob mattrobmattrob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my preference that we have an explicit mixed language example with header_map used and one without, if that's not too difficult. I think not muddling them together might make the path for folks more clear.

I didn't review the tools/hmaptool very closely since I'm not that familiar with the format/this functionality. 🙏

tools/hmaptool/README.md Outdated Show resolved Hide resolved
examples/multi_platform/MixedLib/BUILD Outdated Show resolved Hide resolved
examples/multi_platform/MixedLib/BUILD Outdated Show resolved Hide resolved
@luispadron
Copy link
Contributor Author

It's my preference that we have an explicit mixed language example with header_map used and one without, if that's not too difficult. I think not muddling them together might make the path for folks more clear.

Do you mean you want me to add a new example specifically for header_map? If so, I can do that!

@mattrobmattrob
Copy link
Contributor

It's my preference that we have an explicit mixed language example with header_map used and one without, if that's not too difficult. I think not muddling them together might make the path for folks more clear.

Do you mean you want me to add a new example specifically for header_map? If so, I can do that!

That's my preference. Something like (1) an example without header map and (2) an example with header map. In the future, it'll be easy to use it to demonstrate the two different pathways for the mixed language macro. 🙏

@luispadron
Copy link
Contributor Author

@mattrobmattrob Makes sense, I added a new examples/multi_platform/MixedLibWithHeaderMap example and reverted the changes to the existing example.

@luispadron luispadron enabled auto-merge (squash) January 19, 2024 17:25
@luispadron luispadron merged commit 2b2341b into master Jan 19, 2024
9 checks passed
@luispadron luispadron deleted the luis/add-header-map-rule branch January 19, 2024 19:24
luispadron added a commit that referenced this pull request Jan 20, 2024
This follows up #2352 by adding header map support to
`experimental_mixed_language_library`. It adds a new parameter:
`enable_header_map` which if `True` will generate a headermap for the
mixed language `objc_library`. This header map will be included by the
`swift_library` and `objc_library` to enable `#import <>` semantics.

Closes #2371

Co-authored-by: Matt Robinson <mattrob@hey.com>
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.

Add headermap include documentation
4 participants