[Runtimes][CMake] Add Distributed to Supplemental build #81529
Conversation
edymtt
force-pushed
the
edymtt/add-distributed-to-new-build-system-inert
branch
from
daaa868 to
440ff7b
Compare
|
@swift-ci please smoke test |
edymtt
requested review from
etcwilde,
compnerd and
justice-adams-apple
as code owners
| CACHE FILEPATH "Location for private build system extension") | ||
|
|
||
| include(CxxStandard) | ||
| include(SwiftCallingConventions) |
There was a problem hiding this comment.
Why is distributed special and the only one to perform these checks? I think that we should replicate them across all the builds if we want to perform these.
There was a problem hiding this comment.
Of the Supplemental libraries we are converting, only Distributed and Observation use Swift calling conventions -- since in this rewrite we are trying to apply flags/features only to the libraries that only strictly need those, I thought it was sound not require to apply those to Synchronization and Differentiation.
There was a problem hiding this comment.
Core definitely uses the swift calling conventions as does the Concurrency runtime.
There was a problem hiding this comment.
I agree, my analysis was focused on the Supplemental libraries alone -- Core and Concurrency are running these checks already with CompilerSettings.cmake
|
|
||
| configure_file("CMakeConfig.h.in" | ||
| "${PROJECT_BINARY_DIR}/include/swift/Runtime/CMakeConfig.h" | ||
| ESCAPE_QUOTES @ONLY) |
There was a problem hiding this comment.
This seems sketchy. We shouldn't be re-generating the configuration for the runtime build, but should distribute it or pull it from the build tree.
There was a problem hiding this comment.
Let me see what I can do -- possibly a find module may help here, need to think about the distribution scenario.
There was a problem hiding this comment.
If we install/export this from Core, find_package(SwiftCore) should be able to wire this up.
There was a problem hiding this comment.
Once the Core and supplemental libraries are built, the CMakeConfig.h file aren't used by anything so it shouldn't be necessary so installing it isn't great.
You're right that it's sketchy if the projects get out of sync. I do think that the SwiftCoreConfig.cmake should export the appropriate settings and that the supplemental libraries should import them to ensure that the values are kept in sync.
| if($CACHE{CMAKE_CXX_STANDARD} AND $CACHE{CMAKE_CXX_STANDARD} LESS ${${PROJECT_NAME}_MIN_CXX_STANDARD}) | ||
| message(WARNING "Resetting cache value for CMAKE_CXX_STANDARD to ${${PROJECT_NAME}_MIN_CXX_STANDARD}") | ||
| unset(CMAKE_CXX_STANDARD CACHE) | ||
| endif() |
There was a problem hiding this comment.
I'm not sure that this is valuable to keep. Just using the standard CMAKE_CXX_STANDARD and CMAKE_CXX_STANDARD_REQUIRED should ensure that the compiler is able to support this standard. We also want all the projects to build at the same standard I think, so having a project specific standard is less than ideal.
So far, we haven't set the C++ standard on the other projects, and we should do that uniformly.
There was a problem hiding this comment.
I see, I copied and pasted without thinking about this too much -- also this is something we may pull from the configuration of the core libraries.
Differentiation and Synchronization currently do not contain any C++ code -- I think it makes more sense to not set the C++ standard for them until we add C++ code (which I believe we should not be able to forget, since we need also to change the linkage language)
| swiftCore | ||
| swift_Concurrency) | ||
| # swiftDarwin/Libc/Platform | ||
| # builtin_float |
There was a problem hiding this comment.
_Builtin_float is part of the overlays, we should introduce a find_project(SwiftOverlay) for this.
There was a problem hiding this comment.
Thanks for suggesting this should be more generic -- I was thinking of focusing specifically on _Builtin_float, but that may be too granular.
Leave it inert for now, as this would relies on a few find modules that are not available at the moment. Addresses rdar://151389005
edymtt
force-pushed
the
edymtt/add-distributed-to-new-build-system-inert
branch
from
440ff7b to
fa2add6
Compare
|
(This is a rebase to account for merge conflicts and changes in #81558, no review feedback has been incorporated yet) |
Also add a couple modules to use Swift calling conventions and set the correct C++ standard.
Do not enable this yet, as this would relies on a few find modules that
are not available at the moment.
Addresses rdar://151389005