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

Make InstallBasicPackageFiles FetchContent / add_subdirectory friendly #162

Closed
traversaro opened this issue Aug 9, 2018 · 6 comments
Closed

Comments

@traversaro
Copy link
Member

traversaro commented Aug 9, 2018

A current trend in some CMake projects is to include external dependencies using add_subdirectory command. In general this does not work (due to cross talking between the two projects, see https://www.reddit.com/r/cpp/comments/8sie4b/i_manage_the_release_cycle_for_cmake_the_build/e11imjh/ ) but if the external dependency is simple enough, it is quite convenient.

This workflow is actually the one for which the new CMake module FetchContent has been designed, as you may see in the FetchContent docs.

Ideally, it would be great if using InstallBasicPackageFiles in one project did not prevented to use of the project using FetchContent/add_subdirectory. Unfortunatly at the moment this is not the case, as described in robotology/idyntree#397 .

@traversaro
Copy link
Member Author

traversaro commented Aug 9, 2018

In general, the problem for this are the use of CMAKE_SOURCE_DIR and CMAKE_BINARY_DIR .
@drdanz @claudiofantacci @diegoferigo do you think we can simply substitute CMAKE_SOURCE_DIR with PROJECT_SOURCE_DIR in https://github.com/robotology/ycm/blob/master/modules/InstallBasicPackageFiles.cmake#L399 or we need a specific option to switch between the two behaviors?

GitHub
ycm - Extra CMake Modules for YARP and friends

@claudiofantacci
Copy link
Collaborator

I hope to have got the point correct since I have never dealt with project inceptions. I'll ask some questions to understand the context and also to clarify the situation.

This workflow is actually the one for which the new CMake module FetchContent.

@traversaro could yuo please complete the sentence 😄 ? Thanks!

My first thought about this external projects into project using FetchContent/add_subdirectory thing is that it is (somewhat?) wrong to do in CMake. I would go with ExternalProject module. Why would you need to have an external project in your project instead of installing first the external projects and then your project that depends on it? Maybe I'm missing a point here or a use case.

Anyway:

do you think we can simply substitute CMAKE_SOURCE_DIR with PROJECT_SOURCE_DIR

Basically we could to give a higher "scope of visibility" to InstallBasicPackageFiles.cmake for those variable to allow this particular FetchContent/add_subdirectory use case to work. Am I wrong? Technically, we could do it adding an option to do this "substitution" and, for the purpose of this particular context it should be working. However, if my previous statements are correct, by allowing this aren't we implicitly suggesting that adding external projects with FetchContent/add_subdirectory is (somewhat) correct/doable? If this is the case, I would be considerate in modifying InstallBasicPackageFiles.cmake.

@traversaro
Copy link
Member Author

@traversaro could yuo please complete the sentence 😄 ? Thanks!

This workflow is actually the one for which the new CMake module FetchContent has been designed, as you may see in the FetchContent docs.

@traversaro
Copy link
Member Author

Section 27.2 of Professional CMake: A Practical Guide is related to this discussion.

@drdanz
Copy link
Member

drdanz commented Mar 26, 2019

I don't like the idea of using PROJECT_{SOURCE,BINARY}_DIR instead in #233 I used CMAKE_CURRENT_{SOURCE,BINARY}_DIR. As a consequence, InstallBasicPackageFiles should be used in the main CMakeLists.txt

@drdanz
Copy link
Member

drdanz commented Mar 26, 2019

Fixed by #233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
YCM 1.0.0
  
Done
Development

No branches or pull requests

3 participants