-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
libcurl: add version 8.4.0 (fix CVE-2023-38545) #19769
Conversation
🤖 Beep Boop! This pull request is making changes to 'recipes/libcurl//'. 👋 @Hopobcn you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 need to ask for context on the linked issue and ask where things are regarding it before approving, thanks for brining it to my attention :)
I detected other pull requests that are modifying libcurl/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
@RubenRBS
|
Huh, that's interesting. I'm getting that same error for libjpeg with MSVC in #19298. I wonder if it's related in any way. |
@toge sorry for the late response, didn't see this ping until now Where does the error come from? A previous build? the current logs seem all to be passing |
@RubenRBS To avoid it, I add small patch to dissable HAVE_SSL_SET0_WBIO. |
I noticed that build fails locally for me with the following configuration (buildbot cancelled execution before attempting them): The error message is:
There is actually no .lib-file in the build folder at all. In contrast to that, I tested the libcurl upstream cmake build for 8.4.0 as described in https://github.com/curl/curl/blob/master/docs/INSTALL.cmake and it builds the .lib-file as expected. Diffing the full linker command line in the conan recipe build vs. upstream build showed that the linker option /DEF that is missing in case of conan recipe build. /DEF is used to pass a .def-file with EXPORTS statements, which triggers the linker to create the import library. I suspect this is related to the following change in libcurl release 8.4.0: curl/curl#11914 In the conan recipe, we prevent this from working correctly due to a patch that removes the "CurlSymbolHiding"-logic from the upstream CMakeLists.txt: Suggested fix: Remove this patch in conanfile.py. Its marked as "suspicious" anyway ;-) |
@dietssa |
This comment has been minimized.
This comment has been minimized.
FWIW, I also had to set |
This PR will close #20529. |
@toge @RubenRBS Hi, is this pr ready to merge? |
Conan v1 pipeline ✔️All green in build 9 (
Conan v2 pipeline ✔️
All green in build 9 (
|
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.
Checked the build log https://c3i.jfrog.io/c3i/misc/logs/pr/19769/7-linux-gcc/libcurl/8.4.0//f13ed95cd7c05bdadafca9cb0e33c7e261bae784-build.txt and it looks okay according these new changes.
@@ -303,7 +303,7 @@ def _patch_autotools(self): | |||
"AC_CHECK_LIB(z,", | |||
f"AC_CHECK_LIB({zlib_name},") | |||
replace_in_file(self, configure_ac, | |||
"-lz ", | |||
"-lz", |
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.
This change may break with_zstd=True
: https://github.com/curl/curl/blob/curl-8_4_0/configure.ac#L1512. There was a good reason for this extra space in pattern.
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.
Is there an upstream proposal to add the configure script option?
Specify library name and version: libcurl/*