Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: add libcurlu/libcurltool for unit tests #11446

Closed

Conversation

aloisklink
Copy link
Contributor

@aloisklink aloisklink commented Jul 16, 2023

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
Copy link
Member

bagder commented Jul 19, 2023

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
Copy link
Contributor Author

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:

@dfandrich
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

dfandrich commented Jul 19, 2023 via email

@aloisklink
Copy link
Contributor Author

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

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

@dfandrich
Copy link
Contributor

I see it now. No objections to merging this.

@jzakrzewski
Copy link
Contributor

  1. Those targets should probably be build conditionally - thay are not needed if BUILD_TESTING is OFF
  2. 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
Copy link
Contributor Author

  1. 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:

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.


  1. 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:

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
Copy link
Contributor

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
Copy link
Member

bagder commented Jul 21, 2023

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

@bagder bagder closed this in 39e7c22 Jul 21, 2023
@aloisklink aloisklink deleted the cmake-compile-all-unittests/ci branch July 22, 2023 00:54
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
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
vszakats added a commit to vszakats/curl that referenced this pull request May 18, 2024
vszakats added a commit to vszakats/curl that referenced this pull request May 18, 2024
Before this patch, building unit tests required `CURLDEBUG` to be set
either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON`.

After fixing the build issues in curl#13694, the above requirement is no
longer necessary as a workaround. This patch makes unit tests build
unconditionally.

Depends-on: curl#13694
Ref: curl#13697
Follow-up to 39e7c22 curl#11446
Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request May 18, 2024
Before this patch, building unit tests (= the `testdeps` target)
required `-DCURLDEBUG` be set either via `ENABLE_DEBUG=ON`
or `ENABLE_CURLDEBUG=ON`.

After fixing the build issues in curl#13694, the above requirement is no
longer necessary as a workaround. This patch makes unit tests build
unconditionally.

Depends-on: curl#13694 (fix build issues)
Depends-on: curl#13697 (fix unit test issue revealed by Old Linux CI job)
Follow-up to 39e7c22 curl#11446
Closes curl#13698
vszakats added a commit to vszakats/curl that referenced this pull request May 27, 2024
Before this patch, building unit tests (= the `testdeps` target)
required `-DCURLDEBUG` be set either via `ENABLE_DEBUG=ON`
or `ENABLE_CURLDEBUG=ON`.

After fixing the build issues in curl#13694, the above requirement is no
longer necessary as a workaround. This patch makes unit tests build
unconditionally.

Depends-on: curl#13694 (fix build issues)
Depends-on: curl#13697 (fix unit test issue revealed by Old Linux CI job)
Follow-up to 39e7c22 curl#11446
Closes curl#13698
vszakats added a commit that referenced this pull request May 27, 2024
Before this patch, the `testdeps` build target required `-DCURLDEBUG`
be set either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON` to build
the curl unit tests.

After fixing build issues in #13694, we can drop this requirement and
build unit tests unconditionally.

Depends-on: #13694
Depends-on: #13697 (fix unit test issue revealed by Old Linux CI job)
Follow-up to 39e7c22 #11446
Closes #13698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants