Can't use in continuous integration.

[BengtGustafsson]

I get a problem with building and using QuaZip in our continuous integration pipeline.

This is as the cmakelists file sets the QUAZIP_ENABLE_TESTS variable forecfulle as we have Qt5Network_FOUND AND Qt5Test_FOUND set (apprently).

This means that the qztest subdirectory is included but as this is done with the EXCLUDE_FROM_ALL flag the test program is not built by our build which does add_subdirectory(quazip). However, the cmakelists.txt file in qztest does a add_test which adds the not-built test to the list of tests run by ctest.

On a developers machine this can be offset by manually compiling and linking the test program before running ctest but this becomes very cumbersome in a CI system as there is no platform independent way to build the test program. The only idea I have would be to add a custom build step to our main application which runs cmake -B on the build directory corresponding to the qztest source subdirectory. This is frightingly ugly and brittle.

So please either remove the EXCLUDE_FROM_ALL flag and the corresponding project() call in the qztest cmakelists file or make it actually possible to disable testing from a calling cmakelists.txt file.

[BengtGustafsson]

Found this: https://gitlab.kitware.com/cmake/cmake/-/issues/20212

[stachenov]

I think I understand the problem in general, but I've no clue how to properly fix it without unreasonably breaking anything for the existing users.

Lately, there was a PR (#130) that turned QUAZIP_ENABLE_TESTS from an auto-detected variable into an option, so it is possible now to disable tests externally. Isn't this enough?

As for removing the EXCLUDE_FROM_ALL flag, I think it makes sense too, especially when building the tests is an option now, but what about “the corresponding project() call”? I don't think I get it. I'm not overly experienced with CMake; in fact, QuaZip is the only project I'm using CMake with, so I'm not familiar with all the plumbing, just with the basics, so I don't understand the relationship between this flag, the targets, the tests, the CMakeLists files, and so on.

[BengtGustafsson]

It seems that PR130 fixes the problem. I just downloaded the source zip for the latest tag (1.1) which I assume predates this PR. Do you have plans for a new tag soon?

Sorry for being unclear regarding the project() call, this is in the cmakelists file of the test program, which makes it possible to run cmake directly on the test program (and due to EXCLUDE_FROM_ALL makes it mandatory to be able to run tests).

With the test enable an option it is still valuable to remove these things as then you can run the tests in CI without problems, by just enabling the tests. Also, with this working it would be reasonable to have tests enabled by default.

[stachenov]

I can release the next version any time, no problem. As long as I figure out what's the right thing to do with all this testing stuff...

I don't really run CMake directly on the test program, but rather run CMake with --target all qztest whenever I need tests to be run. But I get it that it's fine for running the tests manually, and not so much for CI.

So... let me confirm if I get it right: if I just remove that single project line from qztest/CMakeLists.txt and replace ${PROJECT_NAME} with qztest, then qztest will become just another target within the QuaZip project, and will compile if tests are enabled? I tried to do it, but now the tests are built, but not run, unless I specify --target check to cmake. How are you going to run the tests in CI then? Or am I missing something here? I really feel the lack of CMake/CI experience now.

I'm also at a complete loss as to whether enable the tests by default or not. There was a short discussion in #130, and I got it that users expect the tests to be disabled by default. Now you're saying the exact opposite. What's the right thing to do, and why?

[BengtGustafsson]

I don't think there is a "standard" for whether to run tests. The safer bet is of course the have them on, but otoh people don't want to run the same test on the same software all the time, and presumably you don't set a tag unless the unit tests run. So either way is fine, just that it is possible to select when using quazip as part of a larger project is enough.

When it comes to tests not being run I'm at a loss. There is an add_test() call which should be enough. There is also an enable_testing() that has to be called somewhere in the cmakelists tree (and maybe before add_test, I don't remember). When this is done you also have to execute ctest (or in VS build the RUN_TESTTS target) to actually get them to run, it doesn't happen as part of the regular build.

Checking the latest code I think it looks good. enable_testing is inside the if for testing and before the add_subdirectory so that should be find. There is even and extra line "add_custom_target(check COMMAND ${CMAKE_CTEST_COMMAND} --verbose DEPENDS qztest)" in the test cmakelists which should add another way to run ctest apart from RUN_TESTS which I'm not sure if it is available outside VS. So on linux you should be able to do ninja check or make check to get the test to run.

You could consider setting the option diferently depending on the result of:

get_directory_property(hasParent PARENT_DIRECTORY)

hasParent is now non-empty only if this cmakelists file was included by some parent using add_subdirectory, in which case you could set the option to OFF.

[stachenov]

I think I'll just leave the tests off by default, so only the users who know what they're doing will turn them on. Otherwise it seems like an unnecessary complication, for example, because they need to have QtTest installed, or their project requires some kind of special setup.

I usually don't invoke make directly, so I typically run tests like this:

cmake --build <insert build dir here> --target check

That's where that custom check target becomes useful. However, I don't know how useful it is for CI.

Could you check the latest master. I've just pushed the discussed changes. Works nice for me, but I'm not sure whether it'll work nice for CI. If it's OK, then I'll make a release.

[BengtGustafsson]

I saw that you changed this in master. I'm just compiling with the latest. Works.

[stachenov]

Just released v1.2 with this fix. Anything more on this issue or should it be closed now?

[BengtGustafsson]

No that's fine now. Thanks for the help!