-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Prevent conflicting curl macros in early Git version CI jobs #6040
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
Prevent conflicting curl macros in early Git version CI jobs #6040
Conversation
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.
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.
I don't like this solution but I cannot suggest something better either.
I think we should go with your suggestion and bump the minimum Git version to 2.34.0 once Debian 11 gets EOL (which is August 14, 2024).
Should we still support Debian 11 even though it is EOL already?
Yes, I think we've committed to matching the LTS (Long-Term Support) EOL date, which will be next August of next year, and also those of the Ubuntu and Linux Mint distributions based on Debian 11, which are supported through April 2027. Our distribution list for Debian 11 has those dates in the comments. However, Debian 10 and RHEL 7/CentOS 7 are distributions we can definitely drop before v3.7.0! |
This PR resolves a problem which has recently caused our
build-earliestCI job to fail on macOS GitHubActions runners when we try to compile Git v2.0.0.Specifically, the compiler reports
error: use of undeclared identifier 'CURLUSESSL_TRY'in the call to thecurl_easy_setopt()function in Git'sget_curl_handle()function, whereCURLUSESSL_TRYis passed as the final argument.As described below, and explicated in further detail in the description of this PR's sole commit, the error occurs because older versions of Git contain a macro whose definition of
CURLUSESSL_TRYoverrides the definition now provided by the curl library, and the result is that the macro expands back to the same name, which is nowhere defined as an actual C identifier and not as a preprocessor macro.To resolve the compiler error we update our
script/build-gitscript to patch older versions of Git'shttp.hsource file and comment out the conditional block which defined theCURLOPT_USE_SSLandCURLUSESSL_TRYmacros. Since all modern versions of curl define these identifiers, there is no need for Git to define them, and Git versions since 2.34.0 do not do so.(We may in the future consider raising the minimum version of Git we support and test against to v2.34.0 or higher, at which point we will no longer need to patch the
http.hsource file. However, we still build packages for Linux distributions with versions of Git older than 2.34.0; for instance, Debian 11 packages Git v2.30.2 with a number of back-ported security patches. Therefore we should for now continue to test with relatively old versions of Git, and as such, will need to patch the Git source to allow ourbuild-earliestCI jobs to succeed.)Background
The error seen in our CI jobs occurs on our macOS runners because the version of the curl library installed by Homebrew has just been upgraded to v8.13.0, and that version includes the changes from commit curl/curl@7b0240c, where the
CURLUSESSL_*enumerated constants, includingCURLUSESSL_TRY, were replaced with macro definitions, for the reasons outlined in curl/curl#16539 and curl/curl#16482.With recent versions of Git, this change has no effect. However, versions of Git between 1.8.3 and 2.33.8 contained a macro definition block which defines
CURLUSESSL_TRYwith the valueCURLFTPSSL_TRY. (This block was added in commit git/git@4bc444e with the intention that it would allow Git to be compiled against versions of curl prior to v7.17.0, which did not define theCURLOPT_USE_SSLandCURLUSESSL_TRYidentifiers, although that may not ever have actually worked as intended.)The curl library, meanwhile, provides a macro definition of
CURLFTPSSL_TRYwhich expands back toCURLUSESSL_TRY.When our
build-earliestCI job tries to compile Git v2.0.0 using curl v8.13.0, these duplicate definitions produce some compiler warnings. They also cause an error, because Git's definition supersedes curl's, so the C preprocessor first expandsCURLUSESSL_TRYtoCURLFTPSSL_TRY(per Git's definition) and then expandsCURLFTPSSL_TRYback toCURLUSESSL_TRY(per curl's definition ofCURLFTPSSL_TRY). At this point, preprocessing stops, at which point the compiler expects to find a C identifier with the nameCURLUSESSL_TRY. When it finds none, an error results.(Note that newer versions of Git do not define the
CURLOPT_USE_SSLandCURLUSESSL_TRYmacros 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 in commit git/git@5db9d38 of Git v2.34.0.)The description of this PR's single commit provides additional information and context on the history of these macro definitions in the Git and curl source code, and some speculation as to whether the conditional governing Git's macro definitions ever worked as intended.