Bug 249024 - cmake.mk: Add USES=cmake:testing to facilitate defining test targets for cmake-based ports
Summary: cmake.mk: Add USES=cmake:testing to facilitate defining test targets for cmak...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-kde (Team)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-31 01:26 UTC by Yuri Victorovich
Modified: 2021-03-30 12:13 UTC (History)
3 users (show)

See Also:


Attachments
patch (1.87 KB, patch)
2020-08-31 01:26 UTC, Yuri Victorovich
no flags Details | Diff
patch (1.87 KB, patch)
2020-08-31 01:27 UTC, Yuri Victorovich
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer 2020-08-31 01:26:01 UTC
Created attachment 217665 [details]
patch

Many cmake-based projects have ctate-based tests with the BUILD_TESTING cmake variable being defined in the documentation as a standard way to enable tests: https://cmake.org/cmake/help/v3.7/module/CTest.html

The suggested feature USES=cmake:testing facilitates defining test targets for such ports.
Comment 1 Yuri Victorovich freebsd_committer 2020-08-31 01:27:17 UTC
Created attachment 217666 [details]
patch
Comment 2 Tobias C. Berner freebsd_committer 2020-08-31 04:25:49 UTC
Moin moin 

I like the idea in principal, I myself looked at adding these targets a few years ago, and lbartoletti@ had the same idea a few weeks back.

However...

The reason kde@ never added those targets is, that up to a few years ago, KDE had unit tests which assumed you are *not* running the build as root (which most port builds are). 
They did funny things in the spirit of "rm -r /etc" as a "failing test case", which of course when running the `make test` as root, would pretty much ruin your day. And of course, took quite a lot of time to figure out that a test-case was doing it.

So, while I technically think this new target is a good idea, with the memory of a broken system, I kind of does do not want this, out of fear that other upstreams are doing similar assumptions about the rights of a user running the tests.


mfg Tobias
Comment 3 Yuri Victorovich freebsd_committer 2020-08-31 04:43:20 UTC
(In reply to Tobias C. Berner from comment #2)

Tobias,

If the project is broken in the way you described, this would cause the same damage when these commands are added into do-test manually. USES=cmake:testing is just a shortcut preventing copying-and-pasting the same lines into Makefiles verbatim over and over again. This doesn't cause or increase any risks.

The real solution is to ban building ports as root by the framework, and require users to define ALLOW_BUILDING_AS_ROOT=yes to waive this ban, IMO.


Yuri
Comment 4 Tobias C. Berner freebsd_committer 2020-08-31 04:46:32 UTC
(In reply to Yuri Victorovich from comment #3)

Yes, but having the porters add the test target manually means they risked their system first.

If the framework automatically adds the test target, there is no way of knowing that it won't simply trash the system of a user, as they were the first to run `make test` in that port.

So, by not providing it, we kind of force the porters to make sure the tests are sane.


But as said, I would prefer to have it in cmake.mk, it's just I really feel bad about it.


mfg Tobias
Comment 5 Adriaan de Groot freebsd_committer 2020-08-31 13:06:23 UTC
This looks mostly sensible to me:
- it only adds testing and BUILD_TESTING via the framework when a port explicitly mentions `USES=cmake:testing`.
- it looks awfully over-engineered with possibilities for ON, OFF, different targets, ... but as long as that complexity is hidden and documented in the framework and the basic interface (`USES=cmake:testing`) is simple, I don't object to that. I wonder what ports will be using it.
- potentially re-building the entire port with new flags for do-test seems a bit over-the-top, perhaps the default setup could push `BUILD_TESTING` to the normal `CMAKE_ARGS` (like an unconditional `include(CTest)` would).

So, overall: LGTM.
Comment 6 Yuri Victorovich freebsd_committer 2020-08-31 15:45:22 UTC
(In reply to Adriaan de Groot from comment #5)

> it looks awfully over-engineered with possibilities for ON, OFF, different targets

This is based on the experience with existing projects/ports, many of which require to define or undefine various other variables in order to run tests.

Yuri
Comment 7 Yuri Victorovich freebsd_committer 2020-08-31 15:46:51 UTC
(In reply to Yuri Victorovich from comment #6)

> Perhaps the default setup could push `BUILD_TESTING` to the normal `CMAKE_ARGS`

No, this would alter the build target which is undesirable. Test code should build only from the test target.
Comment 8 commit-hook freebsd_committer 2021-03-30 12:09:22 UTC
A commit references this bug:

Author: adridg
Date: Tue Mar 30 12:08:23 UTC 2021
New revision: 569551
URL: https://svnweb.freebsd.org/changeset/ports/569551

Log:
  Add support for USES=cmake:testing

  CMake-based ports have a "standard" way of controlling whether
  testing should be built, by passing -DBUILD_TESTING=ON at the
  configure stage (with some footnotes). Add a :testing modifier
  for USES=cmake that enables a boilerplate do-test target that
  rebuilds with testing enabled, and then runs the tests.

  Individual ports need to buy in to this explicitly (because
  tests might not be non-destructive).

  Submitted and explained well by yuri@

  PR:		249024
  Submitted by:	yuri

Changes:
  head/Mk/Uses/cmake.mk