-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
build: untangle UNITTESTS
and DEBUGBUILD
macros
#13694
Conversation
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
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
Based on failing then fixed jobs in curl#13689
@@ -54,7 +54,7 @@ | |||
#define MAX_HSTS_DATELENSTR "64" | |||
#define UNLIMITED "unlimited" | |||
|
|||
#ifdef DEBUGBUILD | |||
#if defined(DEBUGBUILD) || defined(UNITTESTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
DEBUGBUILD
guards that should beUNITTESTS
, in libcurlcode used by unit tests.
UNITTEST
attribute between declarations and definitions.DEBUGBUILD
guard from testunit2600
.unit1660
)and
test0446
.This fixes building tests with
CURLDEBUG
enabled butDEBUGBUILD
disabled. This can happen when building tests with CMake with
ENABLE_DEBUG=ON
in Release config, or withENABLE_CURLDEBUG=ON
and without
ENABLE_DEBUG=ON
. Possibly also with autotoolswhen using
--enable-curldebug
without--enable-debug
.Test results:
(the two failures are unrelated, subject to PR cmake: fix
-Wredundant-decls
in unity/mingw-w64/gcc/curldebug/DLL builds #13705)Ref: #13592 (issue discovery)
Ref: #13689 (CI testing this PR with
DEBUGBUILD
/CURLDEBUG
combinations)Closes #13694