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

[feature] CMake generator: custom target #6069

Closed
1 task
uilianries opened this issue Nov 13, 2019 · 17 comments
Closed
1 task

[feature] CMake generator: custom target #6069

uilianries opened this issue Nov 13, 2019 · 17 comments

Comments

@uilianries
Copy link
Member

Hi!

After the feature #5598, which helped a lot with cmake_find_package, now we have another cmake situation which can be solved with a new feature.

I'll try to illustrate using a real case, Protobuf. This package was divided in two, the library (libprotobuf, libprotoc, libprotobuf-lite) and the compiler (.proto parser). The parser can be invoked from Cmake, using macros to generate both headers and sources:

cmake_minimum_required(VERSION 2.8.12)
project(test_package CXX)

find_package(protoc)   # protoc/3.9.1@
find_package(Protobuf) # protobuf/3.9.1@

PROTOBUF_GENERATE_CPP(PROTO_SRCS PROTO_HDRS addressbook.proto)
add_executable(${CMAKE_PROJECT_NAME} "${PROTO_SRCS}" example.cpp)
target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_BINARY_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} PUBLIC protobuf::protobuf)

Using cmake_find_package, Conan will provide Findprotoc.cmake, which has the CMake target protoc::protoc, and for cmake generator we will have CONAN_PKG::protoc from conanbuildinfo.cmake.

However, those macros are looking for the original target, protobuf::protoc. We could patch those cmake files, but this problem also could be avoided using a custom target:

def cpp_info(self):
    self.cpp_info.target = "protobuf::protoc"

Now the cmake_find_package can use this new custom target name and all cmake files imported from protobuf will work straightforward.

The current PR #5408 is adding new feature to self.cpp_info. Similar to this feature request.

@jgsogo
Copy link
Contributor

jgsogo commented Nov 14, 2019

I'll tag this as a look into, we need to think if we are requesting this as a useful feature, or it is just something we need for protobuf/protoc because of the way we have created these recipes.

@jgsogo
Copy link
Contributor

jgsogo commented Nov 14, 2019

My opinion about all this stuff related to protoc/protobuf:

I really feel like that the components approach (#5408) is the way to go. Probably all the protobuf libraries should be in the same package (it is the way it is designed) and we would get (with the components feature) targets protobuf::protobuf, protobuf::protoc,... all belonging to the one and only package protobuf. Having a different package protoc introduce the problem you are pointing to. Also, having two different packages for the library and the compiler introduce problems not there before: which package is the owner of the CMake's macros, how to ensure both versions match,... we bot are aware of some of these issues.

Together, the new cross-building model and the components should be enough to target this problem without adding a new feature to rename targets (or rename the namespace for the targets).

Nevertheless, in the generic scenario of cross-compilation (native is just a particular use-case), I'm not sure if we would be able to use protobuf::protoc target for those CMake macros. Maybe in the long run the best (only?) way to go is to compile the .protos before entering CMake... because assigning different toolchains to specific targets inside the CMake project could be too much (protobuf::protobuf for host and protobuf::protoc for build machine).

@uilianries
Copy link
Member Author

It's a trade off, we can put all together, but we can't invoke cmake macro when cross-building. I know there are people interested in this scenario, even I used Protobuf for embedded stuff in the past. Protobuf is not only one, gRPC is on the same boat.

@KerstinKeller
Copy link

As for the special case of Protobuf and cross compiling:

I think for many people, it would be a nogo / step backward to do the proto file compilation before
entering cmake.

If you are cross-compiling, usually you would install the package for both build and target architecture. You would then pass a -DProtobuf_PROTOC_EXECUTABLE=/path/to/my/build/system/protoc. Then the find_package(Protobuf REQUIRED) command would locate your cross build Protobuf libraries, but the cmake scripts will use your build system protoc compiler to do the *.proto file compilations.
Things might just work fine, too, if the build protoc compiler is in the system path.

During the CMake execution it is also asserted that protoc and protobuf library versions match, otherwise CMake will error out.

At least this works with the FindProtobuf.cmake that is distributed with CMake, I don't know about their config mode.

For our own packages that come with a "compiler" such as protoc, we create a pattern such as this

INCLUDE("@PACKAGE_xxx_INSTALL_CMAKE_DIR@/xxxTargets.cmake")
IF (CMAKE_CROSSCOMPILING)
  IF (NOT XXX_COMPILER)
    FIND_PROGRAM(XXX_COMPILER xxxc HINTS ${XXX_COMPILER_PATH_HINT})
  ENDIF () 
  IF (XXX_COMPILER)
    ADD_EXECUTABLE(xxx::xxxc IMPORTED)
    SET_TARGET_PROPERTIES(xxx::xxxc PROPERTIES
      IMPORTED_LOCATION "${XXX_COMPILER}"
    )  
  ELSE ()
    MESSAGE(ERROR "You are cross-compiling but xxx needs to locate the xxxc program on your host system. Make sure it is installed and set the XXX_COMPILER_PATH_HINT variable.")
  ENDIF ()  
ELSE ()
  INCLUDE("@PACKAGE_xxx_INSTALL_CMAKE_DIR@/xxxcTargets.cmake")
ENDIF ()

What this means is, when you do a find_package, the targets are always included. The targets for the compiler are only included when not cross compiling. When cross compiling an imported target is created instead.

@uilianries
Copy link
Member Author

Another related case: conan-io/conan-center-index#405

The CMake file has a name, but the target name in the cmake module is not the same.

@gocarlos
Copy link

packaging RocksDB has the same issue:
upstream is using RocksDB::rocksdb

Currently using RocksDB::RocksDB here:
https://gitlab.com/gocarlos/conan-rocksdb

@madebr
Copy link
Contributor

madebr commented Apr 29, 2020

@jgsogo
I think this feature still makes a lot of sense for protobuf. (conan-io/conan-center-index#1179)

Plus adding a variable that contains the run command would also be nice.
e.g. the following generated code in conanbuildinfo.cmake

set(CONAN_protobuf_RUN_COMMAND "${CMAKE_COMMAND}" -E env "DYLD_LIBRARY_PATH=${Protobuf_LIB_DIRS}:${CONAN_LIB_DIRS}" "${CONAN_protobuf_NAME}")

for macos and something similar for the cmake_find_package(_multi) generator (and other os'es).

Ideally, the interface for cmake and cmake_find_package should be the same such that protobuf's cmake scripts can use it independently of the cmake generator. (cmake, cmake_find_package or cmake_find_package_multi

This is also interesting for python: see https://cmake.org/cmake/help/git-stage/module/FindPython.html

There is currently no way to model Python::Interpreter

@TimSimpson
Copy link
Contributor

I think if it is possible to support this in Conan it should be done, for the simple reason that it's currently possible in pure CMake to make the argument to find_package and the exported target namespace different so inevitably people are going to do that and it would be nice if Conan packages could work interchangeably with other solutions.

For example, consider vcpkg's SDL2-image package. Once it's installed the CMake code is find_package(sdl2-image) with target SDL2::SDL2_image. That example is kind of weird, since BinCrafter's work predates vcpkg, but I could see a collection of related projects, such as a post-modularized Boost, wanting to share the same target namespace across different packages, ie find_package(Boost-coroutine) introducing Boost::coroutine could be followed by find_package(Boost-filesystem) introducing Boost::filesystem.

Could this be supported by adding a new property to self.cpp_info, such as self.cpp_info.cmake_target_namespace='Boost'?

@jgsogo
Copy link
Contributor

jgsogo commented Jun 24, 2020

Some comments in this issue are related to the components feature which has already been released. Using cpp_info.names in combination with cpp_info.compoents["xxx"].names it is possible to generate any CMake target name for the cmake_find_package[_multi] generators:

class Recipe:
    name = "any-name"

    def package_info(self):
        self.cpp_info.names["cmake_find_package"] = "Boost"
        self.cpp_info.components["not-exactly-coroutine"].names["cmake_find_package"] = "coroutine"

This will generate target Boost::coroutine for the cmake_find_package generator. Of course, no need to override the names if the package-name or the component-name already matches the desired name.

Am I missing something? IMO, this issue can be closed after all the effort (and more to come) related to components


@madebr I cannot see how this is related to the first part of your comment about protobuf run command.

@madebr
Copy link
Contributor

madebr commented Jun 24, 2020

@jgsogo
A xxx_EXECUTABLE variable/target in cmake is mostly used in execute_process, add_custom_command and add_custom_target. If the DYLD_LIBRARY_PATH is not included, it won't work on apple.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 24, 2020

But, do you think this is something to add in the self.cpp_info object or, as something related only to CMake, it is something that should go in a build_module.cmake? Those files are automatically included when using any CMake generator (you know it well, it is how the protobuf macros/functions are added).

@madebr
Copy link
Contributor

madebr commented Jun 24, 2020

I can only think of cmake as its consumer + using it in the dependency graph alongside library components.

I think the build_module should be able to use the conan generated variables and targets.
For the protobuf recipe, I had to do a lot of fiddling in the cmake script.
Using this future feature, I could just have used protobuf::protoc in the add_custom_command.

Also, on MSVC, executables can have different suffixes for different build types (same for libraries).
Having this information centralized in the recipe would be nice.

I just think all types of targets (that are installed in general) should be addable to the dependency graph.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 24, 2020

Now I see, it is tightly related to #7240

@TimSimpson
Copy link
Contributor

@jgsogo

       self.cpp_info.names["cmake_find_package"] = "Boost"
       self.cpp_info.components["not-exactly-coroutine"].names["cmake_find_package"] = "coroutine"

This will work fine for 1 package and produce FindBoost.cmake. However you then can't use Boost:: from a second package.

For example, suppose in the future Boost is finally modularized in a way that gains mass acceptance. There may exist a Conan package for Boost-coroutine and another one for Boost-asio. From CMake these would be used by calling find_package(Boost-coroutine), which would bring in target Boost::coroutine, and find_package(Boost-asio) which would bring in target Boost::asio.

The solution you outline does not work if self.cpp_info.names["cmake_find_package"] = "Boost" is set from multiple packages. The generator will create one single FindBoost.cmake script which will contain information on only one package's targets, not both of them (so for instance, Boost::coroutine might be defined there, but asio would not be mentioned, or asio would be defined and coroutine omitted), making one of the packages impossible to access from CMake.

@jgsogo
Copy link
Contributor

jgsogo commented Jun 25, 2020

I see what you mean. Your example is valid, also protobuf right now: the file is FindProtobuf.cmake while the namespace is protobuf::, different casing in a quite common package https://github.com/Kitware/CMake/blob/master/Modules/FindProtobuf.cmake

@danimtb
Copy link
Member

danimtb commented Jul 29, 2020

@uilianries anything else need regarding filenames after having #7254 implemented?

@uilianries
Copy link
Member Author

@danimtb No, we are good to go.

@danimtb danimtb added this to the 1.28 milestone Jul 29, 2020
@danimtb danimtb closed this as completed Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants