Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

build: migrate CoverageReport to cmake-extras & fix its usage #358

Open
wants to merge 2 commits into
base: xenial
Choose a base branch
from

Conversation

peat-psuwit
Copy link
Contributor

@peat-psuwit peat-psuwit commented Jan 22, 2021

Uses CoverageReport from cmake-extras and moves its usage to the correct place. This should allow the coverage-reporting build to be built. At least in theory; I tested only the configure phase and didn't actually compile or generate the actual report out of it.

@peat-psuwit peat-psuwit marked this pull request as ready for review January 22, 2021 16:02
Copy link
Member

@dobey dobey left a comment

Choose a reason for hiding this comment

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

Please also remove the coverage related files from the cmake/modules/ directory.

@peat-psuwit
Copy link
Contributor Author

Oh, Unity8 includes its own copy of EnableCoverageReport.cmake? That's why I didn't see the deprecation message...

So, this turns out to be migrating from the vendored copy to the copy in cmake-extras. And look like the copy in this package has not been updated since the project was first committed, so I'm not sure if the usage is still compatible.

Given that the motivation of this PR is to make it possible to bring the newest cmake-extras into xenial so that mir-android-platform can use its GTest versioning, and my ultimate decision to not use that facility in the end, I'm not sure if it's worth it to check that.

@dobey
Copy link
Member

dobey commented Jan 25, 2021

Yes, there's a local copy left over from when various projects simply included their own copies of things, and we didn't have cmake-extras package yet. Everything else still works the same, so it's safe to remove the vendored files for it, and simply depend on cmake-extras for this.

This allows this package to receive updates to CoverageReport in the
future.

Also B-D on new enough cmake-extras (which is ancient at this point, but
just in case).
Both ${SHELL_APP} variable and the target with that name is defined
later in the CMakeLists.txt, so the coverage enabling code has to move
down.
@peat-psuwit peat-psuwit force-pushed the xenial_-_fix-cmake-extras-deprecation branch from f30b293 to 43b3018 Compare January 26, 2021 09:55
@peat-psuwit peat-psuwit changed the title Fix deprecated cmake-extra usage build: migrate CoverageReport to cmake-extras & fix its usage Jan 26, 2021
set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} --coverage" )
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} --coverage" )
ENABLE_COVERAGE_REPORT(TARGETS ${SHELL_APP} FILTER /usr/include ${CMAKE_SOURCE_DIR}/tests/* ${CMAKE_BINARY_DIR}/*)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Actually, only the ENABLE_COVERAGE_REPORT call needs to remain. The cmake-extras module will automatically add the --coverage arguments. So we can get rid of the extra stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like ENABLE_COVERAGE_REPORT also has the TESTS parameter. Am I supposed to put anything?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. It should list the tests targets. I guess we should refactor it a lot more to bubble up all the targets in a couple variables, from all the subdirs.

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

Successfully merging this pull request may close these issues.

2 participants