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

Add CMake install commands for Au #265

Merged
merged 6 commits into from
Jul 16, 2024
Merged

Conversation

chiphogg
Copy link
Contributor

@chiphogg chiphogg commented Jul 12, 2024

We may have gotten the project to build under CMake, but other
projects can't actually use it unless we provide install commands.

First, we add an install(TARGETS...) command (based on the
GNUInstallDirs module) to the header_only_library() function. We
install it as part of an export set, whose name we set with a global
variable (which must be defined before we load this file).

The tricky bit is that this can't work unless the actual code itself is
in a subdirectory of the repo, not the root folder, for reasons I only
dimly understand. To make this work, we now provide the Au code via a
symlink, which we create inside of a new cmake/project_symlinks
folder. This lets us refer to this folder in the BASE_DIRS variable.

In the main CMake file, we add the installation command for the export
set we've been building up. We also write and install the package
config and version files.

The above changes would break bazel support, because there would be
BUILD files reachable underneath cmake, both via the au symlink, and
via cmake/build. To fix this, we make cmake/ a fake local
repository, causing bazel to skip it.

To test, create a new project with two files:

main.cc:

#include "au/au.hh"

int main(int argc, char **argv) { return 0; }

CMakeLists.txt:

cmake_minimum_required(VERSION 3.29)

project(CppUnitsCompare)

include(FetchContent)
FetchContent_Declare(
  Au
  GIT_REPOSITORY https://github.com/aurora-opensource/au
  GIT_TAG "chiphogg/cmake-install#215"
)
FetchContent_MakeAvailable(Au)

add_executable(CppUnitsCompare main.cpp)
target_link_libraries(CppUnitsCompare PUBLIC Au::au)

And execute:

cmake -S . -B build
cmake --build build

Helps #215.

To test, create a new project with two files:

`main.cc`:

```cpp

int main(int argc, char **argv) { return 0; }
```

`CMakeLists.txt`:

```cmake
cmake_minimum_required(VERSION 3.29)

project(CppUnitsCompare)

include(FetchContent)
FetchContent_Declare(
  Au
  GIT_REPOSITORY file:///home/chogg/au
  GIT_TAG "chiphogg/cmake-install#215"
)
FetchContent_MakeAvailable(Au)

add_executable(CppUnitsCompare main.cpp)
target_link_libraries(CppUnitsCompare PUBLIC Au::au)
```

And execute:

```sh
cmake -S . -B build
cmake --build build
```

This PR is done when the above successfully compiles.
@Cazadorro
Copy link

Referencing this Post: #215 (comment)

As for the methods of installation: I'm hoping that the "fetch content"
method in the PR summary is a good stand in for find_package(). If that's
right, then I think our tasks are to (a) figure out how to get that example
to work, and (b) get an add_subdirectory() example to work, and then we'll
have everything we need to write the usage instructions in the docs!

Fetch content here will basically act like a add_subdirectory, not a cmake install. If you want to test out if your cmake project is installable, you can literally call cmake install, and then have another project find_package(your package) See this: https://cmake.org/cmake/help/latest/guide/tutorial/Installing%20and%20Testing.html

Then, run the install step by using the --install option of the cmake command (introduced in 3.15, older versions of CMake must use make install) from the command line. This step will install the appropriate header files, libraries, and executables. For example:

cmake --install .

If you did this with my repo for example, on linux you'd find the libraries installed in the standard linux install directories. If you run cmake install in the top level cmake directory and your project is configured correctly, it should work the same.

Then you could do something like:

cmake_minimum_required(VERSION 3.29)

project(CppUnitsCompare)

find_package(Au CONFIG REQUIRED) 

add_executable(CppUnitsCompare main.cpp)
target_link_libraries(CppUnitsCompare PUBLIC Au::au)

@chiphogg
Copy link
Contributor Author

Thanks for clarifying. I think it will make sense to get the FetchContent pathway working first, because I assume that's the main way we'll want to provide Au to our users. After that, we can make sure it also works when we install and use find_package.

I've made a little bit of progress. When I change the include line in main.cc to this:

#include "build/_deps/au-src/au/au.hh"

...then, it does find this file, so we get a little further. However, it still fails to build because it can't find "au/chrono_interop.hh", which is indeed the first file we include in au.hh.

In other words: every header inside of Au assumes that the other Au headers are found relative to #include "au/...". This is also the include paths we want to expose to our end users (similar to how using the googletest package, we write #include "gtest/...").

So I think we need to find some way to make it so that whenever anyone's CMake project depends on Au::au, or any other Au::... target, then the "build/_deps/au-src" folder gets automatically added to the set of include paths.

Section 35.5.1 offered a clue: maybe I needed to set INCLUDES DESTINATION in the install(TARGETS...) commands, after FILE_SET HEADERS. Setting it to ${CMAKE_INSTALL_INCLUDEDIR} (which is include) doesn't seem to help. The generated AuHeaders.cmake file contains this snippet:

set_target_properties(Au::au PROPERTIES
  INTERFACE_COMPILE_FEATURES "cxx_std_14"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "Au::chrono_interop;Au::constant;Au::math"
)

if(NOT CMAKE_VERSION VERSION_LESS "3.23.0")
  target_sources(Au::au
    INTERFACE
      FILE_SET "HEADERS"
      TYPE "HEADERS"
      BASE_DIRS "${_IMPORT_PREFIX}/include"
      FILES "${_IMPORT_PREFIX}/include/build/_deps/au-src/au/au.hh"
  )
else()
  set_property(TARGET Au::au
    APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES
      "${_IMPORT_PREFIX}/include"
  )
endif()

What I think I want is for BASE_DIRS to be ${_IMPORT_PREFIX}/include/build/_deps/au-src, so that what's left over is au/au.hh. But I'm not sure about this --- especially because I don't know that the include folder even exists in my project.

After playing around, I find:

  • Changing anything in DESTINATION (in the install(TARGETS...) command in Au) will affect both the BASE_DIRS and the FILES commands equally. This is frustrating, because it means I can't make the "au/au.hh" part be the only part that's "left over" from BASE_DIRS.
  • Changing INCLUDES DESTINATION affects only the output INTERFACE_INCLUDE_DIRECTORIES, which makes sense.

I'm starting to worry I've bitten off more than I can chew, and we're not actually at all close to landing CMake support, just because the installation step is so hard.

@chiphogg
Copy link
Contributor Author

One thing I've noticed for projects which do somehow make this work, is that their public headers all live somewhere under an include/ folder.

This is a very counterintuitive way for me to work, coming from a monorepo world. It seems to motivate keeping the header files in a completely different, parallel directory hierarchy relative to the .cc files. Is that really right? It strikes me as hard to maintain, because it would be hard to switch between corresponding .hh and .cc files when they're not simply right next to each other. Then again, maybe it's a moot point for Au, because we don't have .cc files, and probably never will.

So is the upshot that I need to bite the bullet and figure out how to move our headers from au to include/au?

Were you able to install via FetchContent with the approach in your branch?

This appears to work, although I'm not yet sure whether it's a good
approach.
@chiphogg
Copy link
Contributor Author

OK --- I think I see a way forward!

The latest commit is just a proof of concept. I never thought it was production ready, and the surprise build failures reinforce that. But now, for the first time, we're successfully able to incorporate Au into an external project via CMake. And I think it shows me the way forward.

I think the righteous approach will be to create a new subdirectory that contains the entire bazel project --- move everything bazel touches one level deeper into that new home. We'll still keep the main CMakeLists.txt in the root folder of the whole project. What this unlocks is the ability to use that new folder as the base to correctly set the BASE_DIRS argument.

I haven't yet figured out what to call the subfolder. I don't want to call it include, or src, because it's neither of those things (or perhaps both?). Maybe project; I don't know.

That migration can be one PR, where we can work out all the kinks with the bazel integration. And then we can finish up this present PR once that's working.

Meanwhile, comments on the current approach are also welcome.

@chiphogg
Copy link
Contributor Author

Thinking more about it: this could make things really annoying. We would have to invoke everything for bazel from within that subdirectory, instead of the git repo root folder.

Maybe I'll try working around these issues, and then think more about what we should do.

@chiphogg chiphogg changed the title Add CMake install instructions for Au Add CMake install commands for Au Jul 14, 2024
Not sure if the INCLUDEDIR needs to be updated too...
@chiphogg chiphogg marked this pull request as ready for review July 16, 2024 01:32
@chiphogg chiphogg requested a review from geoffviola July 16, 2024 01:32
@chiphogg
Copy link
Contributor Author

OK, I think the current approach is good enough to land.

I'm a little unsure of how the symbolic link would work under windows. But if we find that it's problematic, we could add windows support later.

Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. CMake is rough for packaging. I was able to navigate through the tests in the summary section.

It's funny that cmake --build build builds all the au unit tests and yet CppUnitsCompare doesn't depend on them. The concept of make all is foreign to us monrepo users 😄 . --target CppUnitsCompare does what I expect. -j $(nproc) will build in parallel, although it won't work on Windows.

In the summary

add_executable(CppUnitsCompare main.cpp)

Should be add_executable(CppUnitsCompare main.cc)

I'm not sure if you want to support find_package as well in this repo. I tried cd au/cmake/build && cmake --install ., but i got an error

Here is the long error
sudo cmake --install .
-- Install configuration: ""
-- Up-to-date: /usr/local/include
-- Installing: /usr/local/include/gmock
-- Installing: /usr/local/include/gmock/gmock-actions.h
-- Installing: /usr/local/include/gmock/gmock.h
-- Installing: /usr/local/include/gmock/gmock-nice-strict.h
-- Installing: /usr/local/include/gmock/gmock-cardinalities.h
-- Installing: /usr/local/include/gmock/gmock-spec-builders.h
-- Installing: /usr/local/include/gmock/gmock-matchers.h
-- Installing: /usr/local/include/gmock/gmock-more-actions.h
-- Installing: /usr/local/include/gmock/gmock-function-mocker.h
-- Installing: /usr/local/include/gmock/gmock-more-matchers.h
-- Installing: /usr/local/include/gmock/internal
-- Installing: /usr/local/include/gmock/internal/gmock-port.h
-- Installing: /usr/local/include/gmock/internal/custom
-- Installing: /usr/local/include/gmock/internal/custom/gmock-generated-actions.h
-- Installing: /usr/local/include/gmock/internal/custom/gmock-port.h
-- Installing: /usr/local/include/gmock/internal/custom/gmock-matchers.h
-- Installing: /usr/local/include/gmock/internal/custom/README.md
-- Installing: /usr/local/include/gmock/internal/gmock-internal-utils.h
-- Installing: /usr/local/include/gmock/internal/gmock-pp.h
-- Installing: /usr/local/lib/libgmock.a
-- Installing: /usr/local/lib/libgmock_main.a
-- Installing: /usr/local/lib/pkgconfig/gmock.pc
-- Installing: /usr/local/lib/pkgconfig/gmock_main.pc
-- Installing: /usr/local/lib/cmake/GTest/GTestTargets.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestTargets-noconfig.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestConfigVersion.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestConfig.cmake
-- Up-to-date: /usr/local/include
-- Installing: /usr/local/include/gtest
-- Installing: /usr/local/include/gtest/gtest_pred_impl.h
-- Installing: /usr/local/include/gtest/gtest-test-part.h
-- Installing: /usr/local/include/gtest/gtest.h
-- Installing: /usr/local/include/gtest/gtest-assertion-result.h
-- Installing: /usr/local/include/gtest/gtest-message.h
-- Installing: /usr/local/include/gtest/gtest-matchers.h
-- Installing: /usr/local/include/gtest/gtest-typed-test.h
-- Installing: /usr/local/include/gtest/gtest-printers.h
-- Installing: /usr/local/include/gtest/gtest_prod.h
-- Installing: /usr/local/include/gtest/gtest-param-test.h
-- Installing: /usr/local/include/gtest/gtest-death-test.h
-- Installing: /usr/local/include/gtest/gtest-spi.h
-- Installing: /usr/local/include/gtest/internal
-- Installing: /usr/local/include/gtest/internal/gtest-internal.h
-- Installing: /usr/local/include/gtest/internal/gtest-type-util.h
-- Installing: /usr/local/include/gtest/internal/gtest-death-test-internal.h
-- Installing: /usr/local/include/gtest/internal/gtest-port-arch.h
-- Installing: /usr/local/include/gtest/internal/custom
-- Installing: /usr/local/include/gtest/internal/custom/gtest.h
-- Installing: /usr/local/include/gtest/internal/custom/gtest-printers.h
-- Installing: /usr/local/include/gtest/internal/custom/gtest-port.h
-- Installing: /usr/local/include/gtest/internal/custom/README.md
-- Installing: /usr/local/include/gtest/internal/gtest-port.h
-- Installing: /usr/local/include/gtest/internal/gtest-param-util.h
-- Installing: /usr/local/include/gtest/internal/gtest-string.h
-- Installing: /usr/local/include/gtest/internal/gtest-filepath.h
-- Installing: /usr/local/lib/libgtest.a
CMake Error at _deps/googletest-build/googletest/cmake_install.cmake:84 (file):
  file INSTALL cannot find
  "/home/gviola/git/au/cmake/build/lib/libgtest_main.a": No such file or
  directory.
Call Stack (most recent call first):
  _deps/googletest-build/googlemock/cmake_install.cmake:67 (include)
  _deps/googletest-build/cmake_install.cmake:47 (include)
  cmake_install.cmake:47 (include)

@Cazadorro
Copy link

The changes look good. CMake is rough for packaging. I was able to navigate through the tests in the summary section.

It's funny that cmake --build build builds all the au unit tests and yet CppUnitsCompare doesn't depend on them. The concept of make all is foreign to us monrepo users 😄 . --target CppUnitsCompare does what I expect. -j $(nproc) will build in parallel, although it won't work on Windows.

In the summary

add_executable(CppUnitsCompare main.cpp)

Should be add_executable(CppUnitsCompare main.cc)

I'm not sure if you want to support find_package as well in this repo. I tried cd au/cmake/build && cmake --install ., but i got an error

Here is the long error

sudo cmake --install .
-- Install configuration: ""
-- Up-to-date: /usr/local/include
-- Installing: /usr/local/include/gmock
-- Installing: /usr/local/include/gmock/gmock-actions.h
-- Installing: /usr/local/include/gmock/gmock.h
-- Installing: /usr/local/include/gmock/gmock-nice-strict.h
-- Installing: /usr/local/include/gmock/gmock-cardinalities.h
-- Installing: /usr/local/include/gmock/gmock-spec-builders.h
-- Installing: /usr/local/include/gmock/gmock-matchers.h
-- Installing: /usr/local/include/gmock/gmock-more-actions.h
-- Installing: /usr/local/include/gmock/gmock-function-mocker.h
-- Installing: /usr/local/include/gmock/gmock-more-matchers.h
-- Installing: /usr/local/include/gmock/internal
-- Installing: /usr/local/include/gmock/internal/gmock-port.h
-- Installing: /usr/local/include/gmock/internal/custom
-- Installing: /usr/local/include/gmock/internal/custom/gmock-generated-actions.h
-- Installing: /usr/local/include/gmock/internal/custom/gmock-port.h
-- Installing: /usr/local/include/gmock/internal/custom/gmock-matchers.h
-- Installing: /usr/local/include/gmock/internal/custom/README.md
-- Installing: /usr/local/include/gmock/internal/gmock-internal-utils.h
-- Installing: /usr/local/include/gmock/internal/gmock-pp.h
-- Installing: /usr/local/lib/libgmock.a
-- Installing: /usr/local/lib/libgmock_main.a
-- Installing: /usr/local/lib/pkgconfig/gmock.pc
-- Installing: /usr/local/lib/pkgconfig/gmock_main.pc
-- Installing: /usr/local/lib/cmake/GTest/GTestTargets.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestTargets-noconfig.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestConfigVersion.cmake
-- Installing: /usr/local/lib/cmake/GTest/GTestConfig.cmake
-- Up-to-date: /usr/local/include
-- Installing: /usr/local/include/gtest
-- Installing: /usr/local/include/gtest/gtest_pred_impl.h
-- Installing: /usr/local/include/gtest/gtest-test-part.h
-- Installing: /usr/local/include/gtest/gtest.h
-- Installing: /usr/local/include/gtest/gtest-assertion-result.h
-- Installing: /usr/local/include/gtest/gtest-message.h
-- Installing: /usr/local/include/gtest/gtest-matchers.h
-- Installing: /usr/local/include/gtest/gtest-typed-test.h
-- Installing: /usr/local/include/gtest/gtest-printers.h
-- Installing: /usr/local/include/gtest/gtest_prod.h
-- Installing: /usr/local/include/gtest/gtest-param-test.h
-- Installing: /usr/local/include/gtest/gtest-death-test.h
-- Installing: /usr/local/include/gtest/gtest-spi.h
-- Installing: /usr/local/include/gtest/internal
-- Installing: /usr/local/include/gtest/internal/gtest-internal.h
-- Installing: /usr/local/include/gtest/internal/gtest-type-util.h
-- Installing: /usr/local/include/gtest/internal/gtest-death-test-internal.h
-- Installing: /usr/local/include/gtest/internal/gtest-port-arch.h
-- Installing: /usr/local/include/gtest/internal/custom
-- Installing: /usr/local/include/gtest/internal/custom/gtest.h
-- Installing: /usr/local/include/gtest/internal/custom/gtest-printers.h
-- Installing: /usr/local/include/gtest/internal/custom/gtest-port.h
-- Installing: /usr/local/include/gtest/internal/custom/README.md
-- Installing: /usr/local/include/gtest/internal/gtest-port.h
-- Installing: /usr/local/include/gtest/internal/gtest-param-util.h
-- Installing: /usr/local/include/gtest/internal/gtest-string.h
-- Installing: /usr/local/include/gtest/internal/gtest-filepath.h
-- Installing: /usr/local/lib/libgtest.a
CMake Error at _deps/googletest-build/googletest/cmake_install.cmake:84 (file):
  file INSTALL cannot find
  "/home/gviola/git/au/cmake/build/lib/libgtest_main.a": No such file or
  directory.
Call Stack (most recent call first):
  _deps/googletest-build/googlemock/cmake_install.cmake:67 (include)
  _deps/googletest-build/cmake_install.cmake:47 (include)
  cmake_install.cmake:47 (include)

You get the error because an internal fetch content is not really part of the install process, which it is using for google test. which is not necessary for an installed library.

@chiphogg Testing should be guarded by a CMake option which would allow you to disable testing and run install, which would remove the dependency on gtest and not have to deal with that.

au/CMakeLists.txt

Lines 38 to 50 in 3e25704

enable_testing()
# Bring in GoogleTest so we can build and run the tests.
include(FetchContent)
FetchContent_Declare(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG 58d77fa8070e8cec2dc1ed015d66b454c8d78850 # Release 1.12.1
FIND_PACKAGE_ARGS
1.12.1
NAMES GTest
)

Testing should be guarded anyway because there's no way to use it within a package manager or offline in it's current state, and you don't want end users of an installed library to have to build tests, or worse have your version of gtest conflict with theirs. If they want tests they can subdirectory your project or just straight up clone the project.

@chiphogg
Copy link
Contributor Author

I'm not sure if you want to support find_package as well in this repo. I tried cd au/cmake/build && cmake --install ., but i got an error

Yep --- I tried installing too, and I also got "some kind of error with the googletest deps". In my case, I discovered it by trying to actually use find_package(). It reported that it did find the package, but that there was an error with the GTest::gtest and GTest::gmock dependencies, and it therefore reported it as "not found".

I think it will make the most sense to address this in a separate follow-on PR. But I definitely want to get this working before we close #215.

Interestingly, the error is specifically with targets that use googletest, and not the Au tests themselves (i.e., Au::testing). This means that if we do the installation, and we don't use Au::testing, it will Just Work out of the box! I was able to set up this file, compile it, and run it:

#include "au/au.hh"
#include "au/io.hh"
#include "au/units/feet.hh"
#include "au/units/miles.hh"
#include <iostream>

int main(int argc, char **argv) {
  std::cout << au::miles(1).as(au::feet) << " per mile" << std::endl;
}

That was pretty cool. Still feels a little sloppy to just "use whatever's on the system", but at least I get the appeal now --- for quick and dirty projects, it's pretty awesome.

@chiphogg Testing should be guarded by a CMake option which would allow you to disable testing and run install, which would remove the dependency on gtest and not have to deal with that.

au/CMakeLists.txt

Lines 38 to 50 in 3e25704

enable_testing()
# Bring in GoogleTest so we can build and run the tests.
include(FetchContent)
FetchContent_Declare(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG 58d77fa8070e8cec2dc1ed015d66b454c8d78850 # Release 1.12.1
FIND_PACKAGE_ARGS
1.12.1
NAMES GTest
)

Testing should be guarded anyway because there's no way to use it within a package manager or offline in it's current state, and you don't want end users of an installed library to have to build tests, or worse have your version of gtest conflict with theirs. If they want tests they can subdirectory your project or just straight up clone the project.

Well, it's a little complicated, because we do have one actual target (Au::testing) that does depend on googletest. This target is totally optional. Looking at its contents, literally every public API it contains is specific to googletest APIs (matchers and EXPECT macros) --- there are no generic testing utilities. So perhaps Au::googletest_helpers would have been a better name. 😅

We can start thinking now about what the follow-on PR needs to do. @Cazadorro, let me know if this sounds about right to you:

  • Add an option to enable tests (we won't build the tests if this is absent)
  • Add an option to enable googletest helpers (we won't build Au::testing if this is absent)
  • Make sure that if both of these options are false, we won't have any dependency on googletest
  • Make sure that the "enable tests" option is off when we install
  • Get the googletest dependency to work with the installation if the "enable googletest helpers" option is on when we install
  • Make sure we can install with no googletest dependency if the user disables the "enable googletest helpers" option

@chiphogg
Copy link
Contributor Author

(And, to clarify: by "we won't build ... if this is absent", I mean "we won't create the target for ... if this is absent".)

@chiphogg chiphogg merged commit 49e3e0e into main Jul 16, 2024
10 checks passed
@chiphogg chiphogg deleted the chiphogg/cmake-install#215 branch July 16, 2024 15:46
@chiphogg
Copy link
Contributor Author

Just for posterity @Cazadorro: I believe I ended up resolving all of these issues.

  • Adding EXCLUDE_FROM_ALL to the install instructions stops all the tests from being built when Au is a dependency rather than the main project.
  • Using find_dependency(), as in Properly declare googletest dependency #268, handles the Googletest installation properly.

If we find more problems later, we can see whether these option based solutions would resolve them.

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

Successfully merging this pull request may close these issues.

3 participants