[CMake] Add option to EXCLUDE_FROM_ALL #2705
Conversation
SWIFTSYNTAX_EXCLUDE_FROM_ALL is a CMake option to enable EXCLUDE_FROM_ALL on the entire directory.
|
swiftlang/swift#74733 |
|
swiftlang/swift#74733 |
| @@ -82,5 +82,9 @@ add_compile_definitions( | |||
| $<$<COMPILE_LANGUAGE:Swift>:SWIFT_SYNTAX_BUILD_USING_CMAKE> | |||
| ) | |||
|
|
|||
| if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL}) | |||
There was a problem hiding this comment.
FetchContent clients
https://github.com/swiftlang/swift/pull/74733/files
There was a problem hiding this comment.
| if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL}) | |
| if(SWIFTSYNTAX_EXCLUDE_FROM_ALL) |
There was a problem hiding this comment.
set(...) doesn't really define the option, the option syntax is option(...) (https://cmake.org/cmake/help/latest/command/option.html). Or am I misunderstanding and this is not meant to be a user configurable option?
There was a problem hiding this comment.
The term "option" might not be correct. But at this point, this SWIFTSYNTAX_EXCLUDE_FROM_ALL setting is only usable when this swift-syntax project is a sub project, like by FetchContent. We can't really use option for this because swift project FetchContent this twice and we might need to differentiate the settings for each inclusion.
There was a problem hiding this comment.
option is the right thing for SWIFTSYNTAX_EXCLUDE_FROM_ALL, but getting the right defaults may be a little interesting. Since it seems like this should be set when FetchContent'ing, we can check whether the CMAKE_SOURCE_DIR is equal to SwiftSyntax_SOURCE_DIR (after the SwiftSyntax project invocation). If it is the same, we're building SwiftSyntax directly, and if they are different, we've either been built through someone copying the sources and using add_subdirectory or through FetchContent.
project(SwiftSyntax LANGUAGES C Swift)
# ...
if(NOT ${CMAKE_SOURCE_DIR} STREQUAL ${SwiftSyntax_SOURCE_DIR})
set(default_SwiftSyntax_EXCLUDE_FROM_ALL)
endif()
option(SwiftSyntax_EXCLUDE_FROM_ALL "Exclude SwiftSyntax targets from the all target" ${default_SwiftSyntax_EXCLUDE_FROM_ALL})At the very least, I don't think we need the if at all. The variable is already a boolean and if it's true, we can just pass it through:
| if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL}) | |
| set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL ${SWIFTSYNTAX_EXCLUDE_FROM_ALL}) |
There was a problem hiding this comment.
if(NOT ${CMAKE_SOURCE_DIR} STREQUAL ${SwiftSyntax_SOURCE_DIR}) set(default_SwiftSyntax_EXCLUDE_FROM_ALL) endif() option(SwiftSyntax_EXCLUDE_FROM_ALL "Exclude SwiftSyntax targets from the all target" ${default_SwiftSyntax_EXCLUDE_FROM_ALL})
Not sure I understand this. Can this be just like:
if(NOT ${CMAKE_SOURCE_DIR} STREQUAL ${SwiftSyntax_SOURCE_DIR})
option(SWIFTSYNTAX_EXCLUDE_FROM_ALL "Exclude SwiftSyntax targets from the all target")
endif()
set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL ${SWIFTSYNTAX_EXCLUDE_FROM_ALL})Also is ${CMAKE_SOURCE_DIR} STREQUAL ${SwiftSyntax_SOURCE_DIR} different from PROJECT_IS_TOP_LEVEL?
There was a problem hiding this comment.
Huh, I didn't know about PROJECT_IS_TOP_LEVEL. It looks like they are the same and are both available, and PROJECT_IS_TOP_LEVEL seems clearer, so go with that.
The default_ pattern is to configure a default without having that conflict with the variable declared in the option when CMP0077 is set. It's at least somewhat common in LLVM for situations where there is logic involved in determining the default. e.g https://github.com/llvm/llvm-project/blob/1ed84a862f9ce3c60251968f23a5405f06458975/llvm/CMakeLists.txt#L845-L861
I had thought that you were trying to hide the targets from the depender project's all target so if they just ran ninja, they'd only build the SwiftSyntax targets that their targets depend on, which seems quite reasonable to me (the compiler being one of the dependers, but seems generally useful for anyone not working on SwiftSyntax). If we can go with a one-size default, I'm fine with that too, but in either case, the option should be at the top-level so that it's visible to tooling.
Something like this seems reasonable to me
if(NOT ${CMAKE_SOURCE_DIR} STREQUAL ${SwiftSyntax_SOURCE_DIR})
set(default_SwiftSyntax_EXCLUDE_FROM_ALL YES)
endif()
option(SwiftSyntax_EXCLUDE_FROM_ALL "Exclude SwiftSyntax targets from the all target" ${default_SwiftSyntax_EXCLUDE_FROM_ALL})
set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL ${SwiftSyntax_EXCLUDE_FROM_ALL})
|
swiftlang/swift#74733 |
|
Closing because parent project can do this without changing the child project: |
SWIFTSYNTAX_EXCLUDE_FROM_ALLis a CMake option to enable EXCLUDE_FROM_ALL on the entire directory.