-
Notifications
You must be signed in to change notification settings - Fork 486
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
Replace deprecated tbb task for tbb >= 2021 #3174
Replace deprecated tbb task for tbb >= 2021 #3174
Conversation
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
As tbb::task has been removed in oneTBB 2021.01, we need to replace its use with oneapi::tbb::task_group. Define a wrapper so that tbb::task_group is used for newer versions of oneTBB. Fixes gazebosim#2867. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Suggested by @scpeters. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Suggested by @scpeters. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
In any case, this check wasn't actually halting the build in the case that tbb>=2021. Instead, find_package() is used (as it is if tbb.pc isn't present). Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
The CI is currently failing and this could be a possible problem.
Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
As tbb/version.h is seemingly not present before v2021.01, it cannot be used to get the definition for TBB_VERSION_MAJOR, so let's use the main tbb.h header which is present on all versions. Signed-off-by: Alex Dewar <alex.dewar@gmx.co.uk>
Marked as wip as there are still failures in the tbb==2021 build. |
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
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 for taking this on! I've tested it on macOS with homebrew and noticed some linking and compilation problems. I have an idea for some fixes:
I am seeing compilation failures of the test/integration/transport.cc
test and haven't figure out how to fix those
In file included from /Users/scpeters/clone/gazebo/test/integration/transport.cc:21:
In file included from /Users/scpeters/clone/gazebo/gazebo/test/ServerFixture.hh:44:
In file included from /Users/scpeters/clone/gazebo/build/gazebo/transport/transport.hh:3:
In file included from /Users/scpeters/clone/gazebo/gazebo/transport/Connection.hh:54:
In file included from /Users/scpeters/clone/gazebo/gazebo/transport/TaskGroup.hh:24:
In file included from /usr/local/include/tbb/tbb.h:17:
In file included from /usr/local/include/tbb/../oneapi/tbb.h:51:
In file included from /usr/local/include/oneapi/tbb/flow_graph.h:167:
In file included from /usr/local/include/tbb/../oneapi/tbb/detail/_flow_graph_impl.h:22:
/usr/local/include/oneapi/tbb/task_group.h:135:9: error: no matching function for call to object of type 'const gazebo::transport::PublishTask'
std::forward<F>(f)();
^~~~~~~~~~~~~~~~~~
/usr/local/include/oneapi/tbb/task_group.h:466:25: note: in instantiation of function template specialization 'tbb::detail::d2::(anonymous
namespace)::task_ptr_or_nullptr<const gazebo::transport::PublishTask &>' requested here
task* res = d2::task_ptr_or_nullptr(m_func);
^
/usr/local/include/oneapi/tbb/task_group.h:480:5: note: in instantiation of member function
'tbb::detail::d1::function_task<gazebo::transport::PublishTask>::execute' requested here
function_task(F&& f, wait_context& wo, small_object_allocator& alloc)
^
/usr/local/include/tbb/../oneapi/tbb/detail/_small_object_pool.h:63:57: note: in instantiation of member function
'tbb::detail::d1::function_task<gazebo::transport::PublishTask>::function_task' requested here
auto constructed_object = new(allocated_object) Type(std::forward<Args>(args)...);
^
/usr/local/include/oneapi/tbb/task_group.h:553:22: note: in instantiation of function template specialization
'tbb::detail::d1::small_object_allocator::new_object<tbb::detail::d1::function_task<gazebo::transport::PublishTask>, gazebo::transport::PublishTask,
tbb::detail::d1::wait_context &, tbb::detail::d1::small_object_allocator &>' requested here
return alloc.new_object<function_task<typename std::decay<F>::type>>(std::forward<F>(f), m_wait_ctx, alloc);
^
/usr/local/include/oneapi/tbb/task_group.h:629:16: note: in instantiation of function template specialization
'tbb::detail::d1::task_group_base::prepare_task<gazebo::transport::PublishTask>' requested here
spawn(*prepare_task(std::forward<F>(f)), context());
^
/Users/scpeters/clone/gazebo/gazebo/transport/TaskGroup.hh:41:25: note: in instantiation of function template specialization
'tbb::detail::d1::task_group::run<gazebo::transport::PublishTask>' requested here
this->taskGroup.run(Functor(std::forward<Args>(args)...));
^
/Users/scpeters/clone/gazebo/gazebo/transport/Node.hh:193:33: note: in instantiation of function template specialization
'gazebo::transport::TaskGroup::run<gazebo::transport::PublishTask, boost::shared_ptr<gazebo::transport::Publisher> &, const google::protobuf::Message &>'
requested here
this->taskGroup.run<PublishTask>(pub, _message);
^
/Users/scpeters/clone/gazebo/gazebo/transport/TransportIface.hh:146:13: note: in instantiation of function template specialization
'gazebo::transport::Node::Publish<gazebo::msgs::Scene>' requested here
node->Publish<M>(_topic, _message);
^
/Users/scpeters/clone/gazebo/test/integration/transport.cc:172:14: note: in instantiation of function template specialization
'gazebo::transport::publish<gazebo::msgs::Scene>' requested here
transport::publish<msgs::Scene>("~/scene", msg);
^
/Users/scpeters/clone/gazebo/gazebo/transport/Node.hh:75:20: note: candidate function not viable: 'this' argument has type
'const gazebo::transport::PublishTask', but method is not marked const
public: void operator()()
^
1 error generated.
make[3]: *** [test/integration/CMakeFiles/INTEGRATION_transport.dir/transport.cc.o] Error 1
Thanks @scpeters . By using the earlier patch by @alexdewar we just noticed some downstream linking failure: conda-forge/gazebo-feedstock#121 , so probably yet another thing to do is to add tbb libraries in |
(By the way @scpeters feel free to push directly on the branch of the PR) |
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
ok, I have done so :) I believe we will need to revert gazebo-tooling/release-tools#465 and gazebo-tooling/release-tools#466 and osrf/homebrew-simulation#1479 and osrf/homebrew-simulation#1483 before homebrew CI will use the new |
#include <tbb/task.h> | ||
#define emit |
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 a bit afraid of doing so as it changes also the case tbb<2021. Do you think that defining emit
as empty is ok?
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.
@traversaro emit
is actually defined by Qt as empty too! It's just convention that it's used in Qt code as a pseudo-keyword.
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.
Ahh, cool.
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 hadn't thought clearly about the implications; I was just trying to get to a working build. We can wrap in #ifdef
s if needed
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.
@scpeters Yes but that shouldn't be necessary. As I said, qt just defines it as an empty macro too.
Done in 73db7c7 . |
I have reached the same failure of @scpeters but on Linux/conda:
|
Not on my side, I just forgot to update the title. |
sorry for the delay in reviewing this. I just tested the latest changes and it appears to build well on my Mac laptop, which is great! I will need to adjust the build scripts to make the homebrew CI run properly, but that will be straightforward. The last issue is our windows CI, which is using a very old version of tbb (2014) has a strange error. If it's not obvious how to fix the error, we can try using a newer version of tbb |
Don't set tbb@2020_u3 variables if github branch name matches replace_deprecated_tbb_task, which allows testing of gazebosim/gazebo-classic#3174. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Don't set tbb@2020_u3 variables if github branch name matches replace_deprecated_tbb_task, which allows testing of gazebosim/gazebo-classic#3174. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
|
Probably not a short term solution as it like I can help in setting up a conda-based Windows CI of Gazebo. I think this was already planned by @j-rivero, but I do not know if he was planning to have it on GitHub Actions or on Jenkins (or anythinge else). If you like I can start sketching it on GitHub Actions, while if in on some other system probably we would need to coordinate a bit. |
that would be wonderful if you could start sketching an approach using GitHub Actions! A key feature of testing on our own machines is running tests that require a GPU, but we are not running many tests at the moment in these windows builds, so I think a GitHub Actions workflow that tests compilation and even limited testing would match our current CI. I think the ogre version might be a challenge depending on which version is in use from conda |
can you please merge this branch with |
Done in 4fff786 . |
Even after unpinning tbb-devel, the compilation works fine. |
The builds are failing due to an old version of tbb in the zipped dependency archive. See gazebosim/gazebo-classic#3174 for more information. Signed-off-by: Steve Peters <scpeters@openrobotics.org>
actually I may have a fix for the windows build in #3157 |
* [tbb] Update to 2021.3.0 * [tbb] Add Threads dependency * [pagmo2] Update to support new TBB * [openvdb] Update to 8.1.0 * [embree2] Remove from baseline -- it is no longer receiving support from upstream * [usd] Mark as unsupported * [usd] Disable USD in CI due to policy PixarAnimationStudios/OpenUSD#1600 * [openvdb] Bump port-version * Update version database * [pagmo2] Fix vcpkg.json * update version * wip update * versions * [embree2] deprecate, [openvdb,usd] resolve conflicts, [tbb] update * Added libxml port. * Remove port version as it's initial port. * Added baseline version * Support only for windows and static * Allowed building debug version * update versions * Update ports/libxpm/portfile.cmake Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com> * Added new lines * Update ports/libxpm/vcpkg.json Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com> * Update ports/libxpm/portfile.cmake Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com> * Update ports/libxpm/vcpkg.json Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com> * Regenerated versions * Update ports/libxpm/vcpkg.json Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com> * Libxpm is taken from gitlab now. * Dropped changes against master in original repo for libxpm. * Dropped libxpm from baseline * Dropped version for libxpm * Update cpuinfo * Updated date of version * Update version database * Removed support for arm32 & uwp as library is not supporting it. * Version regenerated * Update ports/cpuinfo/vcpkg.json Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com> * Updated versions * Initial commit WIP * Fixed compilation of latest stable version * Dropped comments * Switch from version-string to version * Added versions * Fixed path of cmake files for other projects * Updated versions * Various modernization. * Fully mark embree2 as deprecated. * [pagmo2] Disable -Werror * [cctag] Fix finding and use of TBB. * [openvdb] Baseline the static versions because they exceed FFFFFFFF in size, and I'm not sure if that's fixable.... * [gazebo] Apply gazebosim/gazebo-classic#3174 patch to fix TBB. * Respond to failures in https://dev.azure.com/vcpkg/public/_build/results?buildId=76586 * [pagmo2] Add license. See https://github.com/esa/pagmo2/blob/19d774fbb6128124305225803f8c1ad9e2f95c8c/src/bfe.cpp#L5-L27 * [tbb] Add license. * [embree2] Add "license". * [tbb] Use vcpkg_cmake_config_fixup and fix version as requested by @LilyWangLL * [usd] Add usd is known broken message. * [embree2] Remove completely. * Fix version database. * Fix usd version database. * Fix wrong case on Linux. * Lowercase the tbb directory to get to their configs. * [cctag] minimize patches * [tbb] Apply supports expression fix suggested in #26284 (comment) * [usd] Add note about upstream issue. * [pagmo2] Minimize patch. Co-authored-by: Robert Schumacher <roschuma@microsoft.com> Co-authored-by: Jonliu1993 <13720414433@163.com> Co-authored-by: Mathis Logemann <mathisloge@gmail.com> Co-authored-by: Victor Romero <viromer@microsoft.com> Co-authored-by: Vladimír Aubrecht <vladimir.aubrecht@me.com> Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com> Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com> Co-authored-by: Alexander Neumann <30894796+Neumann-A@users.noreply.github.com>
I took the great work of @alexdewar in #3146 and: