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.

Sign up for GitHub

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

Jump to bottom

Changed INTERFACE location to CMAKE_CURRENT_SOURCE_DIR for package ma… #298

Merged
merged 1 commit into from
May 2, 2024
Merged

Changed INTERFACE location to CMAKE_CURRENT_SOURCE_DIR for package ma… #298

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
endif()

add_library(peglib INTERFACE)
target_include_directories(peglib INTERFACE ${CMAKE_SOURCE_DIR})
target_include_directories(peglib INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
target_include_directories(peglib INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
target_include_directories(peglib INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_PREFIX}/include>
)

We should separate include directories for build/install.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it even matter if the lib isn't installed? Only the header itself is installed, there's no export for the lib.

Copy link

@jimmy-park jimmy-park May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't check the whole CMakeLists.txt. This suggestion has no effect but I suggested it routinely.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimmy-park so can we merge the original change by @trcwm as it is?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhirose Yes, please ignore this suggestion.


option(BUILD_TESTS "Build cpp-peglib tests" ON)
option(PEGLIB_BUILD_LINT "Build cpp-peglib lint utility" OFF)
Expand Down
Loading