-
Notifications
You must be signed in to change notification settings - Fork 680
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
added CMakeLists.txt to support building via cmake #223
base: master
Are you sure you want to change the base?
Conversation
Thank you for your contribution, @thorsten-klein |
Thank you for your fast reply! 👍🏻
What do you mean with this? I think it would be a very big benefit for users with only this small change. Also many other projects (no matter if big or very small) are moving to cmake. Feel free to merge this PR whenever you want to use cmake as well. |
Unfortunately I don't see (for personal tastes) mass-moving to CMake a good thing. So what I meant is that people willing to use CMake, should be able to integrate Linenoise with CMake easily. I find it strange that a build system requires libraries to explicitly adapt to it. |
Out of curiosity: I would expect that there is at least some library of linenoise which I can link. |
We ship this pkg-config file in the nixpkgs (NixOS) version, fwiw: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/linenoise/linenoise.pc.in On the other hand, I'd expect most distros wouldn't need to ship anything: you'd put If something like this were to be upstreamed, I'd strongly support pkg-config over CMake -- a plain Makefile-based project can much more easily depend on a library that uses pkg-config than one that uses CMake. |
Because the Makefile is just an example of integration with a toy program. You build it with make and can play with the library in 2 seconds. The library is not built because linenoise "default" usage is to include a copy of it inside your project. It's a single C file, no dependencies, and so forth. Of course who have other needs can take the C file and package it in other ways. But in my opinion this is outside the goals of the project itself. |
|
||
include(GNUInstallDirs) | ||
|
||
option(LINENOISE_BUILD_EXAMPLE OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second argument is for a documentation string.
# ----------------- | ||
# LIBRARY | ||
# ----------------- | ||
add_library(${PROJECT_NAME} linenoise.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not abuse PROJECT_NAME
like this.
# ----------------- | ||
add_library(${PROJECT_NAME} linenoise.c) | ||
target_include_directories(${PROJECT_NAME} | ||
INTERFACE $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/linenoise> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use install(TARGETS ... INCLUDES DESTINATION)
instead for specifying includes for the install.
target_compile_options(${PROJECT_NAME} | ||
PRIVATE -Wall -W | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't hardcode toolchain flags. These can be provided from the command line or (more conveniently for developers) a preset.
install (TARGETS ${PROJECT_NAME} | ||
EXPORT ${PROJECT_NAME}Config | ||
DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You set the minimum to be 3.4, but this command invocation relies on defaults provided only by 3.14.
Spell out the RUNTIME
, LIBRARY
and ARCHIVE
destinations or raise the minimum requirement.
cmake_minimum_required(VERSION 3.4) | ||
project(linenoise C) | ||
|
||
include(GNUInstallDirs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefer to put things closer to where they are used.
# ----------------- | ||
# INSTALLATION | ||
# ----------------- | ||
if(TARGET ${PROJECT_NAME}-example) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When and why does an example need to be installed?
The size of a project is absolutely irrelevant when it comes to CMake. The point of providing CMake build tooling that also installs a CMake package that can be found by
CMake makes that possible certainly, but it's also better to have the instructions be written once instead of N times. You also have to figure out how to acquire the project now. If the project already provides a CMake package, there is no doubt how to package and consume the project. |
) | ||
|
||
install(EXPORT ${PROJECT_NAME}Config | ||
NAMESPACE ${PROJECT_NAME}:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add an ALIAS
target that matches this name.
Understand your point of view. I don't agree. This library wants to have zero involvement with building systems and the software "supply chain". So I'll refrain from commenting more :) Peace & Love. |
Simplify cross compilation and integration into other build systems by using cmake.
Additionally simplify usage within other cmake projects by providing a cmake EXPORT (
find_package(linenoise)
)