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

Embed FCL in source code #39

Merged
merged 12 commits into from
Sep 2, 2021
Merged

Embed FCL in source code #39

merged 12 commits into from
Sep 2, 2021

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Aug 27, 2021

Following the discussion here we are going to embed a copy of FCL-0.6.1 into the source code of rmf_traffic. The build script for rmf_traffic will first check if an FCL version 0.6 or greater is available on the target system. If it is not, then rmf_traffic will build fcl-0.6.1 from the embedded source.

So far this has worked fine when testing on my local machine, but there are a few open questions that should probably need to be addressed:

  • Should we build, install, and link against an .so for fcl-0.6.1, or should we skip building the .so and instead only build a static library for fcl-0.6.1 to link rmf_traffic against?
  • Should the package.xml of rmf_traffic include <depends>fcl</depends>? That line is needed if a developer is building their own version of FCL in their colcon workspace and wants to link rmf_traffic against their own version, but I wonder if there are any harmful side effects for the build farm or for rosdep?

@mxgrey mxgrey requested a review from marcoag August 27, 2021 11:29
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #39 (7165dac) into main (875e7e8) will decrease coverage by 10.50%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##             main      #39       +/-   ##
===========================================
- Coverage   32.64%   22.13%   -10.51%     
===========================================
  Files         159      353      +194     
  Lines       15729    29140    +13411     
  Branches    10489    13425     +2936     
===========================================
+ Hits         5134     6449     +1315     
- Misses       1736    13540    +11804     
- Partials     8859     9151      +292     
Flag Coverage Δ
tests 22.13% <0.00%> (-10.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...y/fcl/include/fcl/broadphase/broadphase_SSaP-inl.h 0.00% <0.00%> (ø)
...ty/fcl/include/fcl/broadphase/broadphase_SaP-inl.h 0.00% <0.00%> (ø)
...dparty/fcl/include/fcl/broadphase/broadphase_SaP.h 0.00% <0.00%> (ø)
...include/fcl/broadphase/broadphase_bruteforce-inl.h 0.00% <0.00%> (ø)
.../fcl/broadphase/broadphase_collision_manager-inl.h 0.00% <0.00%> (ø)
...hase/broadphase_continuous_collision_manager-inl.h 0.00% <0.00%> (ø)
.../fcl/broadphase/broadphase_dynamic_AABB_tree-inl.h 0.00% <0.00%> (ø)
...roadphase/broadphase_dynamic_AABB_tree_array-inl.h 0.00% <0.00%> (ø)
...lude/fcl/broadphase/broadphase_interval_tree-inl.h 0.00% <0.00%> (ø)
...nclude/fcl/broadphase/broadphase_spatialhash-inl.h 0.00% <0.00%> (ø)
... and 351 more

@marcoag
Copy link
Member

marcoag commented Aug 31, 2021

* [ ]  Should we build, install, and link against an `.so` for fcl-0.6.1, or should we skip building the `.so` and instead only build a static library for fcl-0.6.1 to link `rmf_traffic` against?

I would say building the .so is wrong because, as a consequence, when installing the rmf_traffic package some extra library (fcl) would be installed in a hidden way without the user knowing about it. This probably goes against debian policies and can cause conflicts with system packages or other installations. I am in favor of the static library option.

* [ ]  Should the `package.xml` of `rmf_traffic` include `<depends>fcl</depends>`? That line is needed if a developer is building their own version of FCL in their colcon workspace and wants to link `rmf_traffic` against their own version, but I wonder if there are any harmful side effects for the build farm or for rosdep?

This still will create problems when building the package in the buildfarm as that package.xml dependency gets converted to a non existent system dependency and the package build fails. This could be solved by removing the dependency with a patch at the release repo level, but those patches are a burden to maintain and a potential source for future errors so I would like to try to avoid them as much as possible. Could we keep it commented (I believe that's how it is now) and leave it up to the developer to make the changes in case he/she wants to build his/her own version of FCL? or is there any other way we can support developers own builds of FCL?

@mxgrey
Copy link
Contributor Author

mxgrey commented Aug 31, 2021

I've hacked up the fcl build scripts to remove the shared library and avoid installing anything (be it shared object files, headers, or config files). I think there shouldn't be any more risk of conflicts with installations of other versions of fcl.

@marcoag marcoag merged commit dea2321 into main Sep 2, 2021
@marcoag marcoag deleted the fix/embed_fcl branch September 2, 2021 06:02
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.

2 participants