Mac Catalyst support for bazel / rules_swift / rules_apple #24
Conversation
This is the first of a few PRs to add Mac Catalyst support for Bazel. I have an app that can be successfully built with a few manual patches to the bundle (proper actool usage, different bundle Info.plist, codesign, and notarization). But overall, it can be successful built with an app that passes macOS AppStore review / automatic checks. This is WIP, as I need to migrate a few manual input flags into crosstool section, and need advices from Bazel maintainers.
|
Note that I still need to build with the following command: I think that I need to move some of the linker flags into the crosstool as well. |
|
Updated. Now the command changed to: @keith let me know if you think the swift specific linkopt should be moved to crosstool too. |
|
should probably be in rules_swift |
|
Then this PR is ready for review! |
|
Hi, let me know if there are any suggestions how to move this PR forward. Other PRs (in rules_apple / rules_swift) depends on this PR gets merged ... |
|
@keith I'm interested in this too. I filed an issue about this here: There's also an issue on the rules_apple side from a while ago which I think you also participated in, saying we need the crosstool changes: It looks like rules_swift already supports catalyst: If we want to use a swift_framework in an ios_application or use an objc library we need changes in rules_apple and bazel though. |
|
@googlewalt let me know if there are anything you want me to fix in this PR to make it mergeable. Thanks! |
WalkthroughThis update introduces support for the "catalyst_arm64" CPU architecture across the Apple platform toolchain configuration. The change expands the set of valid Catalyst target CPUs, updates documentation to reflect the new platform and platform type, and incorporates the new architecture into build constraints, toolchain features, and architecture lists. Several toolchain configuration features are extended or added to handle the new Catalyst CPUs, ensuring consistent compiler and linker behavior. All modifications are additive, with no removals or changes to public API signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant BuildSystem
participant ToolchainConfig
participant Compiler
participant Linker
BuildSystem->>ToolchainConfig: Request build for catalyst_arm64 target
ToolchainConfig->>ToolchainConfig: Identify catalyst_arm64 as valid CPU
ToolchainConfig->>Compiler: Apply default flags, bitcode, frameworks, etc.
ToolchainConfig->>Linker: Apply ios_support_link_flags_feature
Compiler-->>ToolchainConfig: Compilation with correct flags
Linker-->>ToolchainConfig: Linking with iOSSupport framework paths
ToolchainConfig-->>BuildSystem: Build completed for catalyst_arm64
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Seems there are quite a bit of changes on the master branch, need to do more work to fix this. I will work with @keith later this week to bring this branch up to date. |
|
@liuliu Can you bring this up to date? Might also be worth discussing in the #rules_apple-maintainers-discuss Slack channel. |
|
Yeah, I've been using the forked version to release Draw Things for almost a year now. Need to update it for Xcode 15. Will find some time in December. There are a lot of changes in Bazel side since this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces initial Mac Catalyst support to Bazel's rules_swift and rules_apple. The primary goal is to enable building applications that can run on macOS using the Catalyst framework. The changes involve adding 'catalyst_x86_64' and 'catalyst_arm64' as supported target CPUs, updating the crosstool configuration to include Mac Catalyst specific settings, and adjusting compiler and linker flags accordingly. This is a work in progress (WIP) and requires further refinement, particularly in migrating manual input flags into the crosstool section.
Highlights
- Mac Catalyst Support: Adds initial support for Mac Catalyst to build applications for macOS.
- Target CPU Addition: Includes 'catalyst_x86_64' and 'catalyst_arm64' as supported target CPUs in ApplePlatform.
- Crosstool Configuration Updates: Modifies the crosstool configuration to incorporate Mac Catalyst specific settings and flags.
- Compiler and Linker Flags: Adjusts compiler and linker flags to properly build for Mac Catalyst.
Changelog
Click here to see the changelog
- src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java
- Adds 'catalyst_arm64' to the list of supported target CPUs for the Catalyst platform. This allows Bazel to recognize and build for ARM64-based Mac Catalyst applications.
- Line 58: Added
catalyst_arm64to theCATALYST_TARGET_CPUSImmutableSet.
- src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformApi.java
- Adds 'catalyst' to the list of available platforms in the apple_common.platform struct. This makes the Catalyst platform accessible in Starlark build files.
- Line 36: Added
<li><code>apple_common.platform.catalyst</code></li>to the documentation.
- src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformTypeApi.java
- Adds 'catalyst' to the list of available platform types in apple_common.platform_type. This allows users to specify the Catalyst platform type in Starlark build files.
- Line 33: Added
<li><code>apple_common.platform_type.catalyst</code></li>to the documentation.
- tools/osx/crosstool/BUILD.toolchains
- Adds constraints for 'catalyst_arm64' and 'catalyst_x86_64' to define the supported OS and CPU architectures for Mac Catalyst.
- Lines 26-33: Added new constraint definitions for
catalyst_arm64andcatalyst_x86_64.
- tools/osx/crosstool/cc_toolchain_config.bzl
- Adds logic to determine the target system name for Mac Catalyst based on the CPU architecture ('catalyst_x86_64' or 'catalyst_arm64').
- Lines 100-103: Added conditional logic to set
target_system_namefor Catalyst architectures. - Lines 699-701: Added
catalyst_x86_64andcatalyst_arm64to the list of CPUs that apply default compiler flags. - Lines 951-952: Added
catalyst_x86_64andcatalyst_arm64to the list of CPUs that contain Objective-C source. - Lines 1109-1133: Added a feature to apply iOS support link flags for Catalyst, including framework and library paths.
- Lines 1297-1328: Added a feature to include system directories for Catalyst, including the iOSSupport framework path.
- Lines 1613-1614: Added
catalyst_x86_64andcatalyst_arm64to the list of CPUs that apply implicit frameworks. - Lines 2467-2468: Added
catalyst_x86_64andcatalyst_arm64to the list of CPUs that use the unified headers. - Lines 2727-2728: Added
catalyst_x86_64andcatalyst_arm64to the list of CPUs that use fastbuild features. - Line 2787: Added
ios_support_link_flags_featureto the list of features.
- tools/osx/crosstool/osx_archs.bzl
- Adds 'catalyst_x86_64' and 'catalyst_arm64' to the list of supported architectures.
- Lines 35-36: Added
catalyst_x86_64andcatalyst_arm64to theOSX_TOOLS_ARCHSlist.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
For macOS, an app takes flight,
Catalyst bridging day and night.
Bazel's rules expand,
Across the digital sand,
Code compiles, a wondrous sight.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Mac Catalyst support for Bazel, which is a valuable addition. The changes seem well-structured and logically sound. However, there are a few areas that could benefit from further attention to ensure correctness and maintainability.
Summary of Findings
- Missing platform checks: There are several places in
cc_toolchain_config.bzlwhere platform checks are performed usingctx.attr.cpu. It might be more robust to use a dedicated platform check function to ensure consistency and avoid potential errors if the CPU string changes in the future. I did not comment on all instances of this, as per the instructions. - Redundant feature definitions: The
include_system_dirs_featureandios_support_link_flags_featureare defined twice incc_toolchain_config.bzl, once inside anifblock for catalyst and once outside. This could be simplified to avoid code duplication. I did not comment on all instances of this, as per the instructions.
Merge Readiness
The pull request is a good step towards adding Mac Catalyst support. However, the identified issues should be addressed before merging to ensure the changes are robust and maintainable. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the presence of medium and high severity issues, I recommend that the pull request not be merged until those are addressed (at a minimum).
| private static final ImmutableSet<String> CATALYST_TARGET_CPUS = | ||
| ImmutableSet.of("catalyst_x86_64"); | ||
| ImmutableSet.of("catalyst_x86_64", "catalyst_arm64"); |
There was a problem hiding this comment.
Consider adding a comment explaining why both x86_64 and arm64 are included for Catalyst. This would improve readability and help future maintainers understand the reasoning behind this choice.
| private static final ImmutableSet<String> CATALYST_TARGET_CPUS = | |
| ImmutableSet.of("catalyst_x86_64"); | |
| ImmutableSet.of("catalyst_x86_64", "catalyst_arm64"); | |
| private static final ImmutableSet<String> CATALYST_TARGET_CPUS = | |
| ImmutableSet.of("catalyst_x86_64", "catalyst_arm64"); // Catalyst supports both x86_64 and arm64 architectures |
| if (ctx.attr.cpu == "catalyst_x86_64" or | ||
| ctx.attr.cpu == "catalyst_arm64"): | ||
| ios_support_link_flags_feature = feature( | ||
| name = "ios_support_link_flags", | ||
| enabled = True, | ||
| flag_sets = [ | ||
| flag_set( | ||
| actions = all_link_actions + | ||
| ["objc-executable", "objc++-executable"], | ||
| flag_groups = [ | ||
| flag_group( | ||
| flags = [ | ||
| "-iframeworkwithsysroot", | ||
| "System/iOS/System/Library/Frameworks", | ||
| "-L%{sdk_dir}/System/iOSSupport/usr/lib", | ||
| ], | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ) | ||
| else: | ||
| ios_support_link_flags_feature = feature( | ||
| name = "ios_support_link_flags" | ||
| ) |
There was a problem hiding this comment.
The ios_support_link_flags_feature is defined twice, once inside the if block for catalyst and once outside. This duplication can be avoided by defining the feature once and conditionally setting its enabled attribute based on the platform. This would improve code maintainability.
ios_support_link_flags_feature = feature(
name = "ios_support_link_flags",
enabled = (ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64"),
flag_sets = [
flag_set(
actions = all_link_actions +
["objc-executable", "objc++-executable"],
flag_groups = [
flag_group(
flags = [
"-iframeworkwithsysroot",
"System/iOS/System/Library/Frameworks",
"-L%{sdk_dir}/System/iOSSupport/usr/lib",
],
),
],
),
],
)
| if (ctx.attr.cpu == "catalyst_x86_64" or | ||
| ctx.attr.cpu == "catalyst_arm64"): | ||
| include_system_dirs_feature = feature( | ||
| name = "include_system_dirs", | ||
| flag_sets = [ | ||
| flag_set( | ||
| actions = [ | ||
| ACTION_NAMES.c_compile, | ||
| ACTION_NAMES.cpp_compile, | ||
| ACTION_NAMES.cpp_module_compile, | ||
| ACTION_NAMES.cpp_header_parsing, | ||
| ACTION_NAMES.objc_compile, | ||
| ACTION_NAMES.objcpp_compile, | ||
| "objc-executable", | ||
| "objc++-executable", | ||
| ACTION_NAMES.assemble, | ||
| ACTION_NAMES.preprocess_assemble, | ||
| ], | ||
| flag_groups = [ | ||
| flag_group( | ||
| flags = [ | ||
| "-isysroot", | ||
| "%{sdk_dir}", | ||
| "-F%{sdk_framework_dir}", | ||
| "-F%{platform_developer_framework_dir}", | ||
| "-F%{sdk_dir}/System/iOSSupport/System/Library/Frameworks", | ||
| ], | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ) | ||
| else: | ||
| include_system_dirs_feature = feature( | ||
| name = "include_system_dirs", | ||
| flag_sets = [ | ||
| flag_set( | ||
| actions = [ | ||
| ACTION_NAMES.c_compile, | ||
| ACTION_NAMES.cpp_compile, | ||
| ACTION_NAMES.cpp_module_compile, | ||
| ACTION_NAMES.cpp_header_parsing, | ||
| ACTION_NAMES.objc_compile, | ||
| ACTION_NAMES.objcpp_compile, | ||
| "objc-executable", | ||
| "objc++-executable", | ||
| ACTION_NAMES.assemble, | ||
| ACTION_NAMES.preprocess_assemble, | ||
| ], | ||
| flag_groups = [ | ||
| flag_group( | ||
| flags = [ | ||
| "-isysroot", | ||
| "%{sdk_dir}", | ||
| "-F%{sdk_framework_dir}", | ||
| "-F%{platform_developer_framework_dir}", | ||
| ], | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
The include_system_dirs_feature is defined twice, once inside the if block for catalyst and once outside. This duplication can be avoided by defining the feature once and conditionally setting its flags based on the platform. This would improve code maintainability.
include_system_dirs_feature = feature(
name = "include_system_dirs",
flag_sets = [
flag_set(
actions = [
ACTION_NAMES.c_compile,
ACTION_NAMES.cpp_compile,
ACTION_NAMES.cpp_module_compile,
ACTION_NAMES.cpp_header_parsing,
ACTION_NAMES.objc_compile,
ACTION_NAMES.objcpp_compile,
"objc-executable",
"objc++-executable",
ACTION_NAMES.assemble,
ACTION_NAMES.preprocess_assemble,
],
flag_groups = [
flag_group(
flags = select({
"//conditions:default": [
"-isysroot",
"%{sdk_dir}",
"-F%{sdk_framework_dir}",
"-F%{platform_developer_framework_dir}",
],
"ctx.attr.cpu == \"catalyst_x86_64\" or ctx.attr.cpu == \"catalyst_arm64\"": [
"-isysroot",
"%{sdk_dir}",
"-F%{sdk_framework_dir}",
"-F%{platform_developer_framework_dir}",
"-F%{sdk_dir}/System/iOSSupport/System/Library/Frameworks",
],
})
),
],
),
]
)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/osx/crosstool/cc_toolchain_config.bzl (2)
689-702: CPU checks are exploding – consider a maintainable helperMultiple
if (…) or … or …chains received two more Catalyst literals.
This pattern is already fragile and error-prone; every new Apple CPU now requires editing ~15 separate conditionals.Consider extracting reusable predicates, e.g.:
IOS_SIM_CPUS = {"ios_i386","ios_x86_64","ios_sim_arm64", ...} CATALYST_CPUS = {"catalyst_x86_64","catalyst_arm64"} WATCHOS_SIM_CPUS = {...} def _is_ios_like(cpu): return cpu in IOS_SIM_CPUS | CATALYST_CPUS | {...}and then replace huge chains with:
if _is_ios_like(ctx.attr.cpu): ...This will slash duplication, reduce merge conflicts, and eliminate the risk of accidentally omitting a new CPU in one location.
1297-1323: Duplicate framework path flags – keep one source of truth
include_system_dirs_featurehard-codes
-F%{sdk_dir}/System/iOSSupport/System/Library/Frameworks,
whileios_support_link_flags_feature(after the fix above) supplies the same directory via-iframeworkwithsysroot.Carrying both flags is harmless but redundant and makes future path tweaks harder. I’d suggest:
- Keep only the
-iframeworkwithsysrootvariation (preferred by Apple docs), or- Store the relative path in a single variable and reuse it in both features.
This small cleanup will remove another potential divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java(1 hunks)src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformApi.java(1 hunks)src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformTypeApi.java(1 hunks)tools/osx/crosstool/BUILD.toolchains(1 hunks)tools/osx/crosstool/cc_toolchain_config.bzl(9 hunks)tools/osx/crosstool/osx_archs.bzl(1 hunks)
🔇 Additional comments (6)
src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformApi.java (1)
36-36: Documentation updated correctly for Catalyst platform.The addition of
apple_common.platform.catalystto the documentation properly reflects the new platform support being introduced. This change aligns with the broader effort to add Mac Catalyst support.src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java (1)
58-58: ARM64 architecture added to Catalyst target CPUs.Adding
catalyst_arm64alongside the existingcatalyst_x86_64ensures that Mac Catalyst apps can be built for both Intel and Apple Silicon machines. This is consistent with other platform definitions in the file and essential for complete Mac Catalyst support.tools/osx/crosstool/osx_archs.bzl (1)
35-36: Catalyst architectures added to OSX_TOOLS_ARCHS.The addition of
catalyst_x86_64andcatalyst_arm64to the architecture list ensures the crosstool system recognizes these architectures for building. This is a necessary companion change to the updates in ApplePlatform.java.src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformTypeApi.java (1)
33-33: Documentation updated correctly for Catalyst platform type.The addition of
apple_common.platform_type.catalystto the documentation properly reflects the new platform type. This complements the platform instance documentation update in ApplePlatformApi.java.tools/osx/crosstool/BUILD.toolchains (1)
26-33: Catalyst constraints added – double-check arch list propagationThe new
catalyst_arm64andcatalyst_x86_64entries correctly pair the macOS OS constraint with the appropriate CPU constraints – nice work 👍.One thing to verify:
OSX_TOOLS_ARCHS(loaded fromosx_archs.bzl) must now contain these two keys; otherwise the comprehension at lines 109-111 will never emit the corresponding toolchains.
Please confirm thatosx_archs.bzlwas updated in the same PR (or a dependent PR) and thatbazel queryshows the Catalyst toolchains.tools/osx/crosstool/cc_toolchain_config.bzl (1)
100-104: Correct target triple for Catalyst 👍The macabi triples
x86_64-apple-ios{ver}-macabiandarm64-apple-ios{ver}-macabiare spot-on and match Apple’s documented form.
| flag_group( | ||
| flags = [ | ||
| "-iframeworkwithsysroot", | ||
| "System/iOS/System/Library/Frameworks", | ||
| "-L%{sdk_dir}/System/iOSSupport/usr/lib", | ||
| ], |
There was a problem hiding this comment.
Framework search path typo breaks linkage
ios_support_link_flags_feature points the framework flag to
System/iOS/System/Library/Frameworks
but for macOS Catalyst SDKs the directory is
System/iOSSupport/System/Library/Frameworks (note “iOSSupport”).
If left unchanged, -iframeworkwithsysroot will silently ignore the path and the link step can fail to resolve UIKit, etc.
- "-iframeworkwithsysroot",
- "System/iOS/System/Library/Frameworks",
+ "-iframeworkwithsysroot",
+ "System/iOSSupport/System/Library/Frameworks",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| flag_group( | |
| flags = [ | |
| "-iframeworkwithsysroot", | |
| "System/iOS/System/Library/Frameworks", | |
| "-L%{sdk_dir}/System/iOSSupport/usr/lib", | |
| ], | |
| flag_group( | |
| flags = [ | |
| "-iframeworkwithsysroot", | |
| "System/iOSSupport/System/Library/Frameworks", | |
| "-L%{sdk_dir}/System/iOSSupport/usr/lib", | |
| ], |
This is the first of a few PRs to add Mac Catalyst support for Bazel. I have an app that can be successfully built with a few manual patches to the bundle (proper actool usage, different bundle Info.plist, codesign, and notarization). But overall, it can be successful built with an app that passes macOS AppStore review / automatic checks.
This is WIP, as I need to migrate a few manual input flags into crosstool section, and need advices from Bazel maintainers.
Summary by CodeRabbit
New Features
Documentation
Chores