-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[cmake] Add support for importing MODULE_LIBRARY targets and clean th… #3406
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
[cmake] Add support for importing MODULE_LIBRARY targets and clean th… #3406
Conversation
@swift-ci Please test |
endif() | ||
|
||
# LLVM compiles module libraries as | ||
# ${MODULE_NAME}${CMAKE_SHARED_LIBRARY_SUFFIX} on both Linux and macOS. |
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.
It looks like the correct variable is CMAKE_SHARED_MODULE_SUFFIX. Is there really no less ad hoc way to do this?
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.
Actually that is incorrect here. On APPLE, CMAKE_SHARED_MODULE_SUFFIX is set to .so. I am not sure right now why that is broken in this way, but the warning will tell us if things change and we can no longer find it.
43b7a30
to
311106b
Compare
Jordan and I talked about this yesterday and came up with a different approach that is a little cleaner and allows me to remove the libclang hack. |
@swift-ci Please test |
There's a remaining bug in this pull request... line 283 needs the same fix_imported_target_locations_for_xcode -> fix_imported_targets_for_xcode applied to line 190. After that, it appears to work fine for me. |
I just checked locally on my machine. Everything passes. |
@mattgallagher I know. I have that fixed locally. Thanks though! |
…ous libraries when translating Xcode targets, just use the path already provided in LLVMConfig.cmake for the configuration that we compiled LLVM specifically for. If we compile LLVM for <CONFIG> in build-script with Xcode as our generator, LLVMExports.cmake will have specific IMPORTED_*_<CONFIG> variables set for all of our targets. These variables are what we really want to splat across all Swift Xcode build configurations. This patch rips out the old code that determined the actual location and instead just grabs the IMPORTED_*_<CONFIG> variables and splats it accordingly.
311106b
to
d004abc
Compare
I like this a lot better. LGTM, thanks Michael! |
I just updated the patch with my local changes that make everything work. |
@jrose-apple I also like it better. The key thing that won me over was removing the libclang hack. Isn't it amazing what two smart people with good intentions can conjure up? = ). |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
…ings up a little bit.