-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix Generated sonar-project.properties Configuration #120
Conversation
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.
Other than the slight refactor, I see no issue. Would be nice to see a PR that uses this cmake changes to see that this actually solves the issue, but I'm okay without it as well.
I'm still shocked that this was what was required to make the header files visible. To me its completely illogical that that is how sonarcloud expects things, there is no distinction between project code and test code.
Sonarcloud.cmake
Outdated
list(APPEND source_targets ${test_targets}) | ||
|
||
_extract_sonarcloud_project_files(source_source_files source_include_directories ${source_targets}) | ||
_extract_sonarcloud_project_files(test_source_files test_include_directories ${test_targets}) | ||
|
||
set(sonarcloud_project_properties_content "sonar.sourceEncoding=UTF-8\n") | ||
|
||
set(source_files ${source_source_files} ${source_include_directories}) | ||
list(JOIN source_files ",${_sonarcloud_newline}" sonar_sources) | ||
string(APPEND sonarcloud_project_properties_content "sonar.inclusions=${_sonarcloud_newline}${sonar_sources}\n") | ||
|
||
set(test_files ${test_source_files}) | ||
list(JOIN test_files ",${_sonarcloud_newline}" sonar_tests) | ||
string(APPEND sonarcloud_project_properties_content "sonar.tests.inclusions=${_sonarcloud_newline}${sonar_tests}\n") | ||
string(APPEND sonarcloud_project_properties_content "sonar.sources=${_sonarcloud_newline}${sonar_sources}\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.
Would you be able to refactor this to:
_extract_sonarcloud_project_files(source_source_files source_include_directories ${source_targets})
_extract_sonarcloud_project_files(test_source_files test_include_directories ${test_targets})
set(sonarcloud_project_properties_content "sonar.sourceEncoding=UTF-8\n")
set(source_files ${source_source_files} ${source_include_directories} ${test_source_files})
list(JOIN source_files ",${_sonarcloud_newline}" sonar_sources)
string(APPEND sonarcloud_project_properties_content "sonar.sources=${_sonarcloud_newline}${sonar_sources}\n")
I believe this keeps the same logic, but it just makes it clearer that project source files are source_source_files
and test files are actually test_source_files
and for the purposes of sonarcloud's implementation we combine them under sonar.sources
.
Kudos, SonarCloud Quality Gate passed! |
This reverts commit 3550197.
Fixes the emitted sonar.properties file so that header and test files are picked up by Sonarcloud analysis. According to the docs `sonar.tests` is ignored for C/C++ code, so test paths should be treated the same as source paths. Also switches `sonar.inclusions` with `sonar.sources` to fix the include paths being filtered out.
Fixes test and header files not being analyzed by sonarcloud.
From the sonarcloud documentation:
Since we were putting all of our test files under
sonar.test.inclusions
, they were being ignored by the scanner.This changes the
sonar.inclusions
property tosonar.sources
, and appends the test paths to it instead ofsonar.tests
. With these changes, header and test files are properly analyzed by the sonar-scanner.