Skip to content

[CMake] Simplify code coverage CMake configuration#4793

Merged
llvm-beanz merged 2 commits intomicrosoft:mainfrom
llvm-beanz:cbieneman/coverage-workflow
Nov 17, 2022
Merged

[CMake] Simplify code coverage CMake configuration#4793
llvm-beanz merged 2 commits intomicrosoft:mainfrom
llvm-beanz:cbieneman/coverage-workflow

Conversation

@llvm-beanz
Copy link
Copy Markdown
Collaborator

@llvm-beanz llvm-beanz commented Nov 16, 2022

This just adds an option processed by the CMake cache file to configure code coverage for DXC. With this patch you can generate a coverage report locally if you build with Clang using the following commnads:

cmake -G Ninja -DDXC_COVERAGE=On <other options> -C <path to dxc>/cmake/caches/PredefinedParams.cmake <path to dxc>
ninja
ctest
ninja generate-coverage-report

This removes the need to separately specify LLVM_BUILD_INSTRUMENTED_COVERAGE, LLVM_PROFILE_DATA_DIR, and LLVM_CODE_COVERAGE_TARGETS manually as well as increases the robustness of detecting llvm-cov and llvm-profdata on Linux.

This just addes an option processed by the CMake cache file to configure
code coverage for DXC. With this patch you can generate a coverage
report locally if you build with Clang using the following commnads:

cmake -G Ninja -DDXC_COVERAGE=On <other options> -C
<path to dxc>/cmake/caches/PredefinedParams.cmake <path to dxc>
ninja
ctest
ninja generate-coverage-report
@llvm-beanz llvm-beanz requested review from hekota and pow2clk November 16, 2022 21:40
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@dmpots dmpots left a comment

Choose a reason for hiding this comment

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

Might be worth a short comment in the commit message about how its better than before (e.g. what did you have to do before to get code coverage).

@@ -1,3 +1,9 @@
if (DXC_COVERAGE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this have the HLSL change marker comment too?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That file is HLSL-only.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I updated the file with a comment at the top making clear it is an HLSL-only file and describing how CMake cache scripts are used.

CMake cache scripts aren't super common, but are very useful. Adding a
comment to explain how they work is probably generally valuable.
@AppVeyorBot
Copy link
Copy Markdown

@llvm-beanz llvm-beanz merged commit 55be211 into microsoft:main Nov 17, 2022
@llvm-beanz llvm-beanz deleted the cbieneman/coverage-workflow branch November 21, 2022 18:21
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