CMake: Review and cleanup target_include_directories()
- Dominant language
- C++
- Stars
- 16.7k
- Forks
- 1.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 126
Description
In CMake build scripts we are using `target_include_directories()` to make all the header files public. With the current setup, we refer all the header files either with their complete path (relative to the root of the repo) or with the local path.
For example, `fdbclient/versions.h` could be included one of two ways
* `#include "fdbclient/versions.h"` from any file in repo.
* `#include "versions.h"` from the files in `fdbclient/` only
By making header files export PUBLIC in `target_include_directories()` we are adding the third path, which is relative to module root path.
* `#include "versions.h"` from any files in the dependent modules of `fdbclient`
Setting PUBLIC/PRIVATE `target_include_directories()` is the right way to do it, I agree. But, a couple of concerns.
* We are exporting PUBLIC for all. Not sure if we want that.
* These new paths are not consistent with the older way of doing things. This is a bigger concern.
I suggest, for now, go with making all PRIVATE includes to be consistent with the old build. Then, we can slowly move to a new and better way of doing things.
@mpilman @alexmiller-apple
Related to: #999
Contributor guide
Research direction
Search the CMake build scripts for target_include_directories() and compare the current PUBLIC declarations with the include patterns described for fdbclient/versions.h. Review related issue #999, then verify that changing the relevant include visibility to PRIVATE preserves the existing include paths and that the project still configures and builds.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cmake, cpp
- Domain
- build-system
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100