Add curl with curl_config.h #1943
Conversation
This reworks the curl BUILD file to use curl's cmake generated header file. This requires replacing all possible options, but makes sure we can't miss any important defines. For example in the previous build the CA cert chain of trust was invalid because the define for the system location for certs was not provided. Now we should no longer be able to miss settings like that when curl adds new ones.
| + "HAVE_ARPA_INET_H", | ||
| + "HAVE_ATOMIC", | ||
| + "HAVE_BASENAME", | ||
| + "HAVE_BOOL_T", |
There was a problem hiding this comment.
This patch breaks our project in compared to just using the default config.
ERROR: C:/users/zekew/_bazel_zekew/xrgzj2nk/external/ecsact_cli~/ecsact/cli/BUILD.bazel:28:10: Compiling ecsact/cli/ecsact_cli.cc failed: (Exit 2): cl.exe failed:
error executing CppCompile command (from target @@ecsact_cli~//ecsact/cli:ecsact) C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\cl.exe ... (remaining 1 argument skipped)
C:\Users\zekew\_bazel_zekew\xrgzj2nk\execroot\_main\external\curl~\lib\curl_setup_once.h(242): error C2628: '<unnamed-enum-bool_false>' followed by 'bool' is illegal (did you forget a ';'?)
C:\Users\zekew\_bazel_zekew\xrgzj2nk\execroot\_main\external\curl~\lib\curl_setup_once.h(242): warning C4091: 'typedef ': ignored on left of '<unnamed-enum-bool_false>' when no variable is declared
I think the default curl config for windows does a better job with some of these defines
https://github.com/curl/curl/blob/master/lib/config-win32.h#L113-L115
There was a problem hiding this comment.
yea that's kinda tough. I think i fixed this case but it's not ideal how the feature defines are tied to these underlying defines that we don't really care to override at all.
There was a problem hiding this comment.
can you test with the new fix?
There was a problem hiding this comment.
sorry for the delay - I was away
Unfortunately I get a different error now:
C:\Users\zekew\_bazel_zekew\ejyagchf\execroot\_main\external\curl~\lib\curl_setup_once.h(257): error C2061: syntax error: identifier 'bit'
C:\Users\zekew\_bazel_zekew\ejyagchf\execroot\_main\external\curl~\lib\curl_setup_once.h(257): error C2059: syntax error: ';'
tested against commit 47210a7a1f1ffbced4c0158b43f8162cb84ba27d
There was a problem hiding this comment.
ok how about now? I updated the PR to load the generated config, but then still allow the default configs to load too, which I think will always do a better job on these details that we don't really care about anyways
There was a problem hiding this comment.
same error. maybe some of the curl tests could be added to ensure the generated config works with the various platforms?
tested against aa70252a81fb24bc4f01e4953cabfb5639a354ce
There was a problem hiding this comment.
i dug a little further and I'm getting issues on ubuntu as well https://github.com/ecsact-dev/ecsact_cli/actions/runs/9026844616/job/24804753381?pr=93#step:4:85
using the fallback config like in #1917 seems to work though. I'd like to help where I can.
This reworks the curl BUILD file to use curl's cmake generated header
file. This requires replacing all possible options, but makes sure we
can't miss any important defines. For example in the previous build the
CA cert chain of trust was invalid because the define for the system
location for certs was not provided. Now we should no longer be able to
miss settings like that when curl adds new ones.