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

Remove rock macros #48

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Remove rock macros #48

wants to merge 2 commits into from

Conversation

0Nel
Copy link
Collaborator

@0Nel 0Nel commented Feb 28, 2024

No description provided.

@0Nel 0Nel requested a review from skasperski February 28, 2024 11:28
@planthaber
Copy link
Member

  • Also build vizkit3d plugins
  • fix types dependencies (use internal types if base-types is not found

@Rauldg
Copy link
Collaborator

Rauldg commented Feb 28, 2024

Why not build with rock-macros if they are available?

If the rock-cmake macros are available in a workspace and currently the installation supports it use, why just removing this feature? It would make the compatibility with future rock updates easier and it should not make any difference for the goal of building the library without rock (else case).

@skasperski
Copy link
Collaborator

Why not build with rock-macros if they are available?

I think that would just unnecessarily bloat the code and I don't see any benefit from it, as autoproj can easily handle CMake-Packages and install them into a workspace. The CMake-Macros do not provide any advantage anymore.

@Rauldg
Copy link
Collaborator

Rauldg commented Feb 28, 2024

Well it is true that the rock cmake macros are there to save cmake development time. If the developers have already implemented those additional CMake lines, then it indeed doesn't seem to make much sense to keep using the macros as @skasperski points out.

On the other hand, the point of forward compatibility with Autoproj still holds. Although it is also true that Autoproj anyway also supports CMake projects without the macros, it just seems to me that it might bring some extra work when using it in Rock in the future. But not sure yet about that.

@Rauldg
Copy link
Collaborator

Rauldg commented Feb 28, 2024

From my side, this can also be merged but I think @planthaber has pointed out 2 missing points, does that mean that this should not be merged yet? Maybe a good idea then to put this as a draft so we can know when it is ready for testing?

@planthaber planthaber marked this pull request as draft February 29, 2024 10:13
@planthaber
Copy link
Member

If we can create a good solution here, the idea is to update the "rock-create-lib" template or at least create an alternative "rock-create-cmake-only-lib" template that is compatible with the rock definitions (enable/disable vizkit3d, tests, etc.).

This PR is now converted to draft btw.

@haider8645
Copy link
Contributor

See #49 to write the pkg-config deps in correct format for the maps.pc so that the orogen component can find the dependencies.

At the moment the maps. pc requires looks like

Requires: base-types;base-logging;boost_serialization;pcl_io-1.10
After the proposed change it looks like
Requires: base-types base-logging boost_serialization pcl_io-1.10

@haider8645
Copy link
Contributor

I tried to the build the cmake version of the lib and result is a failed build with the error:

/opt/workspace/slam/maps/src/grid/GridMap.hpp:41:10: fatal error: maps/LocalMap.hpp: No such file or directory 41 | #include <maps/LocalMap.hpp> | ^~~~~~~~~~~~~~~~~~~
I removed all the maps/ prefixes because the target include is linked to the src/ directory. The removal made the library build work but then the orogen component failed because the the headers are installed in maps/include/maps/* so the orogen component failed to find the headers included with the /maps prefix.

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.

5 participants