cmake: add `libcurlu`/`libcurltool` for unit tests #11446

aloisklink · 2023-07-16T22:59:39Z

Add a libcurlu/libcurltool static library to the CMake build that is compiled only for unit tests. We use EXCLUDE_FROM_ALL to make sure that they're not built by default, they're only built if unit tests are built.

These libraries allow us to compile every unit test with CMake, rather than skipping some of them.

To make reviewing easier:

libcurltool is defined in the Makefile.am at:

curl/src/Makefile.am

Lines 77 to 82 in ebd83bf

    
           noinst_LTLIBRARIES = libcurltool.la 
        
           libcurltool_la_CPPFLAGS = $(AM_CPPFLAGS) \ 
        
                                     -DCURL_STATICLIB -DUNITTESTS 
        
           libcurltool_la_CFLAGS = 
        
           libcurltool_la_LDFLAGS = -static $(LINKFLAGS) 
        
           libcurltool_la_SOURCES = $(CURL_FILES)

libcurlu is defined in the Makefile.am at:

curl/lib/Makefile.am

Lines 116 to 118 in ebd83bf

    
           libcurlu_la_CPPFLAGS = $(AM_CPPFLAGS) -DCURL_STATICLIB -DUNITTESTS 
        
           libcurlu_la_LDFLAGS = $(AM_LDFLAGS) -static $(LIBCURL_LIBS) 
        
           libcurlu_la_CFLAGS = $(AM_CFLAGS)

You can also easily configure, build, and test everything using:

# configure the build/ directory for unit tests
cmake -B build/ -S . -DENABLE_THREADED_RESOLVER=OFF -DCMAKE_BUILD_TYPE=Debug -DENABLE_DEBUG=ON
# run `make testdeps all -j12`
cmake --build build/ --target=testdeps --target=all -j12
# run `make test-ci` (aka run tests)
# You may wish to modify tests/CMakeLists.txt to run tests with multiple cores
cmake --build build/ --target=test-ci

Add a `libcurlu`/`libcurltool` static library that is compiled only for unit tests. We use `EXCLUDE_FROM_ALL` to make sure that they're not built by default, they're only built if unit tests are built. These libraries allow us to compile every unit test with CMake.

bagder · 2023-07-19T14:44:21Z

any cmake person around who wants to give this PR their thumbs up? I wouldn't be able to say if this is done the right way or not.

aloisklink · 2023-07-19T18:22:19Z

any cmake person around who wants to give this PR their thumbs up?

I found two PRs that touched building unit tests in CMake, see:

CMake: Add runtests.pl targets, do not build test programs unless necessary #5368 by @Lekensteyn (reviewed by @jzakrzewski), and
Hack to build most unit tests under cmake #10722 by @dfandrich. @dfandrich seems pretty active in curl, so it's probably ideal to get a review from them, if possible!

dfandrich · 2023-07-19T19:10:33Z

I like how this gets rid of the hack I added to build unit tests with a static library, but my knowledge of CMake isn't enough to tell if there are any issues with how it's done. Having said that, it looks fine to me, although I wouldn't have expected there to be any changes necessary to src/CMakeLists.txt

aloisklink · 2023-07-19T19:38:26Z

Having said that, it looks fine to me, although I wouldn't have expected there to be any changes necessary to src/CMakeLists.txt

I've mainly placed it there to match the definition of libcurltool in

curl/src/Makefile.am

Lines 75 to 83 in ebd83bf

    
           # if unit tests are enabled, build a static library to link them with 
        
           if BUILD_UNITTESTS 
        
           noinst_LTLIBRARIES = libcurltool.la 
        
           libcurltool_la_CPPFLAGS = $(AM_CPPFLAGS) \ 
        
                                     -DCURL_STATICLIB -DUNITTESTS 
        
           libcurltool_la_CFLAGS = 
        
           libcurltool_la_LDFLAGS = -static $(LINKFLAGS) 
        
           libcurltool_la_SOURCES = $(CURL_FILES) 
        
           endif

(some of the unit tests test functions in the src/ folder)

We could move the definition to tests/unit/CMakeLists.txt, but my gut feeling is to keep the CMake implementation as similar to the existing Autotools implementation, unless there's a more common/better way to do things in CMake.

dfandrich · 2023-07-19T20:33:39Z

I can't spot any objects from src/ in the libcurlu built by autoconf, and there's no reference to it in the src/ directory. Are you sure this is needed?

aloisklink · 2023-07-19T23:39:33Z

I can't spot any objects from src/ in the libcurlu built by autoconf, and there's no reference to it in the src/ directory. Are you sure this is needed?

libcurlu doesn't use any objects from src/. There's a different library for the src/ folder that's also needed for some unit tests called libcurltool. For example, tests/unit/unit1394.c tests a parse_cert_parameter() which is defined in the src/ folder in

curl/src/tool_getparam.h

Lines 63 to 67 in 2900c29

    
           #ifdef UNITTESTS 
        
           void parse_cert_parameter(const char *cert_parameter, 
        
                                     char **certname, 
        
                                     char **passphrase); 
        
           #endif

To be honest, most of the unit tests don't need libcurltool, but I've linked it to every unit test since it makes the CMake simpler, and that's what the current autoconf build does, see

curl/tests/unit/Makefile.am

Lines 48 to 50 in 2900c29

    
           LDADD = $(top_builddir)/src/libcurltool.la   \ 
        
                   $(top_builddir)/lib/libcurlu.la      \ 
        
                   @LDFLAGS@ @LIBCURL_LIBS@ @NSS_LIBS@

dfandrich · 2023-07-19T23:50:11Z

I see it now. No objections to merging this.

jzakrzewski · 2023-07-20T10:02:58Z

Those targets should probably be build conditionally - thay are not needed if BUILD_TESTING is OFF
I haven't tried, but it seems to me that libcurlu basically is the same as libcurl but static. Doesn't it compile the stuff twice? Can we avoid that?

aloisklink · 2023-07-20T18:27:07Z

Those targets should probably be build conditionally - thay are not needed if BUILD_TESTING is OFF

Both the curlu and curltool library are marked with EXCLUDE_FROM_ALL so they won't be built by default:

curl/lib/CMakeLists.txt

Line 70 in ed6ac11

EXCLUDE_FROM_ALL

Instead, they're built automatically whenever you build anything that depends on them. And since the unit tests are the only targets that depend on curlu and curltool, they're only built when unit tests are enabled.

We could hide them behind an if(BUILD_TESTING AND ENABLE_CURLDEBUG), but:

it makes no functional difference, and
I want to change how BUILD_TESTING works in a future PR, since the way it currently works doesn't align with how most CMake projects expect it work (which makes it more difficult to use libcurl from another CMake project).

Maybe it's worth adding a comment? I think most CMake people will know what EXCLUDE_FROM_ALL does, but most curl contributors probably aren't CMake experts.

I haven't tried, but it seems to me that libcurlu basically is the same as libcurl but static. Doesn't it compile the stuff twice? Can we avoid that?

There is one other difference, it also defines UNITTESTS:

curl/lib/CMakeLists.txt

Line 73 in ed6ac11

target_compile_definitions(curlu PUBLIC UNITTESTS CURL_STATICLIB)

Defining UNITTESTS currently does nothing for libcurlu (but git log -S UNITTESTS lib/ shows a bunch of times where it recently used to do things), but it does make quite a few differences for libcurltool in the src/ folder, since there are a bunch of functions that are normally defined as static (aka internal linkage), that are defined without static so that they can be used in unit tests if UNITTESTS is defined:

curl/src/tool_xattr.c

Lines 46 to 51 in 2900c29

    
           #ifdef UNITTESTS 
        
           char *stripcredentials(const char *url); 
        
           #else 
        
           static 
        
           #endif 
        
           char *stripcredentials(const char *url)

So, we could avoid compiling everything twice for libcurlu (since UNITTESTS currently does nothing in lib/). My gut feeling is to leave it how it is though, since git log -S UNITTESTS lib/ shows it being commonly used in the past. If we do want to change it, let me know! We should first make a PR for changing it in automake, though, to keep cmake aligned with automake, something like:

--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -116 +116 @@ libcurl_la_CFLAGS = $(AM_CFLAGS) $(libcurl_la_CFLAGS_EXTRA)
-libcurlu_la_CPPFLAGS = $(AM_CPPFLAGS) -DCURL_STATICLIB -DUNITTESTS
+libcurlu_la_CPPFLAGS = $(AM_CPPFLAGS) -DCURL_STATICLIB

jzakrzewski · 2023-07-21T06:02:49Z

Both the curlu and curltool library are marked with EXCLUDE_FROM_ALL so they won't be built by default:

Damn, I totally missed the EXCLUDE_FROM_ALL there.

My gut feeling is to leave it how it is though

After reading your analysis, I can agree with that. I feel like there could be some complicated way to re-use everything that does not rely on the additional macro, but I'm not sure if it's worth it.

So it's thumbs up from me.

bagder · 2023-07-21T10:04:30Z

Thank you everyone and @aloisklink in particular. I'm merging!

Add a `libcurlu`/`libcurltool` static library that is compiled only for unit tests. We use `EXCLUDE_FROM_ALL` to make sure that they're not built by default, they're only built if unit tests are built. These libraries allow us to compile every unit test with CMake. Closes curl#11446

github-actions bot added build cmake cmdline tool tests labels Jul 16, 2023

aloisklink mentioned this pull request Jul 20, 2023

Remove unneeded unit*_LDADD overrides in unit test Makefile.inc #11494

Closed

bagder closed this in 39e7c22 Jul 21, 2023

aloisklink deleted the cmake-compile-all-unittests/ci branch July 22, 2023 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmake: add `libcurlu`/`libcurltool` for unit tests #11446

cmake: add `libcurlu`/`libcurltool` for unit tests #11446

aloisklink commented Jul 16, 2023 •

edited

bagder commented Jul 19, 2023

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023 via email

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023

jzakrzewski commented Jul 20, 2023

aloisklink commented Jul 20, 2023

jzakrzewski commented Jul 21, 2023

bagder commented Jul 21, 2023

	noinst_LTLIBRARIES = libcurltool.la
	libcurltool_la_CPPFLAGS = $(AM_CPPFLAGS) \
	-DCURL_STATICLIB -DUNITTESTS
	libcurltool_la_CFLAGS =
	libcurltool_la_LDFLAGS = -static $(LINKFLAGS)
	libcurltool_la_SOURCES = $(CURL_FILES)

	libcurlu_la_CPPFLAGS = $(AM_CPPFLAGS) -DCURL_STATICLIB -DUNITTESTS
	libcurlu_la_LDFLAGS = $(AM_LDFLAGS) -static $(LIBCURL_LIBS)
	libcurlu_la_CFLAGS = $(AM_CFLAGS)

cmake: add libcurlu/libcurltool for unit tests #11446

cmake: add libcurlu/libcurltool for unit tests #11446

Conversation

aloisklink commented Jul 16, 2023 • edited

bagder commented Jul 19, 2023

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023 via email

aloisklink commented Jul 19, 2023

dfandrich commented Jul 19, 2023

jzakrzewski commented Jul 20, 2023

aloisklink commented Jul 20, 2023

jzakrzewski commented Jul 21, 2023

bagder commented Jul 21, 2023

cmake: add `libcurlu`/`libcurltool` for unit tests #11446

cmake: add `libcurlu`/`libcurltool` for unit tests #11446

aloisklink commented Jul 16, 2023 •

edited