Skip to content

[CMake] Add option to EXCLUDE_FROM_ALL #2705

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[CMake] Add option to EXCLUDE_FROM_ALL #2705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,9 @@ add_compile_definitions(
$<$<COMPILE_LANGUAGE:Swift>:SWIFT_SYNTAX_BUILD_USING_CMAKE>
)

if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the option defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FetchContent clients
https://github.com/swiftlang/swift/pull/74733/files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL})
if(SWIFTSYNTAX_EXCLUDE_FROM_ALL)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
if(${SWIFTSYNTAX_EXCLUDE_FROM_ALL})
set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL ${SWIFTSYNTAX_EXCLUDE_FROM_ALL})

Copy link
Member Author

@rintaro rintaro Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})

set_directory_properties(PROPERTIES EXCLUDE_FROM_ALL YES)
endif()

add_subdirectory(Sources)
add_subdirectory(cmake/modules)
1 change: 0 additions & 1 deletion Sources/SwiftCompilerPlugin/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
# See http://swift.org/CONTRIBUTORS.txt for Swift project authors

add_swift_syntax_library(SwiftCompilerPlugin
EXCLUDE_FROM_ALL
CompilerPlugin.swift
)

Expand Down