Skip to content

[CMake] Merge Clang's features file with Swift's #41547

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

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

bnbarham
Copy link
Contributor

Clang has a new redirecting-with property in the VFS overlay files.
While this could be added to Swift's features as well, it is generally
the case that features provided by Clang can also be useful to know for
clients of Swift. Merge the features from Clang into Swift's features
file with the "clang-" prefix to differentiate them.

@bnbarham bnbarham requested review from akyrtzi and gottesmm February 24, 2022 20:14
@@ -6,16 +6,18 @@ add_dependencies(swiftOption
target_link_libraries(swiftOption PRIVATE
swiftBasic)

set(features_file_src "${CMAKE_CURRENT_SOURCE_DIR}/features.json")
set(features_file_swift_src "${CMAKE_CURRENT_SOURCE_DIR}/features.json")
set(features_file_clang_src "${LLVM_MAIN_SRC_DIR}/../clang/tools/driver/features.json")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels very wrong to me, if there's a better way to do this please let me know 🙇

@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham force-pushed the merge-clang-features branch from 9f8fb78 to 79e8116 Compare February 25, 2022 18:11
@bnbarham
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@akyrtzi akyrtzi left a comment

Choose a reason for hiding this comment

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

Just one change, otherwise LGTM!

set(features_file_dest "${CMAKE_BINARY_DIR}/share/swift/features.json")

add_custom_command(
OUTPUT
${features_file_dest}
COMMAND
${CMAKE_COMMAND} -E copy ${features_file_src} ${features_file_dest}
${SWIFT_SOURCE_DIR}/utils/merge-features.py -f ${features_file_swift_src} -p \"\" -f ${features_file_clang_src} -p clang- ${features_file_dest}
DEPENDS
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add a dependency on the merge-features.py script itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice catch, thanks!

Clang has a new `redirecting-with` property in the VFS overlay files.
While this could be added to Swift's features as well, it is generally
the case that features provided by Clang can also be useful to know for
clients of Swift. Merge the features from Clang into Swift's features
file with the "clang-" prefix to differentiate them.
@bnbarham bnbarham force-pushed the merge-clang-features branch from 79e8116 to 914b984 Compare February 25, 2022 19:55
@bnbarham
Copy link
Contributor Author

@swift-ci please test

@bnbarham bnbarham merged commit c059506 into swiftlang:main Feb 26, 2022
@bnbarham bnbarham deleted the merge-clang-features branch February 26, 2022 00:25
@compnerd
Copy link
Member

This seems to cause problems on some build configurations. The installation may attempt to install the target multiple files and using the feature merger rather than the previous ${CMAKE_COMMAND} -E copy will fail if the file already exists. See: https://dev.azure.com/compnerd/3133d6ab-80a8-4996-ac4f-03df25cd3224/_apis/build/builds/57130/logs/53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants