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

Normalize include paths #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvukov
Copy link

@mvukov mvukov commented Sep 26, 2020

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
Copy link
Author

mvukov commented Sep 26, 2020

@meyerj PTAL.

@mvukov mvukov force-pushed the feature/normalize_include_paths branch from 287e8a9 to c1756f3 Compare September 26, 2020 21:36
@snrkiwi
Copy link
Contributor

snrkiwi commented Sep 27, 2020

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

@mvukov
Copy link
Author

mvukov commented Sep 27, 2020

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
Copy link
Member

meyerj commented Nov 29, 2021

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

rtt/rtt/CMakeLists.txt

Lines 185 to 187 in c1756f3

# Due to generation of some .h files in build directories, we also need to include some build dirs in our include paths.
INCLUDE_DIRECTORIES(BEFORE ${PROJ_SOURCE_DIR} ${PROJ_SOURCE_DIR}/rtt ${PROJ_SOURCE_DIR}/rtt/os ${PROJ_SOURCE_DIR}/rtt/os/${OROCOS_TARGET} )
INCLUDE_DIRECTORIES(BEFORE ${PROJ_BINARY_DIR}/rtt ${PROJ_BINARY_DIR}/rtt/os ${PROJ_BINARY_DIR}/rtt/os/${OROCOS_TARGET} )

# Due to generation of some .h files in build directories, we also need to include some build dirs in our include paths.
INCLUDE_DIRECTORIES(BEFORE ${PROJ_SOURCE_DIR} ${PROJ_SOURCE_DIR}/rtt ${PROJ_SOURCE_DIR}/rtt/os ${PROJ_SOURCE_DIR}/rtt/os/${OROCOS_TARGET} )
INCLUDE_DIRECTORIES(BEFORE ${PROJ_BINARY_DIR}/rtt ${PROJ_BINARY_DIR}/rtt/os ${PROJ_BINARY_DIR}/rtt/os/${OROCOS_TARGET} )
INCLUDE_DIRECTORIES(BEFORE ${PROJ_BINARY_DIR}/rtt/typekit )

...

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 fa096d0. Anyway, the Travis configuration is currently outdated and does not work anymore since the migration to travis-ci.com. We need to update it (#195, #279) or switch to GitHub actions (#327).

@mvukov
Copy link
Author

mvukov commented Nov 29, 2021

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.

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