[core] Update CMake to use TARGET_SDKS (NFC) #5004
Conversation
|
@swift-ci Please clean test |
|
Build failed |
|
@shahmishal @jrose-apple Every time I have been trying to do a clean test, I have been running into the libdispatch issue that @modocache just ran into. What is the ETA for a fix for this problem? It is important that we have the ability to do clean builds to test cmake changes in a completely safe way. |
|
@modocache Btw, I am reviewing these while juggling a bunch of other things. If I don't respond in a week, please feel free to ping me. [I just want to be clear about that = )] |
|
@erg is working on it. |
|
@gottesmm No problem! Also, I'm not entirely sure the Linux failure here is a CI failure. I do touch ICU linking in the patch, so it could be my fault. Booting my Linux VM to check as we speak. :) |
|
@modocache this is failing in the libdispatch build (unless I read the log wrong). |
|
Oh, yes, you're right. I was reading some unrelated output. :) |
|
@swift-ci Please test Linux platform |
|
Build failed |
|
@swift-ci Please clean test linux platform |
|
@jrose-apple said that the libdispatch problem should be fixed. |
|
Build failed |
|
@jrose-apple @erg It looks like dispatch here is still broken. |
|
Ah, I thought we were still talking about the macOS Dispatch issues. I don't know this one. @dgrove-oss? |
The `add_swift_library` CMake function takes an optional `TARGET_SDKS` parameter. When used, only CMake targets for the specified SDKs are added. Refactor `stdlib/public/core` to use this parameter. This also eliminates logic that determines additional flags or source files to include based on `CMAKE_SYSTEM_NAME`, which makes it easier for hosts to add targets for different platforms. In addition, prevent libicu from being linked to object library public/stdlib/stubs on Linux -- attempting to link libraries to object libraries results in a CMake error.
modocache
force-pushed
the
stdlib-core-cmake-target-sdks
branch
from
4e75a5d to
c27d95d
Compare
|
I think the errors were indeed related to libicu, not necessarily libdispatch. I uploaded a new patch that should address the issues. Thanks for your patience! :) |
|
@swift-ci Please clean test |
|
@gottesmm All good! Mind if I merge? |
| SWIFT_COMPILE_FLAGS ${swift_stdlib_compile_flags} | ||
| LINK_FLAGS ${swift_core_link_flags} | ||
| PRIVATE_LINK_LIBRARIES ${swift_core_private_link_libraries} | ||
| LINK_FLAGS ${swift_runtime_swift_gnu_link_flags} "${ENV_SYSTEMROOT}/system32/psapi.dll" |
There was a problem hiding this comment.
This seems odd. Why is psapi being linked explicitly here by path?
There was a problem hiding this comment.
I just moved this code, I didn't write this path out myself.
|
@gottesmm Friendly ping! :) |
|
I took a quick look at this. Overall, the patch is hard to read since you are re-organizing code as well as performing your actual change. Is it possible for you to do the code re-organization before or after this commit. My reason for asking is that I want to be able to rubber stamp your commit as a simple change, but visually the noise is so large that I am not sure if I can do it without making a mistake/missing something. |
|
Thanks! My brain appreciates it = ). |
|
Closing this in favor of #5095. Thanks for the feedback! |
The
add_swift_libraryCMake function takes an optionalTARGET_SDKSparameter. When used, only CMake targets for the specified SDKs are added.Refactor
stdlib/public/coreto use this parameter. This also eliminates logic that determines additional flags or source files to include based onCMAKE_SYSTEM_NAME, which makes it easier for hosts to add targets for different platforms.This pull request was split out of #4972, which addresses SR-1362.