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.

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

be compatible with log4cxx 0.11 and 0.12 #54

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

dreuter
Copy link
Contributor

@dreuter dreuter commented May 11, 2022

log4cxx uses std::shared_ptr
since version 0.12

Unfortunately Ubuntu 22.04 ships with
0.12, which means
that rosconsole with the log4cxx backend wouldn't compile. This is
also a problem on other distros such as Arch or Gentoo.

I have carefully applied explicit construction/conversion and &*
instead of .get() to make the code compile with both versions of
log4cxx.

Fixes #50

@dreuter
Copy link
Contributor Author

dreuter commented May 11, 2022

If you want to test this easily I have created two quick and dirty containers that I have tested with:
branch: https://github.com/dreuter/rosconsole/tree/docker-to-verify

podman build -t rosconsole-test:jammy jammy
podman run -v$(pwd):/root/ws/src/rosconsole rosconsole-test:jammy

Does work with docker as well (just replace podman with docker)

@dreuter dreuter mentioned this pull request May 11, 2022
[`log4cxx` uses `std::shared_ptr`](https://issues.apache.org/jira/browse/LOGCXX-486)
since [version `0.12`](https://logging.apache.org/log4cxx/latest_stable/changelog.html)

Unfortunately Ubuntu 22.04 ships with
[`0.12`](https://packages.ubuntu.com/jammy/liblog4cxx12), which means
that rosconsole with the `log4cxx` backend wouldn't compile. This is
also a problem on other distros such as Arch or Gentoo.

I have carefully applied explicit construction/conversion and `&*`
instead of `.get()` to make the code compile with both versions of
`log4cxx`.
@AchmadFathoni
Copy link

Got this error when compile it with log4cxx 0.13

 50%] Building CXX object CMakeFiles/rosconsole_log4cxx.dir/src/rosconsole/impl/rosconsole_log4cxx.cpp.o
/home/toni/.cache/yay/ros-noetic-rosconsole/src/rosconsole-1.14.3/src/rosconsole/impl/rosconsole_log4cxx.cpp: In function ‘void ros::console::impl::print(void*, ros::console::Level, const char*, const char*, const char*, int)’:
/home/toni/.cache/yay/ros-noetic-rosconsole/src/rosconsole-1.14.3/src/rosconsole/impl/rosconsole_log4cxx.cpp:187:98: error: no matching function for call to ‘log4cxx::spi::LocationInfo::LocationInfo(const char*&, const char*&, int&)’
  187 |     logger->forcedLog(g_level_lookup[level], str, log4cxx::spi::LocationInfo(file, function, line));
      |                                                                                                  ^
In file included from /usr/include/log4cxx/logger.h:33,
                 from /usr/include/log4cxx/spi/loggingevent.h:28,
                 from /usr/include/log4cxx/layout.h:29,
                 from /usr/include/log4cxx/appenderskeleton.h:28,
                 from /home/toni/.cache/yay/ros-noetic-rosconsole/src/rosconsole-1.14.3/src/rosconsole/impl/rosconsole_log4cxx.cpp:42:
/usr/include/log4cxx/spi/location/locationinfo.h:94:17: note: candidate: ‘log4cxx::spi::LocationInfo::LocationInfo(const log4cxx::spi::LocationInfo&)’
   94 |                 LocationInfo( const LocationInfo& src );
      |                 ^~~~~~~~~~~~
/usr/include/log4cxx/spi/location/locationinfo.h:94:17: note:   candidate expects 1 argument, 3 provided
/usr/include/log4cxx/spi/location/locationinfo.h:88:17: note: candidate: ‘log4cxx::spi::LocationInfo::LocationInfo()’
   88 |                 LocationInfo();
      |                 ^~~~~~~~~~~~
/usr/include/log4cxx/spi/location/locationinfo.h:88:17: note:   candidate expects 0 arguments, 3 provided
/usr/include/log4cxx/spi/location/locationinfo.h:80:17: note: candidate: ‘log4cxx::spi::LocationInfo::LocationInfo(const char*, const char*, const char*, int)’
   80 |                 LocationInfo( const char* const fileName,
      |                 ^~~~~~~~~~~~
/usr/include/log4cxx/spi/location/locationinfo.h:80:17: note:   candidate expects 4 arguments, 3 provided
make[2]: *** [CMakeFiles/rosconsole_log4cxx.dir/build.make:76: CMakeFiles/rosconsole_log4cxx.dir/src/rosconsole/impl/rosconsole_log4cxx.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:353: CMakeFiles/rosconsole_log4cxx.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

@dreuter
Copy link
Contributor Author

dreuter commented May 17, 2022

The culprit seems to be: apache/logging-log4cxx#95

I will need to see how I can fix that :/

@dreuter dreuter changed the title be compatible with log4cxx 0.11 and 0.12/0.13 be compatible with log4cxx 0.11 and 0.12 May 17, 2022
@jspricke
Copy link
Member

for log4cxx 0.13 you can do something like:

#ifdef LOG4CXX_VERSION_MAJOR
    logger->forcedLog(g_level_lookup[level], str, log4cxx::spi::LocationInfo(file, log4cxx::spi::LocationInfo::calcShortFileName(file), function, line));
#else
     logger->forcedLog(g_level_lookup[level], str, log4cxx::spi::LocationInfo(file, function, line));
#endif

}
// reset this so that the logger doesn't get crashily destroyed
// again during global destruction.
//
// See https://code.ros.org/trac/ros/ticket/3271
//
log4cxx::Logger::getRootLogger()->getLoggerRepository()->shutdown();
static_cast<log4cxx::spi::LoggerRepositoryPtr>(log4cxx::Logger::getRootLogger()->getLoggerRepository())->shutdown();
Copy link
Member

Choose a reason for hiding this comment

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

this changes seems not to bee needed and produces segfaults with log4cxx 13.

Copy link
Contributor Author

@dreuter dreuter Jul 5, 2022

Choose a reason for hiding this comment

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

Without this change I do get the following error (I compiled as described with the container mentioned above):

[ 28%] Building CXX object CMakeFiles/rosconsole_log4cxx.dir/src/rosconsole/impl/rosconsole_log4cxx.cpp.o
/root/ws/src/rosconsole/src/rosconsole/impl/rosconsole_log4cxx.cpp: In function ‘void ros::console::impl::shutdown()’:
/root/ws/src/rosconsole/src/rosconsole/impl/rosconsole_log4cxx.cpp:385:58: error: base operand of ‘->’ has non-pointer type ‘log4cxx::spi::LoggerRepositoryWeakPtr’ {aka ‘std::weak_ptr<log4cxx::spi::LoggerRepository>’}
  385 |   log4cxx::Logger::getRootLogger()->getLoggerRepository()->shutdown();
      |                                                          ^~
make[3]: *** [CMakeFiles/rosconsole_log4cxx.dir/build.make:76: CMakeFiles/rosconsole_log4cxx.dir/src/rosconsole/impl/rosconsole_log4cxx.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:368: CMakeFiles/rosconsole_log4cxx.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have an idea why it is SEGFAULTing?

Copy link
Member

Choose a reason for hiding this comment

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

This is with log4cxx 12, I guess?
You can find the version I uploaded for Debian here:
https://salsa.debian.org/science-team/ros-rosconsole/-/blob/dd5d554993aacc6b266e130bd8da5a7b1fdcaaf6/debian/patches/0002-Fix-for-log4cxx-0.12.0.patch
and the one currently used for log4cxx 13 here:
https://salsa.debian.org/science-team/ros-rosconsole/-/tree/master/debian/patches

those got quiet a bit of testing by me and others and there where no problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an idea why it is SEGFAULTing?

Because in log4cxx 0.13 they changed the pointer to a raw pointer after switching to weak_ptr before in 0.12.
Thus in 0.13 your patch creates a shared_ptr from a raw pointer and deletes the logger repository after evaluating the line without having ownership. I'm not surprised that segfaults.

Please see e3753ee for a patch used in ROS-O that's supposed to support log4cxx 0.11 to 0.13. Sadly I had to add another code path to handle 0.12.

twdragon added a commit to twdragon/rosconsole that referenced this pull request Nov 14, 2022
	- Partially implements changes suggested both in ros#54 and
	  orphaned commit e3753ee
	- Tested on liblog4cxx v0.12 and 0.13
	- Sets up pointer-level compatibility with both new and old
	  log4cxx versions
	- Adds a workaround for API breaking changes in liblog4cxx
	- Testing needed

Signed-off-by: Andrey Vukolov <andrey.vukolov@elettra.eu>
@defenzelite-robotics
Copy link

Hello guys i am getting the same error but for ubuntu 24.04 i have checked the version of log4cxx is 1.1.0-1build3: amd64 arm64 armhf ppc64el riscv64 s390x
what changes needs to be done ?

@erikbeall
Copy link

@defenzelite-robotics I also attempted to build with log4cxx-1.1.0-1build3 and I'm not enough of a user of C++ to have found a solution. For a quick hack you could set the ROSCONSOLE_BACKEND to print or glog in the CMakeLists.txt inside the rosconsole base source directory in your build (e.g. ~/ros_catkin_ws/src/rosconsole/CMakeLists.txt).

However, beware that there are other issues with >Ubuntu20 compatibility further along the chain - this is only the first blocker I ran into (noetic is not fully supported past Ubuntu 20.04). I've run thru about ten other mini blockers trying to get roscore to come up (I have ROS2 in parallel and need ROS1 for already-deployed systems as we gradually migrate) with more blockers still coming (I'll likely just end up using docker for ROS1).

@lucasw
Copy link
Contributor

lucasw commented Jun 5, 2024

You might be interested in https://github.com/lucasw/rosconsole/tree/ubuntu_2210 - it's got some necessary patches including the log4cxx fix and some changes of mine for formatting the output differently (those ought to be easy to remove if you made your own branch)- I've been using it fine in 22.04 (using the debian apt installed ros packages with others built from source described here - rosconsole doesn't need to be built from source on 22.04 but I do it for my alterations).

For Ubuntu 24.04 all the base packages need to be built from source but after that it's a similar process. If you try that and have trouble feel free to open an issue in https://github.com/lucasw/ros_from_src/issues (and better to discuss there or maybe https://github.com/ros-o/ros-o/discussions instead of getting more and more off topic in this thread).

@erikbeall
Copy link

erikbeall commented Jun 6, 2024

Thank you Lucas, your fixes for 22 worked on Ubuntu 24 with log4cxx-1.1! For completeness for other users, I cloned lucas's referenced repo in the src subdirectory and checked-out the ubuntu_2210 branch, then built, e.g. for just that package:
~/ros_catkin_ws $ ./src/catkin/bin/catkin_make_isolated --install -DCMAKE_BUILD_TYPE=Release --pkg rosconsole

I'm still working on getting roscore fully working, strace shows a few differences before the hang vs an Ubuntu 20 noetic stock, will report back on your ros_from_source/issues if I get further (and will start from your build instructions).

@erikbeall
Copy link

Lucas, your ubuntu2404 branch worked to build a working roscore! I was able to build other components without no major challenges (perception, robot - I skipped diagnostic_common and perception_pcl). Note to others trying to build for Ubuntu 24, use Lucas's ubuntu_2404 branch at https://github.com/lucasw/ros_from_src/.

@lucasw
Copy link
Contributor

lucasw commented Jun 8, 2024

I can't promise any stability from any of those forks, and sometimes I force push into them and rewrite commit history- if you have something that works now and you don't want deal with upstream issues my changes could create you should stay on an exact commit (or even make your own fork and branch at that commit for another layer of protection), and try out upstream changes only when you want to.

@jspricke
Copy link
Member

jspricke commented Jun 9, 2024

@erikbeall you can also use the ROS One repos: https://github.com/ros-o/ those contain all relevant patches and should give you a more stable base.

@lucasw
Copy link
Contributor

lucasw commented Jun 9, 2024

All my forks are based off of ros-o obese-devel branches where available, but then I found I had to add some debian patches like https://salsa.debian.org/science-team/ros-rosconsole/-/tree/master/debian/patches as well (for example lucasw@048c410 )- but maybe that was only because I was overlaying apt installed debian packages that had all those patches in them, and in Ubuntu 24.04 the debian packages aren't available so that isn't necessary? I haven't tried that (but it's no advantage to me if I could since I want to use the same branches in 22.04 and 24.04 and use pre-built as much as possible).

@jspricke
Copy link
Member

jspricke commented Jun 9, 2024

@lucasw It could be that we miss some patches in ros-o but Soname patch you linked is surely not needed. Feel free to open bugs in ros-o if you find missing patches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when compiling with log4cxx 0.12.0
7 participants