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

Fixes for building on Ubuntu Noble #1016

Merged
merged 6 commits into from
Jul 11, 2024

Conversation

rjoomen
Copy link
Contributor

@rjoomen rjoomen commented Jun 23, 2024

Pending a rebase after #1006 and #1015 have been merged.

@rjoomen rjoomen requested a review from Levi-Armstrong June 23, 2024 11:27
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.84%. Comparing base (2a92fa8) to head (18a89da).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1016      +/-   ##
==========================================
- Coverage   89.86%   89.84%   -0.02%     
==========================================
  Files         280      280              
  Lines       15908    15908              
==========================================
- Hits        14296    14293       -3     
- Misses       1612     1615       +3     

see 2 files with indirect coverage changes

@Levi-Armstrong
Copy link
Contributor

@rjoomen Ping me when this ready. I assume you are working on changes to get the CI to pass?

@rjoomen
Copy link
Contributor Author

rjoomen commented Jun 27, 2024

Yes, there's a test failing and I didn't have time to look into it. Will have time tomorrow, I hope.

@cboostjvisser
Copy link

I have been investigating the failing tests. I have not been able to pinpoint the exact problem (yet).
But here are my findings:

The failing tests are:

There are already comments about failing on Ubuntu 18.04 and a #if BOOST_VERSION > 106800 && !__APPLE__ surrounding the tests.

Boost investigation:

  • Ubuntu 22.04(Jammy) uses Boost 1.74

  • Ubuntu 24.04(Noble) uses Boost 1.83

  • Downgrading just Boost.DLL(library module) from 1.83 to 1.74 does not help.

  • Downgrading whole the Ubuntu Boost installation from 1.83 to 1.74 does fix the tests.

dlopen() seems to be called with exact the same arguments. But does not return handle in 1.83. See here:
https://github.com/boostorg/dll/blob/6c60dde50bf67138c90cc84938111866813feaff/include/boost/dll/detail/posix/shared_library_impl.hpp#L118

Tests might not be correct?

I'm also questioning if the tests are correct.
If I read this post: https://stackoverflow.com/a/17716576
It seems like we need to load a local library with "./" leading the libname, or else DT_RPATH or DT_RUNPATH needs to be set.

The current tesseract_common/class_loader.hpp always sets boost::dll::load_mode::search_system_folders if there is no library_path specified.
For example:

boost::dll::load_mode::append_decorations | boost::dll::load_mode::search_system_folders;

That will trigger the "./" preprending to be disabled in the Boost.DLL library: https://github.com/boostorg/dll/blob/6c60dde50bf67138c90cc84938111866813feaff/include/boost/dll/detail/posix/shared_library_impl.hpp#L97

Conclusion

Removing all the boost::dll::load_mode::search_system_folders in tesseract_common/class_loader.hpp seem to fix the tests. But not sure what other problems that might cause.
I was not able to find what causes the tests to work with Boost 1.74 but not with Boost 1.83.

@rjoomen
Copy link
Contributor Author

rjoomen commented Jul 9, 2024

Additional information, adding the current directory explicitly also fixes the tests.

Change lines 85 and 86 to:

EXPECT_TRUE(ClassLoader::isClassAvailable(symbol_name, lib_name, ".")); 
auto plugin = ClassLoader::createSharedInstance<TestPluginBase>(symbol_name, lib_name, ".");

And add this before line 155:

plugin_loader.search_paths.insert(".");

@rjoomen
Copy link
Contributor Author

rjoomen commented Jul 9, 2024

The current tesseract_common/class_loader.hpp always sets boost::dll::load_mode::search_system_folders if there is no library_path specified. For example:

boost::dll::load_mode::append_decorations | boost::dll::load_mode::search_system_folders;

That will trigger the "./" preprending to be disabled in the Boost.DLL library: https://github.com/boostorg/dll/blob/6c60dde50bf67138c90cc84938111866813feaff/include/boost/dll/detail/posix/shared_library_impl.hpp#L97

Conclusion

Because class_loader sets mode search_system_folders, the Tesseract class loaders by design do not use the local directory if no search path is given.

@Levi-Armstrong, should we modify the class loader or the tests to deal with this?

@Levi-Armstrong
Copy link
Contributor

I would update the unit test for now.

@Levi-Armstrong Levi-Armstrong merged commit 0c6f05c into tesseract-robotics:master Jul 11, 2024
11 of 14 checks passed
@rjoomen rjoomen deleted the noblefixes branch July 11, 2024 15:59
marip8 pushed a commit to marip8/tesseract that referenced this pull request Dec 13, 2024
marip8 pushed a commit that referenced this pull request Dec 23, 2024
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.

3 participants