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

Consuming Bond with FindPackage (and using modern CMake) #1013

Open
KerstinKeller opened this issue Nov 27, 2019 · 4 comments
Open

Consuming Bond with FindPackage (and using modern CMake) #1013

KerstinKeller opened this issue Nov 27, 2019 · 4 comments

Comments

@KerstinKeller
Copy link

KerstinKeller commented Nov 27, 2019

I am trying to integrate Bond with our buildsystem to evaluate it properly, but I am running into various problems:

  • Bond installation does not provide a BondConfig.cmake file, which can be consumed by other projects
  • Bond does not export CMake functions to generate code from bond files (though they are defined in cmake/Bond.cmake, like the add_bond_codegen function.
  • Bond exports its targets to bond.cmake (which is good), but they include absolute paths to the boost library. Hence, the bond package is not relocatable, and I cannot distribute a bond package build by a CI server to be consumed by the users.

If bond used modern CMake, imported targets for libraries, code generation functions and provided a BondConfig.cmake file, it would be much easier to integrate.

Would you be interested for me to work on a PR to address this issues? I am not quite sure how much work it will be, I have not yet looked at all of the CMake files for this project yet.

@chwarr
Copy link
Member

chwarr commented Nov 28, 2019

Yes, please submit PRs for these. No one who works on Bond is a CMake expert, so we're happy to have help!

For the add_bond_codegen, we'll want to make sure that the interface that we ship publicly is a sound one, as the Bond project considers build rules part of its interface and we try to minimize breaking changes.

@KerstinKeller
Copy link
Author

Thank you for the feedback!
I totally agree that if bond exposes an API for cmake code generation, that it should be stable (and backwards compatible).

The options that you currently provide for "add_bond_codegen" make a lot of sense, though I don't really get the TARGET one.

Also I'd prefer to add another function, maybe called bond_target_cpp(my_exe [same arguments as add_bond_codegen] file1.bond [fileX.bond]) which basically works similar to add_bond_executable, but the files can be added to any target (e.g. shared / static library, executable...).

Also are you set on the bond generation functions to be called add_bond_.... It would be more in line with other generators (such as protobuf, capnproto), to call the functions bond_generate_<language>.

Also, do you have any requirements on the cmake_minimum_required? Currently it is set to 3.1 but since then CMake has added multiple nice features, especially declaring boost targets in the provided FindBoost.cmake script. Are you set on this version or would it be acceptable to upgrade it? And which version would be acceptable? (Ubuntu 16.04 comes with 3.5.1, 18.04 with 3.9.1 but they now provide ppa's to upgrade, on Windows it needs to be installed manually anyways.).

Also I can just see how far I get, create a PR and we can discuss the changes.

@KerstinKeller
Copy link
Author

I have made a first draft which allows installation / consumption of Bond via CMake.
It's not yet mergeable, but I do have a few question:

  • Are RapidJSON and Boost public dependencies of Bond (e.g. do public headers of bond include both RapidJSON and/or Boost)? (Some of the includes of the samples do, but again I am not sure if they are all part of the public API)
  • Should all Bond examples (from the examples folder) be buildable against a bond installation? Or do some of them rely on Bond internals?
    (e.g. the import example, the file import.bond includes import "bond/core/bond.bond" but this currently raises an error, because it cannot be found.)
  • The info about CMake miminum required version is very important
    Thanks for the feedback 😄

@chwarr
Copy link
Member

chwarr commented Dec 5, 2019

Thank you for your patience. I've been traveling for the Thanksgiving holiday in the United States and am just catching up on things now.

The options that you currently provide for "add_bond_codegen" make a lot of sense, though I don't really get the TARGET one.

The TARGET argument is used to create an explicit codegen target in the build dependency graph. It's currently used, for example, in the CMakeLists.txt for the unit tests, as the tests all share a bunch of .bond files in common. In the unit tests, we generated the C++ files once with different options for some files, then we use them in various unit tests. Without something like this, each unit tests would perform its own code generation. If there's a more CMakey way to do this, I'm all ears.

Also I'd prefer to add another function, maybe called bond_target_cpp(my_exe [same arguments as add_bond_codegen] file1.bond [fileX.bond]) which basically works similar to add_bond_executable, but the files can be added to any target (e.g. shared / static library, executable...).

That makes sense to me. The pattern would be something like this, I assume:

add_library(foo source1.cpp source2.cpp)
bond_target_cpp(foo source3.bond)

add_executable(bar source3.cpp)
target_link_libraries(bar foo)
bond_target_cpp(bar source4.bond)

Would target_bond_cpp fit the CMake naming scheme better?

Also are you set on the bond generation functions to be called add_bond_.... It would be more in line with other generators (such as protobuf, capnproto), to call the functions bond_generate_<language>.

The name can be changed. bond_generate_cpp &c sound good to me.

Also, do you have any requirements on the cmake_minimum_required? Currently it is set to 3.1 but since then CMake has added multiple nice features, especially declaring boost targets in the provided FindBoost.cmake script. Are you set on this version or would it be acceptable to upgrade it? And which version would be acceptable? (Ubuntu 16.04 comes with 3.5.1, 18.04 with 3.9.1 but they now provide ppa's to upgrade, on Windows it needs to be installed manually anyways.).

We still need to stay compatible with Ubuntu 16.04. Raising the minimum to CMake 3.5.1 is fine, but beyond that is a larger discussion where we'd need to weigh pros and cons.

Are RapidJSON and Boost public dependencies of Bond (e.g. do public headers of bond include both RapidJSON and/or Boost)? (Some of the includes of the samples do, but again I am not sure if they are all part of the public API)

Bond requires Boost to be findable on the system both to build Bond and to consume Bond. A CMake install of Bond should not install Boost. Similarly for RapidJSON. However, historically, RapidJSON has been provided by Bond via a submodule and its headers have been installed by a CMake install, unless -DBOND_FIND_RAPIDJSON=TRUE is set. We should preserve that behavior.

Should all Bond examples (from the examples folder) be buildable against a bond installation? Or do some of them rely on Bond internals?
(e.g. the import example, the file import.bond includes import "bond/core/bond.bond" but this currently raises an error, because it cannot be found.)

None of the examples rely on Bond internals. The import example's import "bond/core/bond.bond" line is how that file should be imported in a consuming project. This probably means that bond_generate_cpp needs to make sure that the installed location of bond/core/bond.bond is added to gbc's search path when its being used from an installed version.

The C# MSBuild code generation targets have a similar pattern using a MSBuild variable BOND_INCLUDE_PATH:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants