Skip to content

Mac Catalyst support for bazel / rules_swift / rules_apple #24

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Mac Catalyst support for bazel / rules_swift / rules_apple #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7acfc07
WIP: Mac Catalyst support for bazel / rules_swift / rules_apple
liuliu Jan 3, 2023
bd862e3
Add ios_support_link_flag_feature.
liuliu Jan 3, 2023
5d15329
Fix minor parameter issue.
liuliu Mar 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public enum ApplePlatform implements ApplePlatformApi {
private static final ImmutableSet<String> TVOS_DEVICE_TARGET_CPUS =
ImmutableSet.of("tvos_arm64");
private static final ImmutableSet<String> CATALYST_TARGET_CPUS =
ImmutableSet.of("catalyst_x86_64");
ImmutableSet.of("catalyst_x86_64", "catalyst_arm64");
Comment on lines 57 to +58
Copy link

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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

// "darwin" is included because that's currently the default when on macOS, and
// migrating it would be a breaking change more details:
// https://github.com/bazelbuild/bazel/pull/7062
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
+ " href='apple_common.html#platform'>apple_common.platform</a> struct:<br><ul>"
+ "<li><code>apple_common.platform.ios_device</code></li>"
+ "<li><code>apple_common.platform.ios_simulator</code></li>"
+ "<li><code>apple_common.platform.catalyst</code></li>"
+ "<li><code>apple_common.platform.macos</code></li>"
+ "<li><code>apple_common.platform.tvos_device</code></li>"
+ "<li><code>apple_common.platform.tvos_simulator</code></li>"
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/starlarkbuildapi/apple/ApplePlatformTypeApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
+ " href='apple_common.html#platform_type'>apple_common.platform_type</a>:<br><ul>"
+ "<li><code>apple_common.platform_type.ios</code></li>"
+ "<li><code>apple_common.platform_type.macos</code></li>"
+ "<li><code>apple_common.platform_type.catalyst</code></li>"
+ "<li><code>apple_common.platform_type.tvos</code></li>"
+ "<li><code>apple_common.platform_type.watchos</code></li></ul><p>Likewise, the"
+ " platform type of an existing platform value can be retrieved using its"
Expand Down
8 changes: 8 additions & 0 deletions tools/osx/crosstool/BUILD.toolchains
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ OSX_TOOLS_CONSTRAINTS = {
"@platforms//os:osx",
"@platforms//cpu:x86_64",
],
"catalyst_arm64": [
"@platforms//os:osx",
"@platforms//cpu:arm64",
],
"catalyst_x86_64": [
"@platforms//os:osx",
"@platforms//cpu:x86_64",
],
"ios_arm64": [
"@platforms//os:ios",
"@platforms//cpu:arm64",
Expand Down
140 changes: 107 additions & 33 deletions tools/osx/crosstool/cc_toolchain_config.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ def _impl(ctx):
target_system_name = "x86_64-apple-tvos{}-simulator".format(target_os_version)
elif (ctx.attr.cpu == "watchos_x86_64"):
target_system_name = "x86_64-apple-watchos{}-simulator".format(target_os_version)
elif (ctx.attr.cpu == "catalyst_x86_64"):
target_system_name = "x86_64-apple-ios{}-macabi".format(target_os_version)
elif (ctx.attr.cpu == "catalyst_arm64"):
target_system_name = "arm64-apple-ios{}-macabi".format(target_os_version)
else:
fail("Unreachable")
Comment on lines 104 to 105
Copy link

Choose a reason for hiding this comment

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

Missing Platform Check

The else clause fails with "Unreachable" but new catalyst platforms were added without updating this error message. This could cause misleading error messages if an unrecognized CPU is encountered.

Standards
  • Logic-Verification-Completeness
  • Business-Rule-Validation
  • Algorithm-Correctness-Error-Handling


Expand Down Expand Up @@ -692,7 +696,9 @@ def _impl(ctx):
ctx.attr.cpu == "watchos_armv7k" or
ctx.attr.cpu == "watchos_i386" or
ctx.attr.cpu == "watchos_x86_64" or
ctx.attr.cpu == "watchos_arm64"):
ctx.attr.cpu == "watchos_arm64" or
ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64"):
apply_default_compiler_flags_feature = feature(
name = "apply_default_compiler_flags",
flag_sets = [
Expand Down Expand Up @@ -941,7 +947,9 @@ def _impl(ctx):
ctx.attr.cpu == "watchos_armv7k" or
ctx.attr.cpu == "watchos_i386" or
ctx.attr.cpu == "watchos_x86_64" or
ctx.attr.cpu == "watchos_arm64"):
ctx.attr.cpu == "watchos_arm64" or
ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64"):
contains_objc_source_feature = feature(
name = "contains_objc_source",
flag_sets = [
Expand Down Expand Up @@ -1098,6 +1106,32 @@ def _impl(ctx):
],
)

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",
Comment on lines +1122 to +1123
Copy link

Choose a reason for hiding this comment

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

Missing iOS Framework Path

Path appears incorrect - should likely be '/System/iOSSupport/System/Library/Frameworks'. Incorrect framework path will cause build failures for Catalyst targets due to missing system frameworks.

Standards
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • ISO-IEC-25010-Reliability-Maturity

],
Comment on lines +1119 to +1124
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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",
],

),
],
),
],
)
else:
ios_support_link_flags_feature = feature(
name = "ios_support_link_flags"
)
Comment on lines +1109 to +1133
Copy link

Choose a reason for hiding this comment

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

medium

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",
                        ],
                    ),
                ],
            ),
        ],
    )


no_deduplicate_feature = feature(
name = "no_deduplicate",
enabled = True,
Expand Down Expand Up @@ -1260,35 +1294,68 @@ def _impl(ctx):

coverage_feature = feature(name = "coverage")

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}",
],
),
],
),
],
)
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}",
],
),
],
),
],
)
Comment on lines +1297 to +1358
Copy link

Choose a reason for hiding this comment

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

medium

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",
                            ],
                        })
                    ),
                ],
            ),
        ]
    )

Comment on lines +1297 to +1358
Copy link

Choose a reason for hiding this comment

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

Duplicated Platform Logic

Significant code duplication in include_system_dirs_feature definition with only one flag difference. This violates DRY principle and increases maintenance burden when updating common flags across platform variants.

Standards
  • Clean-Code-DRY
  • Design-Pattern-Template-Method


input_param_flags_feature = feature(
name = "input_param_flags",
Expand Down Expand Up @@ -1542,7 +1609,9 @@ def _impl(ctx):
ctx.attr.cpu == "watchos_armv7k" or
ctx.attr.cpu == "watchos_i386" or
ctx.attr.cpu == "watchos_x86_64" or
ctx.attr.cpu == "watchos_arm64"):
ctx.attr.cpu == "watchos_arm64" or
ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64"):
apply_implicit_frameworks_feature = feature(
name = "apply_implicit_frameworks",
flag_sets = [
Expand Down Expand Up @@ -2395,6 +2464,8 @@ def _impl(ctx):
ctx.attr.cpu == "tvos_arm64" or
ctx.attr.cpu == "watchos_arm64_32" or
ctx.attr.cpu == "watchos_armv7k" or
ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64" or
ctx.attr.cpu == "darwin_x86_64" or
ctx.attr.cpu == "darwin_arm64" or
ctx.attr.cpu == "darwin_arm64e"):
Expand Down Expand Up @@ -2652,7 +2723,9 @@ def _impl(ctx):
ctx.attr.cpu == "watchos_armv7k" or
ctx.attr.cpu == "watchos_i386" or
ctx.attr.cpu == "watchos_x86_64" or
ctx.attr.cpu == "watchos_arm64"):
ctx.attr.cpu == "watchos_arm64" or
ctx.attr.cpu == "catalyst_x86_64" or
ctx.attr.cpu == "catalyst_arm64"):
features = [
fastbuild_feature,
no_legacy_features_feature,
Expand Down Expand Up @@ -2711,6 +2784,7 @@ def _impl(ctx):
relative_ast_path_feature,
user_link_flags_feature,
default_link_flags_feature,
ios_support_link_flags_feature,
no_deduplicate_feature,
dead_strip_feature,
cpp_linker_flags_feature,
Expand Down
2 changes: 2 additions & 0 deletions tools/osx/crosstool/osx_archs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ OSX_TOOLS_ARCHS = [
"ios_armv7",
"ios_arm64",
"ios_arm64e",
"catalyst_x86_64",
"catalyst_arm64",
"watchos_armv7k",
"watchos_arm64_32",
"tvos_arm64",
Expand Down
Loading