Skip to content
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

Upgrade Sonarcloud Functionality #114

Merged
merged 23 commits into from
May 16, 2022

Conversation

RReichert
Copy link
Contributor

@RReichert RReichert commented May 12, 2022

Changes

I've now introduced a new function generate_sonarcloud_project_properties which no longer requires engineers to explicitly update the sonar-project.properties file whenever they introduce a new project or testing folder. This body of work now means as long as engineers call the swift_add_executable, swift_add_test and/or swift_add_library function, sonarcloud will now know which source/test files to analyze. It will exclude all other files which are deemed non production code.

Future Work / Justification

Look we could have just kept the sonar-project.properties file system as it was, but in all honesty that lead to engineers forgetting to update the sonar cloud properties files (see here for a list of missed projects). These changes also means that we can easily now develop a system which can correctly calculate the true code coverage for our Swift products by running all unit tests from Starling down to libswiftnav and aggregating the results into a code coverage report (not currently possible but the development of this system is easier now that we have this).

Testing

@RReichert RReichert force-pushed the rodrigor/integrity-unit-tests-framework-2-extension branch 3 times, most recently from bcd0bd9 to d7b0da9 Compare May 13, 2022 06:49
@RReichert RReichert force-pushed the rodrigor/integrity-unit-tests-framework-2-extension branch from d7b0da9 to 039d8fa Compare May 13, 2022 06:50
@RReichert RReichert force-pushed the rodrigor/integrity-unit-tests-framework-2-extension branch from 225a763 to 24bf5da Compare May 13, 2022 11:38
@RReichert RReichert force-pushed the rodrigor/integrity-unit-tests-framework-2-extension branch 2 times, most recently from 745a58f to b4c3eec Compare May 13, 2022 12:16
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@RReichert RReichert changed the title Integrity unit tests framework 2 extension Upgrade Sonarcloud Functionality May 15, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -51,10 +51,10 @@ lint:
min_statement_spacing: 1
max_statement_spacing: 2
max_returns: 6
max_branches: 30 # Default target: 12
max_branches: 40 # Default target: 12
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is some cmake linting which is failing due to SwiftTarget .cmake having too many if statements in the function.

@@ -182,7 +182,7 @@ macro(swift_collate_arguments prefix name)
endmacro()

function(swift_add_target target type)
set(this_option INTERFACE OBJECT STATIC SHARED MODULE)
set(this_option INTERFACE OBJECT STATIC SHARED MODULE SKIP_COMPILE_OPTIONS)
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 flag here is introduced to allow Orion + friends to use this function. To allow us to slowly transition all their code to use these swift_add_* function. Currently they are not as pedantic about things as we are in Starling

BRIEF_DOCS "Swift target type"
FULL_DOCS "Identical use as SWIFT_TYPE except that this applies to ALL target types, including INTERFACE")

define_property(TARGET
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently not using this property at the moment.

I originally implemented (accidentally) an alternative solution to swift_list_targets (only after reviewing my code today did I realize that that function already existed) which used a GLOBAL property to accumulate out all registered Swift executables/libraries/etc (see this.) I used this target property to filter the results for specific projects, which in this case did much what ONLY_THIS_REPO did, without using the third_party folder approach. Either way, I believe this might serve some use in the future and might not be bad for reporting purposes so I kept this around.

define_property(TARGET
PROPERTY INTERFACE_SWIFT_TYPE
BRIEF_DOCS "Swift target type"
FULL_DOCS "Identical use as SWIFT_TYPE except that this applies to ALL target types, including INTERFACE")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SWIFT_TYPE wasn't including interface libraries, this is a work around solution. See https://stackoverflow.com/questions/68502038/custom-properties-for-interface-libraries

if(NOT ${type} IN_LIST x_TYPES)
continue()
endif()
endif()

if(x_SWIFT_TYPES)
get_target_property(type ${target} SWIFT_TYPE)
if (type STREQUAL "INTERFACE_LIBRARY")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

previously if the target was an interface library (ex eigen) type, cmake would complain about not being able to obtain any properties that didn't prefix with INTERFACE.

# references to relative paths. This manual step can be done easily with the
# following Linux command:
#
# sed -i -E "s|^(\s+)(${PWD}/)|\1|g" 'sonar-project.properties'
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a one liner but I think it would be good to make it available as a bash script, and have pipelines use the bash script to fix up the paths rather than writing this snippet directly into pipeline scripts.

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 functionality is already incorporated into our Sonarcloud upload script (see here). You can see here how we use it.

@RReichert RReichert requested a review from jungleraptor May 16, 2022 21:38
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

lgtm this is a really nice capability you've introduced!

@RReichert RReichert merged commit 6320bbf into master May 16, 2022
@RReichert RReichert deleted the rodrigor/integrity-unit-tests-framework-2-extension branch May 16, 2022 22:05
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.

2 participants