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

[workspaces] Support for the cmake_paths generator #3302

Closed
AlessandroA opened this issue Aug 2, 2018 · 15 comments
Closed

[workspaces] Support for the cmake_paths generator #3302

AlessandroA opened this issue Aug 2, 2018 · 15 comments

Comments

@AlessandroA
Copy link

Conan version: 1.6.1

Hi, it would be nice if workspaces had a specialized behaviour for the
cmake_paths generator.

The issue with find_package

cmake_paths is useful to keep build and packaging systems separate because it
allows us to use find_package commands as we would do without conan.

The issue is that find_package does not work well with add_subdirectory. The
recommended solution is to re-map find_package from the top-level CMakeLists
file so that it becomes a no-op when the package has been added as a sub-project
(see Daniel Pfeifer's talk at 50m30s).

Feature request

When using workspaces with cmake_paths, create a
top-level CMakeLists that defines a list of sub-projects and re-defines
find_package for them.

The top-level CMakeLists file could look something like this:

cmake_minimum_required(VERSION 3.3)
project(MyProject CXX)

# Add this #######################
set(AS_SUBPROJECT
    HelloA
    HelloB
    HelloC
)

macro(find_package)
    if(NOT "${ARGV0}" IN_LIST AS_SUBPROJECT)
        _find_package(${ARGV})
    endif()
endmacro()
# ###############################

add_subdirectory(HelloA "/home/user/code/build/helloa")
add_subdirectory(HelloB "/home/user/code/build/hellob")
add_subdirectory(HelloC "/home/user/code/build/helloc")

One could maintain their own top-level CMakeLists file, but conan helps a lot
because the order of the add_subdirectory commands is important.

Not sure if this would be consistent with your idea of workspaces and the use of
cmake_paths!

@lasote
Copy link
Contributor

lasote commented Aug 15, 2018

Hi @AlessandroA, this is very interesting, thanks. We will have a look.

@Lawrencemm
Copy link

I believe this would break my project.

My solution to this problem is to always link against the interface target generated by a FindXXX.cmake script whether my executable is in a workspace or not.
See here:
https://github.com/Lawrencemm/workspace

lmeditor consumes lmlib as a package requirement.

Making find_package a no-op would break my project I think.

@Lawrencemm
Copy link

Lawrencemm commented Mar 20, 2019

I have a different idea for supporting cmake_paths workspace generator.

When specifying cmake_paths as the workspace generator, the generated conanworkspace.cmake would look like this:

set(PACKAGE_foo_SRC "...")
set(PACKAGE_foo_BUILD "...")
set(PACKAGE_bar_SRC "...")
set(PACKAGE_bar_BUILD "...")
macro(conan_workspace_subdirectories)
    set(CMAKE_PROJECT_foo_INCLUDE ${PACKAGE_foo_BUILD}/conan_paths.cmake)
    set(CMAKE_PROJECT_bar_INCLUDE ${PACKAGE_bar_BUILD}/conan_paths.cmake)
    add_subdirectory(${PACKAGE_foo_SRC} ${PACKAGE_foo_BUILD})
    add_subdirectory(${PACKAGE_bar_SRC} ${PACKAGE_bar_BUILD})
endmacro()

The only difference is the addition of the setting of CMAKE_PROJECT_foo_INCLUDE variables.

@lasote
Copy link
Contributor

lasote commented Mar 25, 2019

Sounds very good. Did you try?

@Lawrencemm
Copy link

Success! I tried it by manually hacking conanworkspace.cmake, FindXXX was failing before, after adding the project include variables manually the packages were found.

There are some things that are really important to note, however.

  1. This will only work if project name in the CMakeLists.txt of each package has the same name as the package itself. (spent a bit of time debugging that one 😅). This might be overcome with an override in the workspace yml file.

  2. The include variables should only be defined for packages that have the cmake_paths generator set. Otherwise, the inclusion will obviously fail as the cmake_paths.cmake file won't be there.

@lasote
Copy link
Contributor

lasote commented Mar 25, 2019

Mhh, after taking a look at this: #4808
Maybe what you are expecting from a cmake_paths generator is only adjust the CMAKE_MODULE_PATH and CMAKE_PREFIX_PATH to the [buildirs] directories from the projects in the workspace. You could assume that there, CMake will find some FindXXX.cmake or ConfigXXX.cmake files. That would be useful when the projects export their own targets.
WDYT?

@lasote
Copy link
Contributor

lasote commented Mar 25, 2019

To consider: If you edit a file in a subproject, how well the root project is able to "detect" it and recompile if needed etc. It exceeds my knowledge. I would need to try, so I'm tagging it to take a look to get more knowledge to decide what/when to implement.

@lasote lasote added this to the 1.15 milestone Mar 31, 2019
@lasote lasote added the whiteboard Put in common label Mar 31, 2019
@mbodmer
Copy link

mbodmer commented Apr 1, 2019

What about ExternalProject_Add() to create the superbuild project? ... as I suggested here: #3034 (comment)

@jgsogo
Copy link
Contributor

jgsogo commented Apr 9, 2019

Really interesting thread 👍

Some of the alternatives proposed here are very related to some POC I did in #4879. In this POC the standard CMakeLists.txt for any library file looks like this:

conan_basic_setup(TARGETS)

add_library(mylib ...)
target_link_libraries(mylib PUBLIC CONAN_PKG::mydep)

Here CMake/Conan machinery is creating an imported target called CONAN_PKG::mydep (it would be mydep::mydep with find package approach) that I'm adding as a dependency of my project.

But when I'm working in a workspace these CMakeLists.txt are being added to the parent project using the add_subdirectory() and I no longer want to link with the imported target but to the target itself, otherwise CMake won't build the correct graph and some issues could arise.

To bypass it and keep my project files the same we need to do something different, like some of you are proposing in this issue. In my POC I don't need the imported target anymore, CONAN_PKG::mydep should be the target itself, so I can create an ALIAS target (no need to modify my target_link_libraries line). The generated conanworkspace.cmake could look like the following:

set(PACKAGE_mydep_SRC "path/to/mydep/src")
set(PACKAGE_mylib_SRC "path/to/mylib/src")

macro(conan_workspace_subdirectories)
    set(CONAN_IS_WS TRUE)
    
    add_subdirectory(${PACKAGE_mydep_SRC} "mydep/build/folder")
    add_library(CONAN_PKG::mydep ALIAS mydep)  # for cmake generator

    add_subdirectory(${PACKAGE_mylib_SRC} "mylib/build/folder")    
    add_library(CONAN_PKG::mylib ALIAS mylib)  # for cmake generator
endmacro()

And also, it could make sense to no-op all the conan_basic_setup (or find_package) calls for the packages included in the workspace. With the find_package approach it is easy as @AlessandroA suggested, for the conan_basic_setup one it would require a conan_workspace_basic_setup call in the root CMakeLists.txt of the workspace to do the Conan magic.


The PR and this comment is just a draft, I haven't tried it with cmake_paths or cmake_find_package generators, but I think that the problem is almost always the same: changing the imported target by the actual one for the libraries in the workspace.

@lasote lasote removed this from the 1.15 milestone Apr 9, 2019
@lasote
Copy link
Contributor

lasote commented Apr 9, 2019

We have still some things to discuss about this so no time enough for 1.15. I will reconsider it for 1.16

@kenfred
Copy link

kenfred commented Oct 1, 2019

Here CMake/Conan machinery is creating an imported target called CONAN_PKG::mydep (it would be mydep::mydep with find package approach) that I'm adding as a dependency of my project.

But when I'm working in a workspace these CMakeLists.txt are being added to the parent project using the add_subdirectory() and I no longer want to link with the imported target but to the target itself, otherwise CMake won't build the correct graph and some issues could arise.

As I mentioned in #5762, I think we should use externalproject_add and not add_subdirectory. I don't think you gain anything by avoiding the imported targets. I believe the concern about CMake not building the correct graph is mitigated by using the dependency mechanisms provided by externalproject_add.

Although I like that add_subdirectory will result in a nice makefile or Visual Studio project with all the source files there, you lose several things by doing this:

  • You lose an opportunity to run conan build, package, or test from the super project. With externalproject_add you run these as commands. You could potentially avoid handling special, alternate layouts due to dependencies reaching into build directories instead of package directories.
  • You're limiting the workspace packages to CMake projects only. With externalproject_add you could have a variety of build systems under one super build. A cmake workspace generator shouldn't have to require the projects all be cmake.
  • Unifying the cmake configure across the packages could be bad! There could be conflicting targets, redefinition of CMake variables, etc. In other words, you risk affecting a package's build by joining its cmake configuration with other packages.
  • You have to conditionally do the Conan setup or find_package because with add_subdirectory you're changing how targets are defined. I shouldn't need to modify CMakelists.txt to use workspaces. So the cmake workspace generator should do externalproject_add to isolate the builds, providing module paths, toolchains, etc for the individual builds.

@Hopobcn
Copy link
Contributor

Hopobcn commented Mar 13, 2021

Using ExternalProject_Add offers also some challenges:

  • cmake configure step is deferred until the superbuild target is called. This could break projects that have targets depending on other workspace targets provided by conan
  • Conan should provide the dependency graph via cmake DEPENDS

but also some benefits:

  • multiple toolchains could be mixed (ideal for using with CMakeToolchain)

Something like this: (didn't work in my case)

include(ExternalProject)

set(PACKAGE_libA_SRC "path/to/libA/.")
set(PACKAGE_libA_BUILD "path/to/libA/build/Release")
set(PACKAGE_libB_SRC "path/to/libB/.")
set(PACKAGE_libB_BUILD "path/to/libB/build/Release")
set(PACKAGE_libC_SRC "path/to/libC/.")
set(PACKAGE_libC_BUILD path/to/libC/build/Release")
set(PACKAGE_libD_SRC "path/to/libD/.")
set(PACKAGE_libD_BUILD "path/to/libD/build/Release")
set(PACKAGE_App1_SRC "path/to/App1/.")
set(PACKAGE_App1_BUILD "path/to/App1/build/Release")

macro (conan_add_package)
    set(oneValueArgs NAME SRC BUILD)
    set(multiValueArgs DEPENDS)
    cmake_parse_arguments(CONAN_ADD_PACKAGE "" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
    if (NOT CONAN_ADD_PACKAGE_NAME)
        message(FATAL_ERROR "You must provide a name")
    endif ()
    if (NOT CONAN_ADD_PACKAGE_SRC)
        message(FATAL_ERROR "You must provide a source dir")
    endif ()
    if (NOT CONAN_ADD_PACKAGE_BUILD)
        message(FATAL_ERROR "You must provide a build dir")
    endif ()
    if (NOT CONAN_ADD_PACKAGE_DEPENDS)
        message(WARNING "You must provide all dependencies")
    endif ()

    message("Adding ${CONAN_ADD_PACKAGE_NAME}")

    ExternalProject_Add(${CONAN_ADD_PACKAGE_NAME}
        PREFIX ${CONAN_ADD_PACKAGE_NAME}
        SOURCE_DIR ${CONAN_ADD_PACKAGE_SRC}
        BINARY_DIR ${CONAN_ADD_PACKAGE_BUILD}
        CMAKE_ARGS -DCMAKE_TOOLCHAIN_FILE=${CONAN_ADD_PACKAGE_BUILD}/conan_toolchain.cmake
        DEPENDS ${CONAN_ADD_PACKAGE_DEPENDS}
    )
endmacro ()

macro(conan_workspace_subdirectories)
    conan_add_package(NAME libA SRC ${PACKAGE_libA_SRC} BUILD ${PACKAGE_libA_BUILD} DEPENDS "")
    conan_add_package(NAME libB SRC ${PACKAGE_libB_SRC} BUILD ${PACKAGE_libB_BUILD} DEPENDS libA)
    conan_add_package(NAME libC SRC ${PACKAGE_libC_SRC} BUILD ${PACKAGE_libC_BUILD} DEPENDS libA)
    conan_add_package(NAME libD SRC ${PACKAGE_libD_SRC} BUILD ${PACKAGE_libD_BUILD} DEPENDS libB libC)
    conan_add_package(NAME App1 SRC ${PACKAGE_App1_SRC} BUILD ${PACKAGE_App1_BUILD} DEPENDS libD)
endmacro()

But I'm not sure ExternalProject_Add is a good idea. In my company, we are using it and it's a pain in the ass. Once you start using it there's no way back. And that is never good.

To be honest, I would prefer conan workspace to stick to add_subdirectory. But generating FindXXX.cmake and conan_toolchains.cmake in the workspace build directory and not in the build directories from each component.

@davidtazy
Copy link

Hi, in my company, we are coming from superbuild and we try to "conanify" our componants and so use conan workspace feature for the development task.
The migration process cannot be instantaneous , So our componant needs to operate in superbuild (without conan) and in standalone mode( with conan)

we are "believing" transparent usage of conan in cmake, so we are using cmake_paths and cmake_find_packages generators.

In the link below , I modified the workspace example to demonstrate our usage.

I also added a dependancy to a "third part" (zlib). For us, third part are "read only" componant. so they will never be modified in conan workspace.

davidtazy/examples@c5ffc9c

i will be glad to have some feedback... its seems working....

@kenfred
Copy link

kenfred commented Oct 13, 2021

@davidtazy
I have also prototyped a workspaces with ExternalProject and find it to be nice.

@Hopobcn

cmake configure step is deferred until the superbuild target is called. This could break projects that have targets depending on other workspace targets provided by conan

Why is this a challenge. The outer super build happens to be cmake, but otherwise isn't related to the build systems of the packages. In fact, this is the key to enable workspaces with heterogeneous build systems. The cmake-based super build is executed with a cmake --build. In my case it will call conan install + conan build + conan package all via ExternalProject phases. Dependents find the build output in the package dir, just like it would from the conan cache. This works great!

Conan should provide the dependency graph via cmake DEPENDS

Why is this a challenge? This is why cmake + ExternalProject is a good candidate for the super build: it already has the DEPENDS mechanism, which conan could easily set in a workspaces generator. We get package-level incremental builds for free! This "workspaces generator" for ExternalProject is the missing piece I want from conan, and a simple yet comprehensive way to do workspaces.

To be honest, I would prefer conan workspace to stick to add_subdirectory

If so, then all projects must be cmake-based. That doesn't seem acceptable in the mission of conan. It also allows cmake scope to bleed together - no proper separation of the package builds.

@memsharded
Copy link
Member

This is outdated, cmake_paths generator has been replaced by CMakeToolchain, and workspaces removed in 2.0, to be resumed in the 2.X roadmap. Closing this ticket as outdated.

workspaces automation moved this from BACKLOG to Done Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
workspaces
  
Done
Development

Successfully merging a pull request may close this issue.

9 participants