-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[cmake] add FindSwiftPlatformOverlay.cmake #82895
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
Conversation
Runtimes/Supplemental/cmake/modules/FindSwiftPlatformOverlay.cmake
Outdated
Show resolved
Hide resolved
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.
| #ToDo: Handle the static MUSL SDK case | |
| #TODO(swiftlang/swift#????): Handle the static MUSL SDK case |
Can you fill in an issue identifier that has the context on what exactly needs to be done here?
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.
Runtimes/Supplemental/cmake/modules/FindSwiftPlatformOverlay.cmake
Outdated
Show resolved
Hide resolved
Runtimes/Supplemental/cmake/modules/FindSwiftPlatformOverlay.cmake
Outdated
Show resolved
Hide resolved
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.
Seems like a generator expression should allow us to fold the two branches into one:
list(APPEND swiftAndroid_INCLUDE_DIR_HINTS
"${Swift_SDKROOT}/usr/lib/swift$<$<BOOL:${SwiftPlatformOverlay_USE_STATIC_LIBS}>:_static>/swift/android
...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 don't believe that find_library or find_path accept generator expressions as its HINTS since it's run well before the is instantiated.
It turns out that find_library does actually grab a pointer to the global generator from the makefile, so I learned something new today.
cmFindLibraryHelper::cmFindLibraryHelper(cmMakefile* mf,
cmFindBase const* base,
cmFindCommonDebugState* debugState)
: Makefile(mf)
, FindBase(base)
, DebugState(debugState)
{
this->GG = this->Makefile->GetGlobalGenerator();
// Collect the list of library name prefixes/suffixes to try.
std::string const& prefixes_list = get_prefixes(this->Makefile);
std::string const& suffixes_list = get_suffixes(this->Makefile);
this->Prefixes.assign(prefixes_list, cmList::EmptyElements::Yes);
this->Suffixes.assign(suffixes_list, cmList::EmptyElements::Yes);
this->RegexFromList(this->PrefixRegexStr, this->Prefixes,
cmSystemTools::DirCase::Sensitive);
this->RegexFromList(this->ICasePrefixRegexStr, this->Prefixes,
cmSystemTools::DirCase::Insensitive);
this->RegexFromList(this->SuffixRegexStr, this->Suffixes,
cmSystemTools::DirCase::Sensitive);
this->RegexFromList(this->ICaseSuffixRegexStr, this->Suffixes,
cmSystemTools::DirCase::Insensitive);
// Check whether to use OpenBSD-style library version comparisons.
this->IsOpenBSD = this->Makefile->GetState()->GetGlobalPropertyAsBool(
"FIND_LIBRARY_USE_OPENBSD_VERSIONING");
}
That's so that it can call cmGlobalGenerator::GetDirectoryContent, but I don't see it calling cmGeneratorExpression::Expand anywhere in the evaluation.
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.
What is this? The project name is SwiftOverlay:
This will need to align with the Find-module name, (SwiftPlatformOverlay). This will be consistent across all of the platforms.
We should be able to move this bit to the top of the file just like with FindSwiftCore.cmake.
if(SwiftPlatformOverlay_DIR)
if(SwiftPlatformOverlay_FIND_REQUIRED)
list(APPEND args REQUIRED)
endif()
if(SwiftPlatformOverlay_FIND_QUIETLY)
list(APPEND args QUIET)
endif()
find_package(SwiftPlatformOverlay CONFIG ${args})
return()
endif()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 renamed the package to FindSwiftOverlay and moved the check for SwiftOverlay_Dir to the top of the 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.
This is part of the platform overlay package. This should be exported by the SwiftPlatformOverlay module.
Runtimes/Supplemental/cmake/modules/FindSwiftPlatformOverlay.cmake
Outdated
Show resolved
Hide resolved
fb9bbc0 to
f902e53
Compare
f902e53 to
4a02999
Compare
|
@swift-ci smoke test |
|
@swift-ci smoke test macOS |
| endif() | ||
|
|
||
| if(SwiftOverlay_DIR) | ||
| find_package(SwiftOverlay CONFIG) |
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 think it still makes sense to propagate of the QUIET and REQUIRED keywords like find_dependency does (since find_package does not do that, at least from a quick skim of the source code)
| find_package(SwiftOverlay CONFIG) | |
| if(${CMAKE_FIND_PACKAGE_NAME}_FIND_REQUIRED) | |
| list(APPEND args REQUIRED) | |
| endif() | |
| if(${CMAKE_FIND_PACKAGE_NAME}_FIND_QUIETLY) | |
| list(APPEND args QUIET) | |
| endif() | |
| find_package(SwiftOverlay CONFIG ${args}) |
| include(FindPackageHandleStandardArgs) | ||
| include(PlatformInfo) |
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.
(Nitpick) I would suggest to move this after if(SwiftOverlay_DIR), so it matches the structure of FindSwiftCore (and we avoid loading those modules if we have a config file)
| # If the swiftGlibc_DIR_FLAG is specified, look there instead. The cmake-generated | ||
| # config file is more accurate | ||
| if(swiftGlibc_DIR) | ||
| if(swiftGlibc_FIND_REQUIRED) | ||
| list(APPEND args REQUIRED) | ||
| endif() | ||
| if(swiftGlibc_FIND_QUIETLY) | ||
| list(APPEND args QUIET) | ||
| endif() | ||
| find_package(swiftGlibc NO_MODULE ${args}) | ||
| # Look in the SDK | ||
| else() |
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 don't think this is needed -- I expect swiftGlibc to be exported as part of the Overlay build system, so the call to find_package(SwiftOverlay ...) should take care of this.
| # If the swiftGlibc_DIR_FLAG is specified, look there instead. The cmake-generated | |
| # config file is more accurate | |
| if(swiftGlibc_DIR) | |
| if(swiftGlibc_FIND_REQUIRED) | |
| list(APPEND args REQUIRED) | |
| endif() | |
| if(swiftGlibc_FIND_QUIETLY) | |
| list(APPEND args QUIET) | |
| endif() | |
| find_package(swiftGlibc NO_MODULE ${args}) | |
| # Look in the SDK | |
| else() |
(the matching endif() should be deleted as well)
| "${Swift_SDKROOT}/usr/lib/swift" | ||
| "$ENV{SDKROOT}/usr/lib/swift/${${PROJECT_NAME}_PLATFORM_SUBDIR}/${${PROJECT_NAME}_ARCH_SUBDIR}" | ||
| "$ENV{SDKROOT}/usr/lib/swift") | ||
| list(APPEND swiftCRT_NAMES swiftCRT.lib) |
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.
We need to change the name whether we are looking for dynamic or static libraries
(please double check with what we did for FindSwiftCore)
| list(APPEND swiftCRT_NAMES swiftCRT.lib) | |
| if(SwiftOverlay_USE_STATIC_LIBS) | |
| list(APPEND swiftCRT_NAMES libswiftCRT.lib) | |
| else() | |
| list(APPEND swiftCRT_NAMES swiftCRT.lib) | |
| endif() |
| "${Swift_SDKROOT}/usr/lib/swift" | ||
| "$ENV{SDKROOT}/usr/lib/swift/${${PROJECT_NAME}_PLATFORM_SUBDIR}/${${PROJECT_NAME}_ARCH_SUBDIR}" | ||
| "$ENV{SDKROOT}/usr/lib/swift") | ||
| list(APPEND swiftWinSDK_NAMES swiftWinSDK.lib) |
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.
Same feedback as swiftCRT
4a02999 to
0cb933a
Compare
|
@swift-ci smoke test macOS |
|
@swift-ci smoke test |
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.
Looks good to me -- of all my comments, the only important one before merging is about lines 144-148
| FindSwiftOverlay | ||
| ------------ | ||
|
|
||
| Find SwiftOverlay, deferring to the associated <Overlay>Config.cmake when requested. |
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.
(Can be done in a follow up PR) At the moment we can defer only to the config file for the SwiftOverlay project
| Find SwiftOverlay, deferring to the associated <Overlay>Config.cmake when requested. | |
| Find SwiftOverlay, deferring to the associated SwiftOverlayConfig.cmake when requested. |
| This affects Linux, Android, and Windows builds. | ||
| Apple builds always use the overlay provided by the SDK. | ||
|
|
||
| Result Variables |
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.
(Can be done in a follow up PR) This section refers to variables that are not considered (swiftDarwin_DIR et al) or not set by find_package and our search logic (swiftDarwin_DIR_FOUND et all)
The section would need to either be reworked, or possibly removed all together (to nudge users toward using the target)
| if(SwiftOverlay_USE_STATIC_LIBS) | ||
| list(APPEND swiftCRT_NAMES libswiftWinSDK.lib) | ||
| else() | ||
| list(APPEND swiftCRT_NAMES swiftWinSDK.lib) | ||
| endif() |
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 name of the variable does not match the overlay we are searching
| if(SwiftOverlay_USE_STATIC_LIBS) | |
| list(APPEND swiftCRT_NAMES libswiftWinSDK.lib) | |
| else() | |
| list(APPEND swiftCRT_NAMES swiftWinSDK.lib) | |
| endif() | |
| if(SwiftOverlay_USE_STATIC_LIBS) | |
| list(APPEND swiftWinSDK_NAMES libswiftWinSDK.lib) | |
| else() | |
| list(APPEND swiftWinSDK_NAMES swiftWinSDK.lib) | |
| endif() |
| " Build the Overlays for your platform and set the appropriate `<OverlayTarget>_DIR` variable to" | ||
| " the directory containing <OverlayTarget>Config.cmake\n") |
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.
(Can be done in a later PR) I believe we want to suggest the SwiftOverlay project is the place to build additional overlays
| " Build the Overlays for your platform and set the appropriate `<OverlayTarget>_DIR` variable to" | |
| " the directory containing <OverlayTarget>Config.cmake\n") | |
| " Build the Overlays for your platform and set the appropriate `SwiftOverlay_DIR` variable to" | |
| " the directory containing SwiftOverlayConfig.cmake\n") |
|
|
||
| # Setup SwiftOverlay interface library and link it against the explicit | ||
| # overlay targets | ||
| add_library(SwiftOverlay INTERFACE) |
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.
(Can be done in a follow up PR) I would use the same camel case convention we use for the other targets -- that is, start the name with a lower case letter
| add_library(SwiftOverlay INTERFACE) | |
| add_library(swiftOverlay INTERFACE) |
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.
@edymtt I' would probably tackle this in a another PR, but a bit confused, I see the target in the Overlay project is called SwiftOverlay and the FindSwiftCore defines SwiftCore so I was going off that. Might be overlooking something
0cb933a to
86821e4
Compare
|
@swift-ci smoke test |
Add FindSwiftPlatformOverlay module responsible for finding the platform overlays for the supplemental libraries