Skip to content

Conversation

@bagder
Copy link
Member

@bagder bagder commented Feb 25, 2025

To help applications doing the right thing easier, change some enum values into defines with L suffixes so that they get the corect type easier when used with curl_easy_setopt().

To reduce the risk that this breaks the compile for any existing user, the previously provided enums are still provided, but the values to use are not defined by the enums.

@testclutch
Copy link

Analysis of PR #16482 at d21c4f66:

Test 503 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@bagder bagder marked this pull request as ready for review February 25, 2025 22:23
@github-actions github-actions bot added the CI Continuous Integration label Feb 25, 2025
@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

@vszakats look, I figured out some of the mysterious RTSP failures on macOS gcc-12. What's curious is that they only failed these particular builds...

@vszakats
Copy link
Member

vszakats commented Feb 25, 2025

@vszakats look, I figured out some of the mysterious RTSP failures on macOS gcc-12. What's curious is that they only failed these particular builds...

Wow, that's super nice! Those were puzzling. (edit: Ref: item 15 in c349bd6 #14097)

I wonder if there is any way to make these fall out early somehow.
Also why only macOS and why only gcc? It'd be interesting to see
how gcc-13 and gcc-14 are affected.

To help applications do the right thing easier, change some enum values
into defines with L suffixes so that they get the corect type (long)
easier when used with curl_easy_setopt(). This also fixes a few of our
own libtests.

To reduce the risk that this change breaks the compile for any existing
users, the previously provided enums are still provided, but the values
to use are not defined by the enums.

Closes #16482
@bagder bagder force-pushed the bagder/public-enum-define branch from b54099b to 9ea40e7 Compare February 25, 2025 22:58
@jay
Copy link
Member

jay commented Feb 25, 2025

To reduce the risk that this breaks the compile for any existing user, the previously provided enums are still provided, but the values to use are not defined by the enums.

I think it's unnecessary to keep the enums around. When would someone be using the enum type? In C++ ? They'd already have to change it for the other symbols then wouldn't they?

@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

I think it's unnecessary to keep the enums around

I figured this was the most conservative approach. The curl tool itself actually uses one of them and I took that as a hint that maybe there are a few other users too out there in the world who do. And keeping them is a small cost I think. If it helps not breaking a few builds out there.

@bagder
Copy link
Member Author

bagder commented Feb 25, 2025

It'd be interesting to see how gcc-13 and gcc-14 are affected.

It seems reasonable to presume that they are/were. I mean this was just a plain bug in our tests that pass in ints where longs should be used.

@bagder bagder closed this in 2ec0037 Feb 26, 2025
@bagder bagder deleted the bagder/public-enum-define branch February 26, 2025 06:59
vszakats added a commit that referenced this pull request Mar 6, 2025
It fixes tests 1539, and 2402, 2404 (for non-Secure Transport), on macOS
with the gcc compiler.

Also unignore these tests in GHA/macos for non-secure transport.

Ref: c349bd6 #14097 (issue 15.)
Ref: 7b0240c #16539
Ref: 2ec0037 #16482

Closes #16580
vszakats added a commit that referenced this pull request Mar 6, 2025
To fix this test on macOS with the gcc compiler.

Also unignore test 1156 in GHA/macos.

Ref: c349bd6 #14097 (issue 15.)
Ref: 7b0240c #16539
Ref: 2ec0037 #16482

Closes #16579
Comment on lines 2320 to 2332
/* These enums are for use with the CURLOPT_NETRC option. */
#define CURL_NETRC_IGNORED 0L /* The .netrc will never be read.
This is the default. */
#define CURL_NETRC_OPTIONAL 1L /* A user:password in the URL will be preferred
to one in the .netrc. */
#define CURL_NETRC_REQUIRED 2L /* A user:password in the URL will be ignored.
Unless one is set programmatically, the
.netrc will be queried. */
enum CURL_NETRC_OPTION {
CURL_NETRC_IGNORED, /* The .netrc will never be read.
* This is the default. */
CURL_NETRC_OPTIONAL, /* A user:password in the URL will be preferred
* to one in the .netrc. */
CURL_NETRC_REQUIRED, /* A user:password in the URL will be ignored.
* Unless one is set programmatically, the .netrc
* will be queried. */
CURL_NETRC_LAST
/* we set a single member here, just to make sure we still provide the enum,
but the values to use are defined above with L suffixes */
CURL_NETRC_LAST = 3
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this change breaks old code? vcpkg CI builds CMake 3.22.2 (!) as a test port. With curl 8.13.0 rc3, that test port fails on MSVC, GCC, AppleClang. x64-linux (GCC (Ubunto 22.04?)):

In file included from /mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Utilities/cm3p/curl/curl.h:8,
                 from /mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.h:9,
                 from /mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.cxx:3:
/mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.cxx: In function ‘std::string cmCurlSetNETRCOption(CURL*, const string&, const string&)’:
/mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.cxx:78:26: error: invalid conversion from ‘long int’ to ‘CURL_NETRC_OPTION’ [-fpermissive]
   78 |       curl_netrc_level = CURL_NETRC_OPTIONAL;
      |                          ^~~~~~~~~~~~~~~~~~~
      |                          |
      |                          long int
/mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.cxx:80:26: error: invalid conversion from ‘long int’ to ‘CURL_NETRC_OPTION’ [-fpermissive]
   80 |       curl_netrc_level = CURL_NETRC_REQUIRED;
      |                          ^~~~~~~~~~~~~~~~~~~
      |                          |
      |                          long int
/mnt/vcpkg-ci/b/cmake/src/17503183d2-2d56d3dcb1.clean/Source/cmCurl.cxx:82:26: error: invalid conversion from ‘long int’ to ‘CURL_NETRC_OPTION’ [-fpermissive]
   82 |       curl_netrc_level = CURL_NETRC_IGNORED;
      |                          ^~~~~~~~~~~~~~~~~~
      |                          |
      |                          long int

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patched in recent CMake: Kitware/CMake@1b0c92a

Maybe curl/curl.h shall #define or typedef CURL_NETRC_OPTION to long?

bob-beck pushed a commit to openbsd/ports that referenced this pull request Apr 4, 2025
Kitware/CMake@1b0c92a
curl/curl#16482

diff from jca (i added a comment to the patch), ok tb (implicit ok sthen ;)
sthen added a commit to sthen/ports that referenced this pull request Apr 16, 2025
Kitware/CMake@1b0c92a
curl/curl#16482

diff from jca (i added a comment to the patch), ok tb (implicit ok sthen ;)
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
To help applications do the right thing easier, change some enum values
into defines with L suffixes so that they get the corect type (long)
easier when used with curl_easy_setopt(). This also fixes a few of our
own libtests.

To reduce the risk that this change breaks the compile for any existing
users, the previously provided enums are still provided, but the values
to use are not defined by the enums.

This change "magically" fixes a few RTSP test failures we have had on
64-bit platforms because those options were not see using longs
properly.

Closes curl#16482
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
It fixes tests 1539, and 2402, 2404 (for non-Secure Transport), on macOS
with the gcc compiler.

Also unignore these tests in GHA/macos for non-secure transport.

Ref: c349bd6 curl#14097 (issue 15.)
Ref: 7b0240c curl#16539
Ref: 2ec0037 curl#16482

Closes curl#16580
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
To fix this test on macOS with the gcc compiler.

Also unignore test 1156 in GHA/macos.

Ref: c349bd6 curl#14097 (issue 15.)
Ref: 7b0240c curl#16539
Ref: 2ec0037 curl#16482

Closes curl#16579
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Apr 27, 2025
Our macOS CI jobs execute on GitHub Actions runners, and in our
"build-earliest" job we compile a custom version of Git and run our
test suite with that version, which at the moment is Git v2.0.0.
We compile our custom version of Git against the version of the
curl library supplied by Homebrew on macOS GitHub Actions runners.

Recently, after Homebrew updated its version of curl to 8.13.0, our
"build-earliest" job been failing because the C compiler reports that
the CURLUSESSL_TRY identifier is not declared in the call to
curl_easy_setopt() in Git's get_curl_handle() function:

  https://github.com/git/git/blob/v2.0.0/http.c#L363

Between Git versions 1.8.3 and 2.33.8, that curl_easy_setopt() call
would only be compiled if the CURLOPT_USE_SSL macro was defined.
Fortunately, so long as curl v7.17.0 or later was in use, this was
always the case, but only because Git itself defined that macro,
not curl.  Under the same conditions and between the same Git versions,
Git also defined the CURLUSESSL_TRY macro that is now causing problems
when we try to compile Git v2.0.0 with curl v8.13.0.

Starting with commit git/git@4bc444e
in Git v1.8.3, Git's http.h source file included macro definitions of
the CURLOPT_USE_SSL and CURLUSESSL_TRY names.  These were set to the
values CURLOPT_FTP_SSL and CURLFTPSSL_TRY, respectively, so long as
the CURLOPT_USE_SSL macro name was not defined elsewhere and the
CURLOPT_FTP_SSL name was defined:

  https://github.com/git/git/blob/v1.8.3/http.h#L45-L52
  https://github.com/git/git/blob/v2.0.0/http.h#L44-L51

These macro definitions were preceded with the following comment:

  CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
  and the constants were known as CURLFTPSSL_*

This comment specifically refers to the introduction to the curl library
of the CURLOPT_USE_SSL CURLoption enumerated constant in commit
curl/curl@9f44a95, and the introduction
of the associated CURLUSESSL_TRY curl_usessl enumerated constant in commit
curl/curl@3fa6016.

Both of these constants first appeared in curl v7.17.0, and replaced the
earlier CURLOPT_FTP_SSL CURLoption enumerated constant and CURLFTPSSL_TRY
curl_ftpssl enumerated constant, which were retained but were now defined
using macros that mapped those names to the corresponding new identifiers:

  https://github.com/curl/curl/blob/curl-7_17_0/include/curl/curl.h#L1140
  https://github.com/curl/curl/blob/curl-7_17_0/include/curl/curl.h#L510

Thus the intention of the macro block added to Git's http.h source file
in Git v1.8.3 was to check whether the new, generic CURLOPT_USE_SSL
option and associated CURLUSESSL_TRY curl_usessl enumerated constant
were defined, and they were not, define them in terms of their older
FTP-specific counterparts, if those are available.

However, curl has never defined identifiers such as CURLOPT_FTP_SSL as
macros.  Instead, macros named T(), CINIT(), and lately CURLOPT() have
been used to initialize the CURLOPT_* CURLoption enumerated constants.
See, for example:

  https://github.com/curl/curl/blob/curl-7_2/include/curl/curl.h#L172-L175
  https://github.com/curl/curl/blob/curl-7_17_0/include/curl/curl.h#L573-L582
  https://github.com/curl/curl/blob/curl-7_69_0/include/curl/curl.h#L948

These macros generate single enumerated constant assignments of the form:

  CURLOPT_USE_SSL = CURLOPTTYPE_LONG + 119

In other words, CURLOPT_USE_SSL has never been the name of a preprocessor
macro, and CURLOPT_FTP_SSL was not either until curl v7.17.0 when it
was redefined as a macro with the value CURLOPT_USE_SSL.

Thus the first condition in the conditional statement of Git's
macro block, namely !defined(CURLOPT_USE_SSL), has presumably never
had its intended effect because CURLOPT_USE_SSL has never been the name
of a macro.  The defined() macro operator only evaluates to true if the
name it is passed is defined as a macro, not as a regular C language
identifier.  Thus the expression !defined(CURLOPT_USE_SSL) would always
be true, since CURLOPT_USE_SSL has never been defined as a macro.

This means that other condition in Git's conditional statement,
defined(CURLOPT_FTP_SSL), exclusively controlled whether the block
was processed or not.  As mentioned above, since v7.17.0 curl has
defined CURLOPT_FTP_SSL as a macro, not an enumerated constant, so
the conditional would evaluate to true and the macro definitions in
the block would be processed.

Hence with curl v7.17.0 or above, Git would define the macros
CURLOPT_USE_SSL and CURLUSESSL_TRY with the values CURLOPT_FTP_SSL
and CURLFTPSSL_TRY, and the preprocessor would expand those names
to those values.  But because those values match macros defined by
curl, the preprocessor would expand them again, producing the values
CURLOPT_USE_SSL and CURLUSESSL_TRY, which are of course identical to
the names Git initially defined as macros.

However, the C preprocessor does not expand macros it has expanded
previously, to avoid infinite loops of expansions.  Per the GCC manual:

  "If a macro x expands to use a macro y, and the expansion of y refers
   to the macro x, that is an indirect self-reference of x.  x is not
   expanded in this case either."

  https://gcc.gnu.org/onlinedocs/cpp/Self-Referential-Macros.html

Fortunately, both CURLOPT_USE_SSL and CURLUSESSL_TRY are defined by
curl (since v7.17.0) as enumerated constants for the CURLoption
and curl_usessl types, respectively:

  https://github.com/curl/curl/blob/curl-7_17_0/include/curl/curl.h#L1008
  https://github.com/curl/curl/blob/curl-7_17_0/include/curl/curl.h#L497

So long as these names were ultimately defined by curl as non-macro
identifiers, the compilation of older versions of Git that defined the
macros CURLOPT_USE_SSL and CURLUSESSL_TRY would still succeed.

This situation has changed with curl v8.13.0 because commit
curl/curl@7b0240c replaced the
CURLUSESSL_* enumerated constants, including CURLUSESSL_TRY, with
macro definitions, for the reasons outlined in curl/curl#16539 and
curl/curl#16482:

  https://github.com/curl/curl/blob/curl-8_13_0/include/curl/curl.h#L931

The consequence for our "build-earliest" CI job is that with curl updated
to v8.13.0, as is now the case for the Homebrew version of curl installed
on macOS GitHub Actions runners, there are two conflicting definitions
of the CURLUSESSL_TRY macro.  The first definition now appears in curl's
header file, and as described above, older versions of Git provide
another definition in a macro block in the http.h source file, and
that block is processed because its conditional evaluates to true.

The duplicate definitions of the CURLUSESSL_TRY macro only produce
compiler warnings, in and of themselves.  However, Git's definition
supersedes curl's, and so the preprocessor first expands CURLUSESSL_TRY
to CURLFTPSSL_TRY (per Git's definition) and then expands CURLFTPSSL_TRY
back to CURLUSESSL_TRY (per curl's definition of CURLFTPSSL_TRY).
At this point, preprocessing stops, at which point the compiler expects
to find a C identifier with the name CURLUSESSL_TRY.  When it finds none,
an error results.

(Note that newer versions of Git do not define the CURLOPT_USE_SSL
and CURLUSESSL_TRY macros at all, so our other CI jobs are not affected
by the changes in curl v8.13.0.  The conditional block which defined
these macros was removed from Git's http.h source file in commit
git/git@5db9d38 of Git v2.34.0.)

To resolve the compiler error without raising our minimum supported and
tested version of Git from 2.0.0 all the way to 2.34.0, we instead
update our script/build-git script to patch older versions of Git's
http.h source file and comment out the conditional block which defined
the CURLOPT_USE_SSL and CURLUSESSL_TRY macros.  Since all modern
versions of curl define these identifiers, there is no need for Git to
define them, and as noted above, Git versions since 2.34.0 do not do so.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous Integration cmdline tool libcurl API

Development

Successfully merging this pull request may close these issues.

5 participants