search

orocos-toolchain / rtt

Orocos Real-Time Toolkit
http://www.orocos.org
Other
72 stars 79 forks source link

Normalize include paths #324

Open mvukov opened 3 years ago

mvukov commented 3 years ago

This should simplify compilation and make includes clear. To some extent this was necessary to build rtt with Bazel (Bazel build files are not in this PR). CMake build works on my machine as well.

mvukov commented 3 years ago

@meyerj PTAL.

snrkiwi commented 3 years ago

Why was the include of CMAKE_CURRENT_BINARY_DIR required? The rest of the changes look benign

mvukov commented 3 years ago

CMake-generated config header files live in ${CMAKE_CURRENT_BINARY_DIR}/rtt folder -- that is why I included CMAKE_CURRENT_BINARY_DIR.

With changes in this PR, it would be sufficient to have only two include folders: ${CMAKE_CURRENT_SOURCE_DIR} and ${CMAKE_CURRENT_BINARY_DIR}.

BTW, I don't think CI picked up my new commit after the forced push. The build should succeed with the latest changes.

meyerj commented 2 years ago

Hi @mvukov! Thanks for your patch and I think it is the right approach!

Likely some more redundant INCLUDE_DIRECTORIES() calls can be removed in CMakeLists.txt files in subdirectories? For example

https://github.com/orocos-toolchain/rtt/blob/c1756f3d9b43ae08838ed0fb9613712bfa4b4bd0/rtt/CMakeLists.txt#L185-L187

https://github.com/orocos-toolchain/rtt/blob/c1756f3d9b43ae08838ed0fb9613712bfa4b4bd0/rtt/typekit/CMakeLists.txt#L12-L15

...

Would you still try and update the patch despite of the long time since you submitted this pull request? Otherwise I will try myself and update the patch soon...

BTW, I don't think CI picked up my new commit after the forced push. The build should succeed with the latest changes.

Yes, you are right. Both builds of this PR have been for version fa096d092f54ba7876ac341d2d31861376123365. Anyway, the Travis configuration is currently outdated and does not work anymore since the migration to travis-ci.com. We need to update it (https://github.com/orocos-toolchain/rtt/pull/195, https://github.com/orocos-toolchain/rtt/pull/279) or switch to GitHub actions (https://github.com/orocos-toolchain/rtt/pull/327).

mvukov commented 2 years ago

Hi @meyerj , I'll try to revive this work locally after I merge the latest master. I'll try to clean up INCLUDE_DIRECTORIES as well.