Sanitize test projects to remove unnecessary code copies, replace with build deps

Bug #1371690 reported by Allan LeSage on 2014-09-19
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Coverage Builder
Allan LeSage

Bug Description

Discussing with pitti:

<pitti> alesage: right; I mean, these test projects in c-g are a nice reference how build systems *should* be done
<pitti> alesage: and there should ideally be zero copies of boilerplate stuff

Specifically, remove libgtest-dev-supplied files, and get working with cmake-extras-supplied EnableCoverageReport.cmake. Martin any other style points to remedy for those?

Related branches

Allan LeSage (allanlesage) wrote :

Note that this results from a discussion of a test-adding MP: .

Martin Pitt (pitti) wrote :

Seems fine like that.

summary: - Sanitize test projects to remove unnecessary dependencies
+ Sanitize test projects to remove unnecessary code copies, replace with
+ build deps
Changed in coverage-builder:
status: New → Confirmed
Allan LeSage (allanlesage) wrote :

I've remembered that per Satoris' advice the cmake-extras EnableCoverageReport.cmake macro works in a different way that makes it incompatible with the virally-distributed EnableCoverageReport.cmake that we find in mir and unity8, etc.

Specifically, in the existing builds we do

$ cmake ../ -DCMAKE_BUILD_TYPE=coverage

whereas in cmake-extras we do

$ cmake -Dcoverage=1

So I won't remove that file--Martin would we then want a new test for the 'cmake-extras-EnableCoverageReport.cmake' method? Upon consideration that's an unfortunate name-collision with the virally distributed one--or maybe cmake-extras-EnableCoverageReport.cmake should support both enablement mechanisms?

Note that the new method is nowhere in production and is therefore not yet a case that coverage-builder needs to cover.

You can read Jussi's and my correspondence about the new method here: .

Meanwhile FindGtest.cmake has indeed made to the archive, will attach that branch to this bug.

Martin Pitt (pitti) wrote :

If the two kinds of EnableCoverageReport.cmake indeed differ (that's precisely the reason why it's a really bad idea to copy such code around and then modify it!), then I think the best way to merge them so that they support both configuration syntaxes. If for some reason that's not possible, I suggest renaming one, negotiating which one we want in the future, putting a big "this is deprecated, please move to XXX" stamp on the other, and over time convert projects.

Martin Pitt (pitti) wrote :

> would we then want a new test for the 'cmake-extras-EnableCoverageReport.cmake' method?

If that's the one in widespread use, then I think we do need that, yes.

Allan LeSage (allanlesage) wrote :

Having thought about it I do think it's best for the macro to support both the -DCMAKE_BUILD_TYPE and the new strategy, I've made a bug for that in cmake-extras: lp:1372535 .

Note that the 'new strategy' is not yet in production--we'll add those tests for lp:1372538 , propose to close this when we've removed FindGtest.cmake cruft.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers