-
Notifications
You must be signed in to change notification settings - Fork 85
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
End-to-end support of SDK clang and swift modules #901
Conversation
…dules A sdk_clang_module may depends on other sdk_clang_modules. The previous implementation returns a _SwiftInteropInfo which doesn't support deps. In this PR, we rewrite the rule to generate a precompiled c module and return a SwiftInfo so that the deps will be correctly handled.
Xcode has already included a .swiftmodule file for each SDK swift module. This rule is to copy a xcode .swiftmodule into Bazel, and then use swift_import to import the .swfitmodule file.
Updates the xcode_sdk_frameworks repository rule to correctly configure the SDK swift modules. Updates the hello world app to use the SDK frameworks.
This example has a swift_library and a swift_binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a bunch of questions and no logic altering suggestions
@@ -5,7 +5,7 @@ common --enable_bzlmod | |||
build --spawn_strategy=standalone | |||
|
|||
# Setup Xcode configuration. | |||
build --xcode_version_config=//:host_xcodes | |||
build --xcode_version=15.4.0.15F31d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe override this just for build:explicit_modules
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also i still cannot build as is with xcode 16, i wonder if we just add this to line 418 of rules/xcode_sdk_frameworks.bzl
to add TODO that we for now just support xcode 15.4
if not xcode_version_name.startswith("version15_4"):
continue
) | ||
|
||
swift_binary( | ||
name = "hello_world", | ||
srcs = ["main.swift"], | ||
deps = [ | ||
":shims", | ||
":swift_concurrency_shims", | ||
"@xcode_sdk_frameworks//version15_4_0_15F31d/MacOSX:Foundation_swift", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to have this as a separate var that can be specified next to build:explicit_module --xcode_version=xxx
? this way we at least control the xcode version EMB points to easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a ticket to make it automatically select the right xcode version
@@ -1 +1,3 @@ | |||
import Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
modules = [ | ||
create_module( | ||
name = ctx.attr.module_name, | ||
clang = create_clang_module( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just surface them up to be on the same level as precompiled_module at line 41? that way it's clear to see that the chain is: precompile_clang_module -> create_clang_module -> create_module -> create_swift_info
), | ||
}, | ||
implementation = _sdk_clang_module_impl, | ||
fragments = ["cpp"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having this is to access what's listed here? https://bazel.build/rules/lib/fragments/cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's required to build the explicit pcm
CcInfo(compilation_context = cc_common.create_compilation_context()), | ||
# Required to add sdk_clang_module targets to the deps of swift_module_alias. | ||
# TODO(cshi): create the SwiftInfo correctly | ||
create_swift_info(), | ||
] | ||
|
||
sdk_clang_module = rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i notice that the refence to sdk_clang_module is removed in the example... that is because those are code gen by the repo rules right?
"swiftmodule_path": attr.string( | ||
doc = """\ | ||
The absolute path to the SDK swiftmodule, e.g., | ||
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/iphoneos/prebuilt-modules/17.5/UIKit.swiftmodule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not do substitution? referring to BAZEL_XCODE_SDKROOT for the module_map attr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run_shell doesn't do that. We will probably need to use apple_common
@@ -231,13 +244,15 @@ def _create_build_file_for_sdk( | |||
for i in range(module_count): | |||
name_idx = 2 * i | |||
details_idx = 2 * i + 1 | |||
targets.append(_target_for_module( | |||
target = _target_for_module( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_target_for_module
might return None
? is that even allowed? or this rules is still in construction since some code is commented out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The graph doesn't have None, but we need to skip the module bazel_xcode_imports
which is created by the rule to generate the sdk dep graph.
What
Test
bazel run --config=explicit_modules example/explicit_module:hello_world