Externals: Update libcurl #11853
Externals: Update libcurl #11853
Conversation
AdmiralCurtiss
force-pushed
the
update-curl
branch
from
f26a80a
to
b74eb5c
Compare
|
CMake with our External is still broken but I think this should fix the MSVC project? |
|
What's going on with the broken builds here? |
|
There is no working CMake for the curl External currently. |
|
@dolphin-emu-bot rebuild |
|
I don't think using a pre-configured config header file for every platform is a good idea. For the Windows VS project, yeah sure, but for everything else it should just generate it. I'm also questioning just globbing for all the *.c files in the external, but that might be okay depending on how curl is set up, I'd have to check... |
|
What exactly do we need from curl anyway, just HTTP/HTTPS? |
AdmiralCurtiss
force-pushed
the
update-curl
branch
from
6dd3110
to
9ff4a32
Compare
|
Pretty sure that is it yes. |
|
I give up, this entire thing refuses to work properly (on Windows, anyway). Using the CMakeLists directly doesn't work (it fails to detect various headers and symbols that should be there), using FetchContent() doesn't work (it fails to link against our other externals), using a preconfigured header doesn't work (this actually builds but all the HTTPS calls fail at runtime). If you want to figure this out be my guest, but I'm getting too frustrated with this. I genuinely think it might just be easier to use a different library that doesn't have a million settings instead. Here's my current state if you want to mess with it: https://github.com/AdmiralCurtiss/dolphin/tree/curl812 |
AdmiralCurtiss
force-pushed
the
update-curl
branch
from
9ff4a32
to
584a30f
Compare
|
The hacky solution of just not using a config header on Windows seems to work, which makes no sense to me and I still hate the preconfigured header for the other platforms, but I guess I'll just hope that they use their system curl instead... |
| list(APPEND CURL_LIBS "-framework SystemConfiguration") | ||
| endif() | ||
|
|
||
| file(GLOB SRCS curl/lib/*.c curl/lib/vauth/*.c curl/lib/vquic/*.c curl/lib/vssh/*.c curl/lib/vtls/*.c) |
There was a problem hiding this comment.
Globbing is generally frowned upon but should we consider using CONFIGURE_DEPENDS for this?
There was a problem hiding this comment.
The reason you don't usually glob for sources is because then CMake can't know when new files are added or files are deleted. That's not really a concern for an external we update once in a blue moon, as long as we also change any CMakeLists in the same commit that we update the external.
There was a problem hiding this comment.
Yeah, I was aware why, I didn't know if we wanted to handle changes from the glob. This should be relatively rare, just thought I'd bring it up as I wasn't sure if it had been discussed before.
Externals/curl/CMakeLists.txt
Outdated
| if (WIN32) | ||
| target_compile_definitions(curl PUBLIC "BUILDING_LIBCURL=1" "CURL_STATICLIB=1" "CURL_DISABLE_LDAP" "USE_WINDOWS_SSPI" "USE_SCHANNEL") | ||
| target_link_libraries(curl Crypt32.lib) | ||
| else() | ||
| target_compile_definitions(curl PUBLIC "BUILDING_LIBCURL=1" "CURL_STATICLIB=1" "USE_MBEDTLS=1" "HAVE_CONFIG_H=1" "CURL_DISABLE_LDAP") | ||
| if (CURL_CA_PATH_SET) | ||
| target_compile_definitions(curl PUBLIC CURL_CA_PATH="${CURL_CA_PATH}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
I would assume BUILDING_LIBCURL=1 should be PRIVATE?
There was a problem hiding this comment.
Yes, I agree, that should be private.
noahpistilli
force-pushed
the
update-curl
branch
2 times, most recently
from
30f4d01
to
3a4e4f1
Compare
noahpistilli
force-pushed
the
update-curl
branch
from
3a4e4f1
to
df6e89a
Compare
|
Alright, I already said this on Discord, but for posterity. I think this is terrible. A library as big and complicated as curl should absolutely not use a single preconfigured header for every platform we build on. However, I also acknowledge that I have no idea how to get CMake to build the correct config header in our setup -- I've tried and failed myself. I also realize that the previous version of curl we're updating from has done the exact same thing. Given that, and that this update is required in order to make some missing WiiConnect24 functionality work, I am okay with merging this and leaving it as-is as long as no other maintainer has any issues with it. Ideally we can eventually switch to a better system for dependency management ( Please fix the |
noahpistilli
force-pushed
the
update-curl
branch
from
df6e89a
to
6f55e44
Compare
|
Well, no one spoke up against this, so let's hope this doesn't break compiling on half the linux distros... |
|
@AdmiralCurtiss any chance for an update to 8.4.0? https://daniel.haxx.se/blog/2023/10/11/how-i-made-a-heap-overflow-in-curl/ |
|
I'm not sure if that's even exploitable with how we use curl in Dolphin but can't hurt, I suppose. |
#11746 is depends on this because one of the mail endpoints requires us to send MIME data, which the current version of libcurl in Externals does not support.