forked from git-lfs/git-lfs
-
Notifications
You must be signed in to change notification settings - Fork 2
Commit 4804673
committed
script/build-git: hide conflicting curl macros
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, specifically, Git v2.0.0. We compile
Git against the version of curl supplied by Homebrew on the macOS
GitHub Actions runners.
Recently, this version of curl has been updated to v8.13.0, and
our "build-earliest" job has started to fail as a result, because
the C compiler reports that the CURLUSESSL_TRY identifier is not
declared when it is used in a call to curl_easy_setopt() in the
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 was in use, this was always
the case, but not because curl defined that macro.
Instead, between those same versions of Git, the CURLOPT_USE_SSL macro
was defined by Git itself, along with the CURLUSESSL_TRY macro which
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 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 commit specifically refers to the introduction 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 in the http.h source file of
the Git project 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 the 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 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 is not 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 is not the name of a defined 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. Because these values are also macro names, as
defined by curl, the preprocessor would substitute those macro's
values, namely CURLOPT_USE_SSL and CURLUSESSL_TRY, i.e., the names
Git initially defined as macros.
However, the preprocessor does not expand macros it has expanded
previously, to avoid infinite 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:
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 with macros
defining 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:
https://github.com/curl/curl/blob/curl-8_13_0/include/curl/curl.h#L931
The consequence for our "build-earliest" CI job is if curl is 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, as per the reference above, and the second definition
is in the conditional macro block in Git's http.h source file, which
as explained above is always processed because CURLOPT_USE_SSL is still
not defined as a macro and CURLOPT_FTP_SSL is defined as a macro.
This in and of itself only produces compiler warnings; however, because
Git redefines CURLUSESSL_TRY as CURLFTPSSL_TRY, which is a macro defined
by curl that expands back to CURLUSESSL_TRY, the compiler eventually looks
for an actual C identifier with that name and finds none, leading to
a compilation error.
(Note that newer versions of Git do not define these macros at all, so
our other CI jobs are not affected. The conditional block in http.h
which defined these macros was removed in commit
git/git@5db9d38 of Git v2.34.0.)
To resolve this problem without raising our minimum supported and
tested version of Git from 2.0.0 all the way to 2.34.0, we instead
simply update our script/build-git script to patch older versions of
the http.h Git source file and comment out the conditional block which
defined the CURLOPT_USE_SSL and CURLUSESSL_TRY macros. Since these are
properly defined by all modern versions of curl, there is no need to try
to define them, and as noted above, Git since v2.34.0 does not do so.1 parent 9b9bccb commit 4804673Copy full SHA for 4804673
File tree
Expand file treeCollapse file tree
1 file changed
+23
-0
lines changedFilter options
- script
Expand file treeCollapse file tree
1 file changed
+23
-0
lines changed+23Lines changed: 23 additions & 0 deletions
Original file line number | Diff line number | Diff line change | |
---|---|---|---|
| |||
22 | 22 |
| |
23 | 23 |
| |
24 | 24 |
| |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
25 | 48 |
| |
26 | 49 |
| |
27 | 50 |
| |
|
0 commit comments