-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
NONJAVACLI-3460: update third party dependencies #4690
Conversation
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.
Thanks a lot for the PR!
The windows versions also require an update. That can be done for zlib, curl, and zstd with updation of vcpkg.json.
src/lz4.h
Outdated
@@ -100,7 +100,7 @@ extern "C" { | |||
/*------ Version ------*/ | |||
#define LZ4_VERSION_MAJOR 1 /* for breaking interface changes */ | |||
#define LZ4_VERSION_MINOR 9 /* for new (non-breaking) interface capabilities */ | |||
#define LZ4_VERSION_RELEASE 3 /* for tweaks, bug-fixes, or development */ | |||
#define LZ4_VERSION_RELEASE 4 /* for tweaks, bug-fixes, or development */ |
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.
upgrading lz4 is trickier, as we have actually copied the entire code into our source tree. An update will look similar to https://github.com/confluentinc/librdkafka/pull/3148/files
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 was under the impression that most of the update has already happened, it was just the version update missing:
#4232
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.
oh, it was just the CVE fix,
maybe it would be worthwhile to update lz4 as there are some significant performance gains for ARM64 platform:
https://github.com/lz4/lz4/releases
@@ -45,8 +45,8 @@ void foo (void) { | |||
function install_source { | |||
local name=$1 | |||
local destdir=$2 | |||
local ver=7.86.0 | |||
local checksum="3dfdd39ba95e18847965cd3051ea6d22586609d9011d91df7bc5521288987a82" | |||
local ver=8.7.1 |
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.
Adding a note for the team here that curl 8 has no API or ABI changes
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.
@milindl
Thank you. Can you please see this commit through completion - this is as far as I can go with it.
/sem-approve |
Running CI, and then I'll run try running OIDC tests locally to test out the libcurl update. |
@milindl |
I can bump it here, we're changing things anyway. However, openssl bumps are partial, linux only, because the latest versions of openssl 3.0.x are not available on vcpkg. |
Closing and reopening this to try and make sem-approve work. |
/sem-approve |
In the latest changes (which include openssl bump):
|
resolves ( with downstream propagation) |
replacing the branch name to dev_janjwerner_deps_update per @milindl suggestion |
No description provided.