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

There should be exportation of include directories in CMake file. #460

Closed
navidR opened this issue Oct 6, 2018 · 10 comments
Closed

There should be exportation of include directories in CMake file. #460

navidR opened this issue Oct 6, 2018 · 10 comments

Comments

@navidR
Copy link

navidR commented Oct 6, 2018

After this line there should be a line along this line :

target_include_directories(dmlc PUBLIC include)

FYI this is a temporary fix. A better solution would be only using target_include_directories instead of include_directories. I am willing to send a small patch to fix CMake files if you are willing to accept patches.

@hcho3
Copy link
Contributor

hcho3 commented Oct 7, 2018

@navidR

After this line there should be a line along this line

Can you describe what kind of problems arise because we don't have target_include_directories currently?

@navidR
Copy link
Author

navidR commented Oct 7, 2018

You are using include_directories. There are a bunch of problems with this.

First of all, in my case, I am using dmlc-core as a subdirectory in my cmake project. So when you don't use target_include_directories, when I do target_link_libraries(${my_project} PRIVATE dmlc) then include include directories of dlmc-core does not get included automatically. Which makes a pain in the back.

Second, this is not the correct way in Modern CMake. As I told you, I am willing to send a patch to modernize your cmake implementation since I am going to use dmlc-core and halideir in one of my projects.

For more information you can look at this links:
https://stackoverflow.com/questions/31969547/what-is-the-difference-between-include-directories-and-target-include-directorie
https://cliutils.gitlab.io/modern-cmake/

@hcho3
Copy link
Contributor

hcho3 commented Oct 7, 2018

Sure, ability to automatically locate dmlc-core headers sounds nice. You should submit a pull request so that me and other maintainers can review it.

Some considerations:

  • There are several projects that uses dmlc-core, such as apache/incubator-mxnet and dmlc/xgboost. I don't think they follow what you call "Modern CMake" practices, your pull request should not break existing CMake bulids.
  • To make this a learning opportunity for me and others: is there significant consensus in C++ community about what constitutes Modern CMake or its merits? Is Modern CMake akin to Modern C++ in terms of adoption and support? I know a bit about what Modern C++ looks like (e.g. don't manage raw pointers; use smart pointers / resource-managing classes), but to me the term Modern CMake is quite new.

@navidR
Copy link
Author

navidR commented Oct 7, 2018

About the first point, sure. I understand how painful it is to rewrite portions of your build tools. So I will keep it 100% compatible.

About the second point, we all can agree CMake has become the defacto build system for C++ (particularly modern C++ libraries. Even boost is in process of migrating their build system to CMake). But the problem is CMake initial design had some serious flows. They are (have) working (worked) hard to improve it. So the old way in CMake was to use stuff like include_directores or link_libraries and stuff like this which was universal and there was no notion of targets. But in modern CMake you have targets and you should only use targets. Why?

  1. Because with the target using your, and other people libraries is as simple as add_subdirectory.
  2. You have encapsulation. You can expose a library or include directory to your user so when people are linking against your library they exposed to those headers and libraries too. Conversely, you can not expose any library you are depending on to your users.

In terms of adaptation, as far as I know (and see), most modern (and newly written) C++ library has adopted CMake (particularly Modern CMake) or they are in process of adopting it. For example, Boost developers have decided they are going to abandon their build system in favour of CMake. Facebook/folly has changed it build system to CMake and many others.

@hcho3
Copy link
Contributor

hcho3 commented Oct 7, 2018

I get your point about encapsulation. Also, the widespread adoption of CMake as build tool makes sense as well. My question would be: Do newly written C++ libraries tend to adopt Modern CMake conventions? Technical merits alone (encapsulation) are sufficient to consider the switch, but if we know other projects are adopting it as well the case for switching would be even stronger, since future contributors would find the practices conventional and idiomatic.

Also, a bit of googling gave me the link https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/. Is this in line with what you had in mind for Modern CMake?

@navidR
Copy link
Author

navidR commented Oct 7, 2018

  1. As far as I know, most newly written C++ libraries emphasize Modern CMake. Some of them switch to CMake from other build systems. But some other improve their old style CMake to newer style. But the thing is as you already know people tend to don't touch their build system. But switching to Modern CMake is already happening.

  2. Yes, although I didn't read the link but after looking at codes in its page that is the general idea.

@hcho3
Copy link
Contributor

hcho3 commented Oct 7, 2018

Got it. I'll look forward to your pull request.

@hcho3
Copy link
Contributor

hcho3 commented Oct 27, 2018

@navidR I'd like to bring CMakeLists.txt up to modern standards and submit a pull request. Would you be willing to review it?

@navidR
Copy link
Author

navidR commented Oct 29, 2018

Yes of course. Sadly I am so busy right now to write it myself (Although I am definitely going to write it if has not done till then). But I would definitely read & review it.

@hcho3
Copy link
Contributor

hcho3 commented Apr 30, 2019

Resolved by #495

@hcho3 hcho3 closed this as completed Apr 30, 2019
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

No branches or pull requests

2 participants