Created attachment 217665 [details]
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.
Created attachment 217666 [details]
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.
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.
(In reply to Tobias C. Berner from comment #2)
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.
(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.
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.
(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.
(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.
A commit references this bug:
Date: Tue Mar 30 12:08:23 UTC 2021
New revision: 569551
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@
Submitted by: yuri