You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Remove more global include_directories calls in favor of target_include_directories. Make sure the targets relying on these global includes link/include what they need.
Make the api target an interface link to the sdk target
Remove usage of the sdk headers in the foo_* example libraries (These example libraries should link to the api only)
For significant contributions please make sure you have completed the following items:
dbarker
changed the title
remove global include_directories usage and rely on target properties
[CMAKE] remove global include_directories usage and rely on target properties
May 20, 2025
Seems this is a breaking change if the user relies on the include_directories() behavior?
@ThomsonTan I don't expect any breaking changes to users. The scope of the include_directories call covers all targets defined in the same CMakeLists.txt file and the sub directories it adds. User code shouldn't be included in this scope.
Seems this is a breaking change if the user relies on the include_directories() behavior?
@ThomsonTan I don't expect any breaking changes to users. The scope of the include_directories call covers all targets defined in the same CMakeLists.txt file and the sub directories it adds. User code shouldn't be included in this scope.
FYI, here is one case which relied on include_directories() to work, but needs to switch to target_link_libraries() now.
Seems this is a breaking change if the user relies on the include_directories() behavior?
@ThomsonTan I don't expect any breaking changes to users. The scope of the include_directories call covers all targets defined in the same CMakeLists.txt file and the sub directories it adds. User code shouldn't be included in this scope.
FYI, here is one case which relied on include_directories() to work, but needs to switch to target_link_libraries() now.
That is a good catch. One of the drawbacks with the global include_directories (and why it is good to move towards target_include_directories) is that it can hide issues like this since the global includes are not transitive with the targets. The fix you made in open-telemetry/opentelemetry-cpp-contrib#551 is a good one.
Another instance where issues were hidden is with the foo_* library examples. They had included the sdk headers but did not link to the sdk targets (and shouldn't have). If a user copied those examples and the cmake files to build in their project - they would have previously failed to find the sdk headers at compile time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
More cmake cleanup.
Changes
include_directoriescalls in favor oftarget_include_directories. Make sure the targets relying on these global includes link/include what they need.foo_*example libraries (These example libraries should link to the api only)For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes