build: untangle UNITTESTS and DEBUGBUILD macros#13694
build: untangle UNITTESTS and DEBUGBUILD macros#13694vszakats wants to merge 4 commits intocurl:masterfrom
UNITTESTS and DEBUGBUILD macros#13694Conversation
lib/hsts.c
Outdated
There was a problem hiding this comment.
Why is this needed?
The unit tests assume that DEBUGBUILD is defined as a prereq for running. This change seems to say that you can build with UNITTESTS without DEBUGBUILD ?
There was a problem hiding this comment.
Unit tests (and any other test) are building fine without either DEBUGBUILD or CURLDEBUG. (That is after fixing the places where DEBUGBUILD was used to enable functions for unit tests.)
This is an outlier case where the deltatime variable of a debug feature (CURL_TIME) is actually required for building a unit test.
I wish we could drop the distinction completely and just use one of them. I can't remember the difference, I don't think anyone uses them separately. Am I wrong? |
They are not the same thing, but their names don't make it trivial to figure out which does what exactly:
In CMake, Yet, they are separate and possible to use separately, with quite a bit of breakage around the edges. The goal of this patch isn't really to untangle these two from each other, but to fix to use the Merging This PR is an attempt to fix all existing build combinations. |
|
There seem to be a few suspicious cases, where I think we might consider renaming With the list of related open PRs in the air, I'd prefer merging these first, then continue tackling more. Two PRs are blocked by this one: #13705 and #13698. |
UNITTESTS and DEBUGBUILD macros
UNITTESTS and DEBUGBUILD macrosUNITTESTS and DEBUGBUILD macros
As Viktor mentions above, yes they are separate because CURLDEBUG is (supposed to be) the guard for the trackmemory feature. Typically I run debug builds of curl with DEBUGBUILD but not CURLDEBUG.
I'm with you on the first part (maybe something shorter?) but not the second part. Renaming DEBUGBUILD to CURL_DEBUG is confusing to me since we've been using CURLDEBUG for memory tracking. |
Indeed, I agree. If we want to rename them (or one of them), we can brainstorm for new names. Starting with Anyway, after tidying this up, I think it will be a little bit harder to make a mistake by copy-pasting existing code or looking up existing uses. As for edit: merging |
|
I can see UNITTESTS being dependent on DEBUGBUILD but not the other way around |
Before this patch `ENABLE_DEBUG=ON` always enabled the TrackMemory (aka `ENABLE_CURLDEBUG=ON`) feature, but required the `Debug` CMake configration to actually enable curl debug features (aka `-DDEBUGBUILD`). Curl debug features do not require compiling with C debug options. This also made enabling debug features unintuitive and complicated to use. Due to other issues (subject to PR #13694) it also caused an error in default (and `Release`/`MinSizeRel`/`RelWithDebInfo`) configs, when building the `testdeps` target: ``` ld: CMakeFiles/unit1395.dir/unit1395.c.o: in function `test': unit1395.c:(.text+0x1a0): undefined reference to `dedotdotify' ``` Ref: https://github.com/curl/curl/actions/runs/9037287098/job/24835990826#step:3:2483 Fix it by always defining `DEBUGBUILD` when setting `ENABLE_DEBUG=ON`. Decoupling this option from the selected CMake configuration. Note that after this patch `ENABLE_DEBUG=ON` unconditionally enables curl debug features. These features are insecure and unsuited for production. Make sure to omit this option when building for production in default, `Release` (and other not-`Debug`) modes. Also delete a workaround no longer necessary in GHA CI jobs. Ref: 1a62b6e (2015-03-03) Ref: #13583 Closes #13592
880859e to
ce83d3c
Compare
c21e7da to
6f51279
Compare
- fix `DEBUGBUILD` guards that should be `UNITTESTS`. - fix guards for libcurl functions used in unit tests only. - sync `UNITTEST` attribute between declarations with definitions. - drop `DEBUGBUILD` guard from test `unit2600`.
- fix guards for libcurl code used by both unit tests (`unit1660`) and `test0446`.
0081cb3 to
9ccdca8
Compare
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
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
…uilds
It affected cmake-unity shared-curltool curldebug mingw-w64 gcc builds
when building the `testdeps` target.
Apply the solution already used in `lib/base64.c` and `lib/dynbuf.c`
to fix it.
Also update an existing GHA CI job to test the issue fixed.
```
In file included from curl/lib/version_win32.c:35,
from curl/_bld/src/CMakeFiles/curl.dir/Unity/unity_0_c.c:145:
curl/lib/memdebug.h:52:14: error: redundant redeclaration of 'curl_dbg_logfile' [-Werror=redundant-decls]
52 | extern FILE *curl_dbg_logfile;
| ^~~~~~~~~~~~~~~~
In file included from curl/src/slist_wc.c:32,
from curl/_bld/src/CMakeFiles/curl.dir/Unity/unity_0_c.c:4:
curl/lib/memdebug.h:52:14: note: previous declaration of 'curl_dbg_logfile' with type 'FILE *' {aka 'struct _iobuf *'}
52 | extern FILE *curl_dbg_logfile;
| ^~~~~~~~~~~~~~~~
curl/lib/memdebug.h:55:44: error: redundant redeclaration of 'curl_dbg_malloc' [-Werror=redundant-decls]
55 | CURL_EXTERN ALLOC_FUNC ALLOC_SIZE(1) void *curl_dbg_malloc(size_t size,
| ^~~~~~~~~~~~~~~
curl/lib/memdebug.h:55:44: note: previous declaration of 'curl_dbg_malloc' with type 'void *(size_t, int, const char *)' {aka 'void *(long long unsigned int, int, const char *)'}
55 | CURL_EXTERN ALLOC_FUNC ALLOC_SIZE(1) void *curl_dbg_malloc(size_t size,
| ^~~~~~~~~~~~~~~
[...]
curl/lib/memdebug.h:110:17: error: redundant redeclaration of 'curl_dbg_fclose' [-Werror=redundant-decls]
110 | CURL_EXTERN int curl_dbg_fclose(FILE *file, int line, const char *source);
| ^~~~~~~~~~~~~~~
curl/lib/memdebug.h:110:17: note: previous declaration of 'curl_dbg_fclose' with type 'int(FILE *, int, const char *)' {aka 'int(struct _iobuf *, int, const char *)'}
110 | CURL_EXTERN int curl_dbg_fclose(FILE *file, int line, const char *source);
| ^~~~~~~~~~~~~~~
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49840554/job/a4aoet17e9qnqx1a#L362
After: https://ci.appveyor.com/project/curlorg/curl/builds/49843735/job/hbo2uah2vj0ns523
Ref: #13689 (CI testing this PR with `DEBUGBUILD`/`CURLDEBUG`/shared-static combinations)
Depends-on: #13694
Depends-on: #13800
Closes #13705
`CURLDEBUG` is meant to enable memory tracking, but in a bunch of cases, it was protecting debug features that were supposed to be guarded with `DEBUGBUILD`. Replace these uses with `DEBUGBUILD`. This leaves `CURLDEBUG` uses solely for its intended purpose: to enable the memory tracking debug feature. Also: - autotools: rely on `DEBUGBUILD` to enable `checksrc`. Instead of `CURLDEBUG`, which worked in most cases because debug builds enable `CURLDEBUG` by default, but it's not accurate. - include `lib/easyif.h` instead of keeping a copy of a declaration. - add CI test jobs for the build issues discovered. Ref: #13694 (comment) Closes #13718
Before this patch, autotools disabled building unit tests for non-debug-enabled (`DEBUGBUILD`) builds. runtests skipped running this combination, though they were built in cmake builds. There seems to be no technical reason anymore for these restrictions. This patch removes them, allowing to build and run unit tests for non-debug-enabled builds. To improve unit test build and run coverage. - autotools: do not disable building unit tests for non-debug-enabled build. Bringing behavior closer to cmake builds. (There are still exceptions in autotools, something for another PR) - runtests: run unit tests for non-debug-enabled builds. It extends coverage by 50 tests. - `lib/altsvc.c`: fix to include `CURL_TIME` support in libcurlu, for unit tests. It fixes test 1654, and syncs `CURL_TIME` behavior with test 1660 and `lib/hsts.c`. Ref: 10a7d05 Ref: fc8e0de #13694 Ref: 99f78cb #16770 Closes #16771
Before this patch, autotools disabled building unit tests for non-debug-enabled (`DEBUGBUILD`) builds. runtests skipped running this combination, though they were built in cmake builds. There seems to be no technical reason anymore for these restrictions. This patch removes them, allowing to build and run unit tests for non-debug-enabled builds. To improve unit test build and run coverage. - autotools: do not disable building unit tests for non-debug-enabled build. Bringing behavior closer to cmake builds. (There are still exceptions in autotools, something for another PR) - runtests: run unit tests for non-debug-enabled builds. It extends coverage by 50 tests. - `lib/altsvc.c`: fix to include `CURL_TIME` support in libcurlu, for unit tests. It fixes test 1654, and syncs `CURL_TIME` behavior with test 1660 and `lib/hsts.c`. Ref: 10a7d05 Ref: fc8e0de curl#13694 Ref: 99f78cb curl#16770 Closes curl#16771
DEBUGBUILDguards that should beUNITTESTS, in libcurlcode used by unit tests.
UNITTESTattribute between declarations and definitions.DEBUGBUILDguard from testunit2600.unit1660)and
test0446.This fixes building tests with
CURLDEBUGenabled butDEBUGBUILDdisabled. This can happen when building tests with CMake with
ENABLE_DEBUG=ONin Release config, or withENABLE_CURLDEBUG=ONand without
ENABLE_DEBUG=ON. Possibly also with autotoolswhen using
--enable-curldebugwithout--enable-debug.Test results:
(the two failures are unrelated, subject to PR cmake: fix
-Wredundant-declsin unity/mingw-w64/gcc/curldebug/DLL builds #13705)Ref: #13592 (issue discovery)
Ref: #13689 (CI testing this PR with
DEBUGBUILD/CURLDEBUGcombinations)Closes #13694