cmake: generalize the handling of the `UNIT_TEST_OBJS` list

Commit Message

Johannes Schindelin Sept. 18, 2024, 7:29 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In a15d4465a991 (cmake: also build unit tests, 2023-09-25), I
accommodated the CMake definition. Seeing that a `UNIT_TEST_OBJS` list
was introduced that was built by transforming the `UNIT_TEST_PROGRAMS`
list and then adding a single, hard-coded file
("t/unit-tests/test-lib.c"), I decided to hard-code that in the CMake
definition, too.

The reason why I hard-coded it instead of imitating the
`parse_makefile_for_sources()` paradigm that was used elsewhere when
using the `Makefile` as source of truth for given lists of files: This
function expects _only_ hard-coded values, and that transformed
`UNIT_TEST_PROGRAMS` list complicated everything.

In 872721538c26 (cmake: fix build of `t-oidtree`, 2024-07-12), I
accommodated the CMake definition again, after seeing that the
`UNIT_TEST_OBJS` was still defined via that transformed list but now
appending _two_ hard-coded files ("t/unit-tests/lib-oid.c" joined the
fray).

In 428672a3b16 (Makefile: stop listing test library objects twice,
2024-09-16), the `Makefile` was changed so that `UNIT_TEST_OBJS` is
finally only constructed using hard-coded file names just like the other
`*_OBJS` variables. I missed that and therefore did not adjust the CMake
definition. Besides, the code was working, so there was no real need to
adjust it.

With a4f50bb1e9b (t/unit-tests: introduce reftable library, 2024-09-16),
however, the `UNIT_TEST_OBJS` list became a trio, and the CMake
definition has to be adjusted again. Now that we can use the
`parse_makefile_for_sources()` function without many complications,
let's do that.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
    cmake: generalize the handling of the UNIT_TEST_OBJS list
    
    This is an add-on for ps/reftable-exclude to let it build with CMake and
    Visual C.
    
    This patch on its own does not actually fix vs-build; The patch "cmake:
    stop looking for REFTABLE_TEST_OBJS in the Makefile" which I submitted
    in
    https://lore.kernel.org/git/[e-mail]
    is also needed for a successful vs-build.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1797%2Fdscho%2Freftable-exclude%2Bcmake-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1797/dscho/reftable-exclude+cmake-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1797

 contrib/buildsystems/CMakeLists.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)


base-commit: 18695250667912d8278e76dce453706c3d488173

Comments

Junio C Hamano Sept. 24, 2024, 1:58 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> With a4f50bb1e9b (t/unit-tests: introduce reftable library, 2024-09-16),
> however, the `UNIT_TEST_OBJS` list became a trio, and the CMake
> definition has to be adjusted again. Now that we can use the
> `parse_makefile_for_sources()` function without many complications,
> let's do that.

Am I correct to understand that it is not "trio"-ness (has three
things) that makes the approach feasible, but the fact that all
three things are concrete values?

The longer-term aspiration is to migrate everything to clar-based
unit tests, so the list will hopefully keep getting shorter and then
become empty (https://lore.kernel.org/git/[e-mail]/).

Will queue.  Thanks.

Johannes Schindelin Sept. 29, 2024, 5:48 p.m. UTC | #2

Hi,

On Mon, 23 Sep 2024, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > With a4f50bb1e9b (t/unit-tests: introduce reftable library, 2024-09-16),
> > however, the `UNIT_TEST_OBJS` list became a trio, and the CMake
> > definition has to be adjusted again. Now that we can use the
> > `parse_makefile_for_sources()` function without many complications,
> > let's do that.
>
> Am I correct to understand that it is not "trio"-ness (has three
> things) that makes the approach feasible, but the fact that all
> three things are concrete values?

No, the fact that a third entry was added made it obvious that adding
hard-coded equivalents to the CMake definition is too much of a
whac-a-mole thing and that the `Makefile` parsing approach would be
preferable. With just two items, it was still kind of okay to duplicate
the list, but three is a trend.

> The longer-term aspiration is to migrate everything to clar-based
> unit tests, so the list will hopefully keep getting shorter and then
> become empty (https://lore.kernel.org/git/[e-mail]/).

Once it becomes empty, I will have to send another CMake-related patch
similar to when the `REFTABLE_TEST_OBJS` list went away, as the CMake
definition cannot handle such empty lists.

Ciao,
Johannes

Junio C Hamano Sept. 30, 2024, 6:06 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

>> The longer-term aspiration is to migrate everything to clar-based
>> unit tests, so the list will hopefully keep getting shorter and then
>> become empty (https://lore.kernel.org/git/[e-mail]/).
>
> Once it becomes empty, I will have to send another CMake-related patch
> similar to when the `REFTABLE_TEST_OBJS` list went away, as the CMake
> definition cannot handle such empty lists.

It may be ideal if all contributors learned and updated inputs to
all the build process options like cmake, make, and autoconf at the
same time as they futz with the code they build (in other words, a
change that makes such a list to empty includes cmake update), but
different people chipping in with help in their strong areas and
polishing the system as a whole as a collaborative effort is
probably the best practical alternative.

Thanks.

Patch

diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 832f46b316b..249c72075bb 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -975,13 +975,14 @@  parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
 list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
 
 #unit-tests
-add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
-add_library(unit-test-lib-oid OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/lib-oid.c)
+parse_makefile_for_sources(unit-test_SOURCES "UNIT_TEST_OBJS")
+list(TRANSFORM unit-test_SOURCES REPLACE "\\$\\(UNIT_TEST_DIR\\)/" "${CMAKE_SOURCE_DIR}/t/unit-tests/")
+add_library(unit-test-lib STATIC ${unit-test_SOURCES})
 
 parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
 foreach(unit_test ${unit_test_PROGRAMS})
 	add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
-	target_link_libraries("${unit_test}" unit-test-lib unit-test-lib-oid common-main)
+	target_link_libraries("${unit_test}" unit-test-lib common-main)
 	set_target_properties("${unit_test}"
 		PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests/bin)
 	if(MSVC)