Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/zstd vendor does not find system zstd #1111

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Fix/zstd vendor does not find system zstd #1111

merged 2 commits into from
Oct 6, 2022

Conversation

DasRoteSkelett
Copy link
Contributor

Closes: #1110

by the way, what a cool issue id to have :-)

@DasRoteSkelett DasRoteSkelett requested a review from a team as a code owner October 2, 2022 14:26
@DasRoteSkelett DasRoteSkelett requested review from emersonknapp and hidmic and removed request for a team October 2, 2022 14:26
clalancette
clalancette requested changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it out, and it is indeed the case that we don't detect the built-in zstd. So thanks for this.

I left a few things inline that should clean this up more. Once that is in, we can run CI. I'd also like to get @emersonknapp 's opinion before merging.

zstd_vendor/CMakeLists.txt Outdated Show resolved Hide resolved
zstd_vendor/CMakeLists.txt Outdated Show resolved Hide resolved
zstd_vendor/CMakeLists.txt Show resolved Hide resolved
zstd_vendor/cmake/Modules/Findzstd.cmake Outdated Show resolved Hide resolved
@emersonknapp emersonknapp changed the title Fix/zstd vendoor does not find system zstd Fix/zstd vendor does not find system zstd Oct 3, 2022
@emersonknapp
Copy link
Collaborator

Thanks @clalancette for the careful pass. Like you, I think this looks mostly fine, handling the little nitpicks puts it in good shape to move forward.

@DasRoteSkelett DasRoteSkelett force-pushed the fix/zstd_vendoorDoesNotFindSystemZstd branch from 6ff5d94 to 3029357 Compare October 4, 2022 06:35
@DasRoteSkelett
Copy link
Contributor Author

Thanks for the review @clalancette and @emersonknapp . I applied most suggestions, except that the version check is done with find_package, which i believe is the more standard way. I documented it a little better and hope you are okay with it

clalancette
clalancette approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now, @emersonknapp can you take another look?

emersonknapp
emersonknapp approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup lgtm - this needs a rebase on rolling branch unfortunately for the Action CI to pass, there was a new package added - @DasRoteSkelett could you do that? Then I'll run CI on the buildfarm

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

@emersonknapp Oops, I missed your edit. You are right, this needs a rebase. I'll let you take it from here and run CI.

Matthias Schoepfer added 2 commits October 5, 2022 10:59
zstd_vendor: CMakeLists fix to find Module
92e277d 
cmake did not find the Findzstd.cmake in cmake/Modules. This is
at least ture for yocto, but I could also reproduce with gentoo and
ubuntu. The error you get when you remove the `QUIET` keyword in
`find_package` will give this error message:

```
CMake Warning at CMakeLists.txt:12 (find_package):
  By not providing "Findzstd.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "zstd", but
  CMake did not find one.

  Could not find a package configuration file provided by "zstd" with any of
  the following names:

    zstdConfig.cmake
    zstd-config.cmake

  Add the installation prefix of "zstd" to CMAKE_PREFIX_PATH or set
  "zstd_DIR" to a directory containing one of the above files.  If "zstd"
  provides a separate development package or SDK, be sure it has been
  installed.
```

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
zstd_vendor: Fix Findzstd.cmake to properly respect version
efa5c8f 
Even with the Findzstd.cmake being found, the zstd_VERSION is not
set in Findzstd.cmake, so that the version check always failes even
though a recent version of zstd is available in the system.

The version check is implemented in the Findzstd.cmake and done
with find_package(<library> <version>) and
find_package_handle_standard_args.

Since we cannot use pkg-config for some Windows issues, the parsing of
the version is done by looking for the string in zstd.h.

This unfortunately makes this patch a little verbose.

Tested with ubuntu, gentoo, yocto.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
@DasRoteSkelett DasRoteSkelett force-pushed the fix/zstd_vendoorDoesNotFindSystemZstd branch from 3029357 to efa5c8f Compare October 5, 2022 09:00
@DasRoteSkelett
Copy link
Contributor Author

@emersonknapp @clalancette Thanks, I did the rebase. Guess now you need to do your magic
image

@emersonknapp
Copy link
Collaborator

Gist: https://gist.githubusercontent.com/emersonknapp/2a2db76258857598c12a0e6d5e6955fe/raw/a9cedca1f7f5d06066eb408baed55ab445d2fde6/ros2.repos
BUILD args: --packages-above-and-dependencies zstd_vendor
TEST args: --packages-above zstd_vendor
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10952

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp emersonknapp merged commit e7e7269 into ros2:rolling Oct 6, 2022
@DasRoteSkelett DasRoteSkelett deleted the fix/zstd_vendoorDoesNotFindSystemZstd branch October 6, 2022 15:07
@clalancette clalancette mentioned this pull request Jan 31, 2024
Closed
@clalancette
Copy link
Contributor

@Mergifyio backport humble

@mergify Mergify
Copy link

mergify bot commented Jan 31, 2024 •
edited
Loading

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 31, 2024
@DasRoteSkelett @mergify
Fix/zstd vendor does not find system zstd (#1111)
8bd646f 
cmake did not find the Findzstd.cmake in cmake/Modules

Since we cannot use pkg-config for some Windows issues, the parsing of
the version is done by looking for the string in zstd.h.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
(cherry picked from commit e7e7269)
@mergify mergify bot mentioned this pull request Jan 31, 2024
Merged
clalancette pushed a commit that referenced this pull request Feb 1, 2024
@mergify @DasRoteSkelett
Fix/zstd vendor does not find system zstd (#1111) (#1560)
411da91 
cmake did not find the Findzstd.cmake in cmake/Modules

Since we cannot use pkg-config for some Windows issues, the parsing of
the version is done by looking for the string in zstd.h.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
(cherry picked from commit e7e7269)

Co-authored-by: DasRoteSkelett <matthias.schoepfer@googlemail.com>
emersonknapp added a commit that referenced this pull request Jun 10, 2024
@emersonknapp @mergify
@ahcorde @MichaelOrlov @fujitatomoya @christophfroehlich @audrow @arneboe @DasRoteSkelett @clalancette @Barry-Xu-2018 @r7vme
Merge latest from Humble upstream (#25)
3d05f7d 
* Link and compile against rosbag2_storage_mcap: Fixed issue 1492 (#1496) (#1498)

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 7fcb703)

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* [humble] Bugfix for incorrect playback rate changes when pressing buttons (backport #1513) (#1515)

* Bugfix for incorrect playback rate changes when pressing buttons (#1513)

- Playback rate expected to be changed by 10% with each
increase/decrease step.
- Use +0.1 and -0.1 in decrease/increase rate formula instead of
multiply by factor of the 1.1 and 0.9 respectively.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 95f78b6)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/player.cpp

* Address merge conflicts after auto-backporting

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* call cv.wait_until only if necessary. (#1521) (#1523)

* call cv.wait_until only if necessary.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* add comment to avoid extra delay for performance.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

---------

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit a16704b)

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* [humble] Install signal handlers in recorder only inside record method (backport #1464) (#1526)

* Install signal handlers in recorder only inside record method (#1464)

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit 195e406)

# Conflicts:
#	rosbag2_py/src/rosbag2_py/_transport.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] `Recording stopped` prints only once. (backport #1530) (#1535)

* `Recording stopped` prints only once. (#1530)

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 73b0772)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/recorder.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Give proper log message for `--start-paused` (backport #1537) (#1541)

* Add proper message for --start-paused (#1537)

Signed-off-by: Christoph Froehlich <christoph.froehlich@ait.ac.at>
(cherry picked from commit 317286c)

# Conflicts:
#	rosbag2_transport/src/rosbag2_transport/recorder.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* 0.15.9 (#1551)

* 0.15.9

Signed-off-by: Audrow Nash <audrow@intrinsic.ai>

* Update rosbag2_transport/CHANGELOG.rst

Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>

---------

Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Add default initialization for CompressionOptions (backport #1539) (#1546)

* Add default initialization for CompressionOptions (#1539)

* feat: add sane defaults for CompressionOptions

Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>

* Update rosbag2_compression/include/rosbag2_compression/compression_options.hpp

Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>

---------

Signed-off-by: Arne Böckmann <a.boeckmann@cellumation.com>
Co-authored-by: Arne Böckmann <a.boeckmann@cellumation.com>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 931bf54)

# Conflicts:
#	rosbag2_compression/include/rosbag2_compression/compression_options.hpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Arne B <arne@rnae.de>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* Fix/zstd vendor does not find system zstd (#1111) (#1560)

cmake did not find the Findzstd.cmake in cmake/Modules

Since we cannot use pkg-config for some Windows issues, the parsing of
the version is done by looking for the string in zstd.h.

Signed-off-by: Matthias Schoepfer <m.schoepfer@rethinkrobotics.com>
(cherry picked from commit e7e7269)

Co-authored-by: DasRoteSkelett <matthias.schoepfer@googlemail.com>

* [humble] Use rw_lock to protect mcap metadata lists. (backport #1561) (#1567)

* Use rw_lock to protect mcap metadata lists. (#1561)

* use rw_lock to protect mcap metadata lists.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* introduce MCAPStorage::write_lock_free private method.

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

---------

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
(cherry picked from commit 90d1da8)

# Conflicts:
#	rosbag2_storage_mcap/src/mcap_storage.cpp

* Resolve merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* Add /bigobj to MSVC compiles. (#1571)

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

* Fix split by time. (backport #1022) (#1616)

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>

* [humble] Add BagSplitInfo service call on bag close (backport #1422) (#1637)

* Add BagSplitInfo service call on bag close (#1422)

- Note: The `BagSplitInfo::opened_file` will have empty string to
indicate that it was "bag close" and not bag split event.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
(cherry picked from commit ba199d0)

# Conflicts:
#	rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp

* Fix merge conflicts

- Ensure that writer_ is destructed before intercepted fake_metadata_

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [Humble] Resolve recording option problem (backport #1649) (#1651)

* Resolve recording option problem (#1649)

Signed-off-by: Barry Xu <barry.xu@sony.com>
(cherry picked from commit 4914ab3)

# Conflicts:
#	ros2bag/ros2bag/verb/record.py

* Fix cherry-pick conflicts for mergify/bp/humble/pr-1649 (#1652)

Signed-off-by: Barry Xu <barry.xu@sony.com>

---------

Signed-off-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Barry Xu <barry.xu@sony.com>

* [humble] Add --log-level to ros2 bag play and record (#1655)

* Add --log-level to ros2 bag play and record

Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

* Fix missing import

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>

---------

Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

* [humble] Bugfix for writer not being able to open again after closing (backport #1599) (#1653)

* [iron] Bugfix for writer not being able to open again after closing (backport #1599) (#1635)

* re-applies fixes from #1590 to rolling. Also removes new message definition in sequential writer test for multiple open operations. Also clears topic_names_to_message_definitions_ and handles message_definitions_s underlying container similarly. Lastly, also avoids reset of factory in the compression writer, adds unit test there too.

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* removes unused compressor_ member from compresser writer class. Also delegates rest of the closing behavior to the base class in close method, as it is handled in the open and write methods of the compression writer

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>

* Remove unrelated delta

- message_definitions_ was intentionally allocated on the stack and
should persist between writer close() and open() because it represents
cache for message definitions which is not changes.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Don't call virtual methods from destructors

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Cleanup 'rosbag2_directory_next' after the test run

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Protect Writer::open(..) and Writer::close() with mutex on upper level

- Rationale: To avoid race conditions if open(..) and close() could be
ever be called from different threads.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Bugfix for WRITE_SPLIT callback not called for the last compressed file

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Bugfix for lost messages from cache when closing compression writer

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Address build failure by using rcpputils::fs instead of std::filesystem

- Note: On Iron we haven't migrated to the std::filesystem and using
rcpputils::fs

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Adopt failing 'open_succeeds_twice' test for Iron

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Return from writer's open() immediately if storage already open

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Yannick Schulz <yschulz854@gmail.com>
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Yannick Schulz <yschulz854@gmail.com>
(cherry picked from commit a360d9b)

# Conflicts:
#	rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
#	rosbag2_cpp/src/rosbag2_cpp/writer.cpp
#	rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

* Address merge conflicts

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix for segfault in open_twice test

- Ensure that writer_ is destructed before intercepted fake_metadata_

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Fix for "open_succeeds_twice" test failure on second run

- Use std::filesystem for temp files and folders operation. For some
reason rcpputils::fs::delete_all(folder_name) wasn't able to delete temp
folder with subfolders.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Adopt changes in TestRosbag2CPPAPI::minimal_writer_example for humble

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test. This is not a bug - this is by design.

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Signed-off-by: Audrow Nash <audrow@intrinsic.ai>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Roman Sokolkov <rsokolkov@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
Co-authored-by: Audrow Nash <audrow@openrobotics.org>
Co-authored-by: Arne B <arne@rnae.de>
Co-authored-by: DasRoteSkelett <matthias.schoepfer@googlemail.com>
Co-authored-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Barry Xu <barry.xu@sony.com>
Co-authored-by: Roman <rsokolkov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@clalancette clalancette clalancette approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner automatically assigned from ros2/tooling-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zstd_vendor does not find system zstd and will always build and install
3 participants
@DasRoteSkelett @emersonknapp @clalancette