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

Peglib as a CMake target #278

Closed
wants to merge 2 commits into from
Closed

Peglib as a CMake target #278

wants to merge 2 commits into from

Conversation

glebzlat
Copy link

It is not comfortable to use a library with FetchContent module, when the library is not specified as a target, so I added add_library(peglib INTERFACE). Also I added options to conditionally build tests, lint and examples, which are not building by default, modified .github/workflows/cmake.yml to build tests, and added a section about CMake into README.md.

Dave and others added 2 commits June 11, 2023 19:58
…pecify option to build tests in a workflow; add CMake Integration part into a README
@yhirose
Copy link
Owner

yhirose commented Jun 13, 2023

@edKotinsky thanks for the pull request. Could you take a look at #251 regarding BUILD_TESTS and #252 which shows my principle for CMake support in this project? Also I would like to keep the CMake support in this project as simple as possible, so that I can build everything quickly with mkdir build && cd $_ && cmake .. && make without memorizing any extra CMake options.

@yhirose
Copy link
Owner

yhirose commented Jun 13, 2023

So, I am ok to accept only the following lines, and I think this can benefit other users to install this header-only library easier. Thanks for your understanding!

add_library(Peglib INTERFACE)
target_include_directories(Peglib INTERFACE ${CMAKE_SOURCE_DIR})

@glebzlat
Copy link
Author

@yhirose yes, of course. But when CMake fetches the project, it automatically executes its CMakeLists.txt file, so tests and examples will be built. Usually user expects when the dependency is fetched, it builds only itself.

@yhirose
Copy link
Owner

yhirose commented Jul 8, 2023

@glebzlat sorry for the late reply. I am not thinking to merge your changes.
The only thing is about the change for BUILD_TESTS. Could you take a look at #251? Is there any strong reason to get back to PEGLIB_BUILD_TESTS ?

@yhirose
Copy link
Owner

yhirose commented Jul 12, 2023

I implemented it in another way based on https://github.com/tinfoilboy/CTML/blob/master/CMakeLists.txt. Thanks for showing this idea!

@yhirose yhirose closed this Jul 12, 2023
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

Successfully merging this pull request may close these issues.

2 participants