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

build: add Debug, TrackMemory, ECH to feature list #14096

Closed
wants to merge 5 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jul 4, 2024

Also:

  • remove stray ECH and HTTPSRR from cmake protocol list.

  • stop excluding Debug and TrackMemory in test1013.pl.

  • configure: delete CURL_CHECK_CURLDEBUG check.
    Ref: 065047d
    This check was effectively doing nothing, except disabling
    --enable-curldebug in curl-config for
    Cygwin/MSYS/cegcc/OS2/AIX targets with c-ares enabled.

Closes #14096


  • TODO: Delete CharConv references from tests.

@vszakats vszakats added the cmake label Jul 4, 2024
@github-actions github-actions bot added the build label Jul 4, 2024
@vszakats
Copy link
Member Author

vszakats commented Jul 4, 2024

Maybe also this for autotools, though it was explicitly deleted when adding ECH support in a362962:

iff --git a/configure.ac b/configure.ac
index 6c9a3f1f1..e89d97ae9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4817,6 +4817,10 @@ else
   AC_MSG_RESULT([no])
 fi
 
+if test "x$ECH_ENABLED" = "x1"; then
+  SUPPORT_FEATURES="$SUPPORT_FEATURES ECH"
+fi
+
 if test ${ac_cv_sizeof_curl_off_t} -gt 4; then
   if test ${ac_cv_sizeof_off_t} -gt 4 -o \
      "$curl_win32_file_api" = "win32_large_files"; then

@vszakats vszakats changed the title cmake: add Debug, TrackMemory, fix ECH, in feature list build: add Debug, TrackMemory, fix ECH, in feature list Jul 4, 2024
@vszakats vszakats changed the title build: add Debug, TrackMemory, fix ECH, in feature list build: add Debug, TrackMemory, ECH to feature list Jul 4, 2024
@vszakats
Copy link
Member Author

vszakats commented Jul 4, 2024

Why are Debug and TrackMemory features not supported in curl-config?:

# These features are not supported by curl-config
@curl = grep(!/^(Debug|TrackMemory|CharConv)$/i, @curl);

or curl-debug not expected to know about them: 2f3d520 ?

Do we still need CharConv? It doesn't seem to be enabled anymore.

@bagder
Copy link
Member

bagder commented Jul 4, 2024

Maybe also this [ECH] for autotools, though it was explicitly deleted when adding ECH support in a362962:

I wonder if that perhaps was a mistake? Or perhaps done so because there is no corresponding internal feature bit for ECH.

Why are Debug and TrackMemory features not supported in curl-config?:

I believe the thinking was that they are developer-oriented, not end user oriented. But since they are exposed by curl itself anyway I think that logic is flawed....

Do we still need CharConv? It doesn't seem to be enabled anymore.

Correct. We removed support for that in 7.82.0

@bagder
Copy link
Member

bagder commented Jul 4, 2024

perhaps done so because there is no corresponding internal feature bit for ECH.

Oops, it is added as a feature name though.

@vszakats
Copy link
Member Author

vszakats commented Jul 4, 2024

Hit a bump where configure thinks TrackMemory isn't supported,
but it's appearing on the runtime feature list as enabled:
https://github.com/curl/curl/actions/runs/9792965739/job/27039899818?pr=14096#step:6:178

Happening with msys/cygwin autotools builds.

It's caused by configure first checking for the intent of enabling TrackMemory and setting the CURLDEBUG macro (in CURL_CHECK_OPTION_CURLDEBUG), then checking if it's supported (in CURL_CHECK_CURLDEBUG via XC_CHECK_LT_SHLIB_USE_NO_UNDEFINED) and overriding the previous decision, leaving CURLDEBUG macro set, but the internal flag disabled. It was implemented in the single commit 065047d.

CMake doesn't have the extra check. Either it can be deleted, or if useful, fix the place defining CURLDEBUG to remain in sync with the interal flag.

@vszakats
Copy link
Member Author

vszakats commented Jul 4, 2024

It seems that since that original commit in 2009-06-09, the second check performed in CURL_CHECK_CURLDEBUG had the only consequence of disabling --enable-curldebug in curl-config for c-ares builds. To me this tells it's likely unnecessary, otherwise somebody using Cygwin, mingw, cegcc, OS/2 or AIX would have complained? It also only affects development/debug builds, so even if deleting it messes up something, the impact is small.

Unless I'm misreading/missing something.

@vszakats vszakats closed this in 96a1a05 Jul 6, 2024
@vszakats vszakats deleted the cmake-feat-fix branch July 6, 2024 22:34
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

2 participants