-
Notifications
You must be signed in to change notification settings - Fork 244
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
Updated zstd to 1.5.5 #1617
Updated zstd to 1.5.5 #1617
Conversation
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
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.
Looks good to me with green CI.
Let's put this on the queue to merge after the freeze is lifted.
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 would prefer to suppress such warnings via compiler flags for vendor package rather than patching third-party library.
I will point out that this is essentially a backport of facebook/zstd#2838 . That said, I really don't feel strongly either way, so we can go with a change to the compiler flags rather than this patch if you prefer. |
@clalancette @ahcorde Please go with a change to the compiler flags rather than this patch. |
@clalancette With this patch 51924ed the warning is still there, do you know what I'm doing wrong? |
This is just a guess, but I believe that the zstd code is C, not C++. So you probably need to set CMAKE_C_FLAGS. |
I setted cd /root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor-build/lib && /usr/bin/clang -DXXH_NAMESPACE=ZSTD_ -DZSTD_LEGACY_SUPPORT=0 -DZSTD_MULTITHREAD -Dlibzstd_shared_EXPORTS -I/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor-build/lib -I/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/build/cmake/lib -I/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/build/cmake/../../lib -I/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/build/cmake/../../lib/common -Wno-unused-but-set-variable -std=c99 -Wall -Wextra -Wundef -Wshadow -Wcast-align -Wcast-qual -Wstrict-prototypes -O3 -DNDEBUG -fPIC -MD -MT lib/CMakeFiles/libzstd_shared.dir/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/lib/decompress/huf_decompress.c.o -MF CMakeFiles/libzstd_shared.dir/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/lib/decompress/huf_decompress.c.o.d -o CMakeFiles/libzstd_shared.dir/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/lib/decompress/huf_decompress.c.o -c /root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/lib/decompress/huf_decompress.c
/root/ros2_ws/build/zstd_vendor/zstd_vendor-prefix/src/zstd_vendor/lib/compress/zstd_compress_superblock.c:412:12: warning: variable 'litLengthSum' set but not used [-Wunused-but-set-variable]
412 | size_t litLengthSum = 0;
| ^ and the |
So I poked at this a bit. The problem is the way in which I discussed this with @MichaelOrlov and @ahcorde , and we came to agreement to go back to patching the source code, which is the most reliable thing to do here. That is, we can change this PR back to 772aaaa , and go from there. @ahcorde I'll leave this to you to do. |
d7ae34b
to
772aaaa
Compare
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.
Looks good to me with green CI.
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.
@ahcorde @clalancette I've just realized that instead of patching an old version of the zstd library we need to bump its version to the 1.5.5 which is going to be on noble https://packages.ubuntu.com/noble/zstd. And there this warning should already been fixed.
I've double checked that it is fixed on 1.5.5 version https://github.com/facebook/zstd/blob/63779c798237346c2b245c546c40b72a5a5913fe/lib/compress/zstd_compress_superblock.c#L138 |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@MichaelOrlov I updated to version 1.5.5 but there are two new warnings... |
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.
LGTM
@ros-pull-request-builder retest this please |
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
https://github.com/Mergifyio backport jazzy |
✅ Backports have been created
|
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> (cherry picked from commit cb90408)
Removed this warning