Recent changes broke CMake

Question

Recent changes broke CMake

DNKpp opened this issue a year ago · 14 comments

As already stated in the issue #126, there are some configuration issues. Building on msvc (as cmake project) is fine, but when I do remote compiling on my ubuntu runner, a similar error appears.

CMake Error in build/_deps/flux-src/CMakeLists.txt:
  Target "flux" INTERFACE_SOURCES property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the build directory.


CMake Error in build/_deps/flux-src/CMakeLists.txt:
  Target "flux" INTERFACE_SOURCES property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the build directory.Target "flux" INTERFACE_SOURCES
  property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the source directory.

Going back the history, it seems that a00f83b introduces the error for me. Until then, everything works fine.

Answer 1 · 2023-10-09T16:16:07.000Z

Hi @DNKpp, thanks for the bug report and finding the commit that introduced it.

I don't seem to be able to reproduce this locally (I'm on MacOS) which unfortunately makes it a bit hard to investigate! Are you able to give me any more details of what's triggering the error?

I think I must be misunderstanding something because I was under the impression that the line

file(GLOB_RECURSE FLUX_HPPS RELATIVE "${CMAKE_CURRENT_SOURCE_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}/include/*.hpp" )

would make all of the paths in the FLUX_HPPS list relative to the current source dir, so I don't get what CMake is complaining about here

Answer 2 · 2023-10-09T18:06:53.000Z

Hi,
I must admit, that I simply copied the message from the original author, mine is slightly different.
It only happens when I add flux as a dependency via cmake target_link_libraries and compiling on my remote ubuntu machine. Compiling locally (with vs2022) works.
My message is:

1> [CMake] CMake Error in /home/dnkpp/.vs/flux_test/6bd237ac-1d21-467b-addd-45d21113d78a/out/build/Linux-GCC-12-Debug/_deps/flux-src/CMakeLists.txt:
1> [CMake]   Target "flux" INTERFACE_SOURCES property contains path:
1> [CMake] 
1> [CMake]     "/home/dnkpp/.vs/flux_test/6bd237ac-1d21-467b-addd-45d21113d78a/out/build/Linux-GCC-12-Debug/_deps/flux-src/"
1> [CMake] 
1> [CMake]   which is prefixed in the build directory.

Edit: No, it also doesn't work locally. I'm using CPM for managing packages and usually utilize a source cache in one of my drives, which is then of course outside of the build tree. I've temporarily disabled the source cache for that project and it then also fails locally. In fact <build-dir>/_deps/flux-src is inside the build dir, which AFAIK is the usual location for dependencies pulled via fetch_content.

Answer 3 · 2023-10-09T19:02:15.000Z

I've tried it again with the previous commit and saw this warning:

1> [CMake] CMake Warning (dev) at out/build/x64-Debug/_deps/flux-src/CMakeLists.txt:13 (target_sources):
1> [CMake]   Policy CMP0076 is not set: target_sources() command converts relative paths
1> [CMake]   to absolute.  Run "cmake --help-policy CMP0076" for policy details.  Use
1> [CMake]   the cmake_policy command to set the policy and suppress this warning.
1> [CMake] 
1> [CMake]   An interface source of target "flux" has a relative path.
1> [CMake] This warning is for project developers.  Use -Wno-dev to suppress it.

This one is disabled in the next commit and thus raising the error. So, it's indeed the line which causes the error/warning, but the applied policy is responsible for the actual error.
flux_test.zip

Answer 4 · 2023-10-11T09:32:43.000Z

Hmm, cmake --help-policy CMP0076 says:

The target_sources() command converts relative paths to absolute.

In CMake 3.13 and above, the target_sources() command now converts
relative source file paths to absolute paths in the following cases:

Source files are added to the target's INTERFACE_SOURCES
property.

The target's SOURCE_DIR property differs from
CMAKE_CURRENT_SOURCE_DIR.

A path that begins with a generator expression is always left unmodified.

This is the new behaviour for CMake 3.13+, so it seems like what we want? But I don't really understand what's causing the error -- especially since this policy says it doesn't affect generator expressions, which it seems like we're using?

To add to the confusion, apparently vcpkg has recently started shipping Flux (yay!). But they are carrying a "fixup targets" patch which strongly suggests that we're doing something wrong in Flux's CMake config.

Unfortunately this is all way past the limit of my CMake knowledge, and I think I'm going to need some assistance in understanding what the actual problem is and how to solve it. Any suggestions would be greatly appreciated!

Answer 5 · 2023-10-11T12:37:51.000Z

Hey,
I wish I could help, but my CMake is also just ok-ish. What's your actual intention? Perhaps we can work out a better alternative together :)

Answer 6 · 2023-10-11T12:50:58.000Z

This was introduced in #123 with the goal of being able to make install the library (i.e. copying the headers to the right place) and also have a CMake flux::flux library target that people could use with target_link_libraries().

This doesn't seem like it should be a tremendously tricky thing to do -- I'd imagine just about every library is going to want to do the same thing! -- but apparently my CMake skills aren't up to the task. Probably the best thing to do is for me to have a look at how some other header-only libraries manage it and just copy them.

Answer 7 · 2023-10-17T15:55:03.000Z

Hey, sry for the delay.
As I said, I'm just ok'ish with CMake. I've started reading the book "Modern CMake for C++" a while ago, but completely skipped the install and export topics. Over the last few days I've read them and think have an basic understanding, what to do and where the issue is:

First of all, you said something about the target flux::flux, which irritated me a bit, because I'm never using dependencies in that way (installing them on the system). I think, there are two distinct ways, a header-only lib should be linked into a project:

via cmake --install somewhere central on the system
via fetch_content_... relative to a specific project

Let's start with the second approach:
The two issues with your current CMakeLists.txt are, the already mentioned target_sources statement and a missing flux::flux alias target (which isn't required, but AFAIK good style).

The install approach is working, but can be slightly improved. It's probably best when I provide an adjusted CMakeLists.txt. That's the best I can come up with, but it should be a good baseline for a discussion about what's good practice and what not.

cmake_minimum_required(VERSION 3.5)
cmake_policy(SET CMP0076 NEW) # remove warning for target_sources(flux...) for < 3.13
cmake_policy(SET CMP0092 NEW) # remove warning for CMAKE_LANG_FLAG MSVC for < 3.15

project(libflux CXX)

include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

# you manually appended the list; can be simplified like this
list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake")

set(PORT_NAME flux)

add_library(flux INTERFACE)
# this adds a flux::flux alias target for the users, who directly add the whole project as dependency (e.g. by ``fetch_content_...``)
add_library(${PORT_NAME}::flux ALIAS flux)

# your if/else branches can be "simplified" (some may say it's not simpler, but at least more modern) with generator expressions
target_compile_features(
    flux
    INTERFACE
    $<IF:$<CXX_COMPILER_ID:MSVC>,cxx_std_23,cxx_std_20>
)

target_compile_options(
    flux
    INTERFACE
    $<$<CXX_COMPILER_ID:MSVC>:/permissive-> 
)

target_include_directories(
    flux 
    INTERFACE
        # paths should always be wrapped in ""
        "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
        "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
)

set(CMAKE_CXX_EXTENSIONS Off)

# that's just my solution. Keep it or throw it away. The idea is, to opt-out the examples/tests/etc when flux itself is the root project
# and opt-in if linked as dependency
if (CMAKE_SOURCE_DIR STREQUAL libflux_SOURCE_DIR)
	set(IS_ROOT_PROJECT TRUE)
else()
	set(IS_ROOT_PROJECT FALSE)
endif()

option(FLUX_BUILD_DOCS "Build Flux documentation (requires Sphinx)" Off)
option(FLUX_BUILD_EXAMPLES "Build Flux examples" ${IS_ROOT_PROJECT})
option(FLUX_BUILD_TESTS "Build Flux tests"  ${IS_ROOT_PROJECT})
option(FLUX_BUILD_BENCHMARKS "Build Flux benchmarks"  ${IS_ROOT_PROJECT})
option(FLUX_BUILD_TOOLS "Build single-header generator tool" Off)
option(FLUX_BUILD_MODULE "Build C++20 module (experimental)" Off)
option(FLUX_ENABLE_ASAN "Enable Address Sanitizer for tests" Off)
option(FLUX_ENABLE_UBSAN "Enable Undefined Behaviour Sanitizer for tests" Off)

# at least from my knowledge, vars should not be wrapped in ${} inside of if statements, as this will have subtile differences of the evaluation
if (FLUX_BUILD_DOCS)
    add_subdirectory(docs)
endif()

if (FLUX_BUILD_EXAMPLES)
    enable_testing()
    add_subdirectory(example)
endif()

if (FLUX_BUILD_BENCHMARKS)
    add_subdirectory(benchmark)
endif()

if (FLUX_BUILD_TESTS)
    enable_testing()
    add_subdirectory(test)
endif()

if (FLUX_BUILD_TOOLS)
    add_subdirectory(tools)
endif()

if (FLUX_BUILD_MODULE)
    add_subdirectory(module)
endif()

set(LIB_INSTALL_DIR "${CMAKE_INSTALL_LIBDIR}/${PORT_NAME}")

# header-only doesn't need architeture differences so clear CMAKE_SIZEOF_VOIDP
# temporarily when creating the version file.
set(ORIGINAL_CMAKE_SIZEOF_VOIDP ${CMAKE_SIZEOF_VOIDP})
set(CMAKE_SIZEOF_VOIDP "")
write_basic_package_version_file(
    "${PORT_NAME}-version.cmake"
    VERSION -1 # When there is a PROJECT_VERSION, remove this line
    COMPATIBILITY SameMajorVersion
    # ARCH_INDEPENDENT # showed up in CMake 3.14 and gets rid of the need to do the CMAKE_SIZEOF_VOIDP thing
)
set(CMAKE_SIZEOF_VOIDP ${ORIGINAL_CMAKE_SIZEOF_VOIDP})

configure_package_config_file(
    "${CMAKE_CURRENT_SOURCE_DIR}/cmake/${PORT_NAME}-config.cmake.in"
    "${CMAKE_CURRENT_BINARY_DIR}/${PORT_NAME}-config.cmake"
    INSTALL_DESTINATION "${LIB_INSTALL_DIR}/cmake"
    PATH_VARS LIB_INSTALL_DIR
)

# set target installation location properties and associates it with the targets files
install(
    TARGETS ${PORT_NAME}
    EXPORT ${PORT_NAME}-targets
    ARCHIVE
    PUBLIC_HEADER DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${PORT_NAME}"
)

# install the headers
install(
    DIRECTORY "include/"
    TYPE INCLUDE
    FILES_MATCHING PATTERN "*.hpp"
)

#install the targets files
install(
    EXPORT ${PORT_NAME}-targets
    DESTINATION "${LIB_INSTALL_DIR}/cmake"
    NAMESPACE ${PORT_NAME}::
)

# install the config and version files
install(
    FILES
        "${CMAKE_CURRENT_BINARY_DIR}/${PORT_NAME}-config.cmake"
        "${CMAKE_CURRENT_BINARY_DIR}/${PORT_NAME}-version.cmake"
    DESTINATION "${LIB_INSTALL_DIR}/cmake"
)

And a rewrite of your flux-config.cmake.in:

@PACKAGE_INIT@

set_and_check(@PORT_NAME@_LIB_DIR "@PACKAGE_LIB_INSTALL_DIR@")
include("${@PORT_NAME@_LIB_DIR}/cmake/@PORT_NAME@-targets.cmake")

check_required_components(@PORT_NAME@)

I've performed some test with both approaches and they seem fine. But as I've never actually used the install approach myself in practice, someone should have a look, if it's working the intended way.

Hopefully this helps :)

Answer 8 · 2023-10-20T13:32:14.000Z

@tcbrindle do you have any questions or concerns?

Answer 9 · 2023-12-07T17:42:50.000Z

Fwiw, whenever I do a cmake .. in my project from scratch, I always get a:

CMake Error in build/_deps/flux-src/CMakeLists.txt:
  Target "flux" INTERFACE_SOURCES property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the build directory.


CMake Error in build/_deps/flux-src/CMakeLists.txt:
  Target "flux" INTERFACE_SOURCES property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the build directory.Target "flux" INTERFACE_SOURCES
  property contains path:

    "/home/cph/parrot/build/_deps/flux-src/"

  which is prefixed in the source directory.

and it tells me CMake fails even though I'm able to successfully make afterwards. Haven't figured out how to make the erroneous error disappear.

Answer 10 · 2023-12-07T20:54:18.000Z

After chatting with chat-gpt for a while and determining that nothing was wrong with the flux CMakelists.txt, it said something must be wrong with my CMakelists.txt. Previously I had:

# --- Fetch flux --------------------------------------------------------------

FetchContent_Declare(flux
GIT_REPOSITORY https://github.com/codereport/flux
GIT_TAG main
)

FetchContent_GetProperties(flux)
if(NOT flux_POPULATED)
FetchContent_Populate(flux)
add_subdirectory(${flux_SOURCE_DIR} ${flux_BINARY_DIR} EXCLUDE_FROM_ALL)
endif()

But when i removed add_subdirectory(${flux_SOURCE_DIR} ${flux_BINARY_DIR} EXCLUDE_FROM_ALL) it resolved my issue : )

Answer 11 · 2024-01-09T14:02:09.000Z

@DNKpp @codereport I've finally gotten around to tackling this in #146, which I've just merged. I'm pretty sure this fixes the problem -- would you be able to give it a try and let me know?

Answer 12 · 2024-01-09T15:24:11.000Z

@DNKpp @codereport I've finally gotten around to tackling this in #146, which I've just merged. I'm pretty sure this fixes the problem -- would you be able to give it a try and let me know?

Had to upgrade CMake, but once I did it worked with line i removed : ) 🙏

Answer 13 · 2024-01-09T16:27:19.000Z

@DNKpp @codereport I've finally gotten around to tackling this in #146, which I've just merged. I'm pretty sure this fixes the problem -- would you be able to give it a try and let me know?

Had to upgrade CMake, but once I did it worked with line i removed : ) 🙏

Yeah, I decided to go with the latest recommended CMake approach which requires a new-ish version, but at least it should be future-proof! Glad to hear it's working -- I think you should be able to use FetchContent_MakeAvailable(flux) now rather than needing to do it manually with FetchContent_GetProperties() etc too

Answer 14 · 2024-01-16T22:26:42.000Z

@DNKpp I'm going to close this issue as I believe it's fixed now. If you have any more problems please let me know, and sorry it took me so long to get to this!