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

Simplify CMake files to always use `find_package()`. #2802

Closed
coryan opened this issue Jun 21, 2019 · 16 comments · Fixed by #2967

Comments

@coryan
Copy link
Collaborator

commented Jun 21, 2019

This is a large task, probably should be broken down into smaller tasks.

I would like us to consider simplifying the CMake files to always use find_package() to find the dependencies. Then CI builds can pre-install all the dependencies. The users can do the same, or (at their choice), use a super build that installs all the dependencies for them.

We did this in googleapis/google-cloud-cpp-spanner#39 and it has been much easier to use, and I think it is easier to maintain.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

What does this mean for the readme/Dockerfile*? Since they're testing grabbing the dependencies via external projects, does this mean we should remove the test readmes?

@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

We will add a super build (make in ci/super/, or simply super/), and change the README.md instructions (and the corresponding Dockerfiles) to use the super build.

I believe (but have not thought much about it, nor discussed it with my colleagues) the README structure will change to:

  • Do you just want to build this quickly, run the examples and see if it works?
    • With Bazel: --> Link to the instructions on how to do that with Bazel.
    • With CMake --> Link to the instructions with a super build.
  • Do you want to install the library in /usr/local and use them from there? --> link to the INSTALL.md file.
  • Do you want to use the library as part of a larger Bazel project? --> link to a new document or README section.
  • Do you want to use the library as part of a larger CMake project? --> link to a new document or README section on how to do that (currently this is hidden in the landing page for the reference docs).
@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Do we also want to change how we obtain nlohmann_json (build, install, find_package) or leave it alone? The logical place to install it would be /usr/local/include, but that might also be where a user has installed it. It wouldn't make much sense to #include <nlohmann/json.hpp> in nljson_use_after_third_party_test.cc.

@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Do we also want to change how we obtain nlohmann_json (build, install, find_package) or leave it alone?

Good question. I think we should aim to finding it via find_package() too. We can take an intermediate step where we leave it alone if that is simpler to implement.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

It should be easy to implement, I'm just asking what is the right thing to do here. We currently support the user including a potentially different version of the library, we also patch it to remove the user-defined literals. If we use find_package for it I think we need to install it to a non-standard location then set the search paths to that location.

@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

If we use find_package for it I think we need to install it to a non-standard location then set the search paths to that location.

Or maybe (but I am not certain) we can use the same version that the customer already installed, just like we do for protobuf or gRPC if they are already installed.

I think we should proceed with some caution, maybe do the other dependencies first, and tackle nlohmann's json last. Thoughts?

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2019

Given #2874 we should probably do it sooner than later. If we proceed with find_package then the only hurdle is patching the file. Is it urgent that we remove the user-defined literals? If pollution is a problem, maybe it would be better to send an upstream patch wrapping it in a namespace or something.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

I am now convinced that we should tackle nlohmann_json last, but some notes about problems I ran into for future reference:

  • nlohmann_json 3.4.0 requires a minimum of CMake 3.8 (they changed it to 3.1 in later releases), which means we can't build it from source in some of the Ubuntu distros

  • I believe (but not entirely sure) that nlohmann_json 3.6.1 is incompatible and that we'll need to make some changes

  • I did run into the ambiguous error w.r.t. the user-defined literals. I guess we can attempt to patch it out still

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

One more question.

The Dockerfiles in ci/kokoro/readme now run the super build. I'm assuming the ones in ci/kokoro/install don't need to change.

What about in ci/kokoro/docker? Is it planned to move the non-install ones to Bazel like in spanner?

@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

The Dockerfiles in ci/kokoro/readme now run the super build. I'm assuming the ones in ci/kokoro/install don't need to change.

I agree.

What about in ci/kokoro/docker? Is it planned to move the non-install ones to Bazel like in spanner?

More difficult to answer. I think we made better calls in Spanner, so if you see one build that we should move just ask. In general I think that:

  • Builds that require setting compiler options for the dependencies probably are better with Bazel (think ASAN, MSAN, UBSAN, C++17, etc.)

  • Builds where CMake has good (or well understood) integration probably better in CMake (install of course, but also clang-tidy and coverage)

  • The builds in ci/kokoro/readme and ci/kokoro/install seem like the right place to check for OS and compiler variations, we do not need to repeat those in ci/kokoro/docker.

  • If we can simplify or speed up the builds in ci/kokoro/docker by using pre-packaged versions of gRPC or protobuf, or any dependency then we should. Building those from source is tested in ci/kokoro/{install,readme} in any case.

  • There is going to be some overlap in the testing done by each build, that is Okay. We should prefer to avoid duplicate builds all other things being equal.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

I believe (but not entirely sure) that nlohmann_json 3.6.1 is incompatible and that we'll need to make some changes

Some additional information. It seems at some point that they changed some of their include guards to look like this:

INCLUDE_NLOHMANN_JSON_HPP_
INCLUDE_NLOHMANN_JSON_FWD_HPP_

While we could also undef these, it is brittle and we are technically relying on an implementation detail.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Here are two related issues:

nlohmann/json#1394

nlohmann/json#1539

Furthermore the changelog says that the former is implemented. However, I can't seem to find any evidence of it actually being implemented. If it is implemented, we should change our approach to utilize those changes.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Here is a tentative list of things to be done. Feel free to amend or correct as needed.

  • Remove ci/kokoro/docker/Dockerfile.ubuntu-trusty. It doesn't seem to be used by anything
  • Reorganize the conditionals in ci/kokoro/docker/build.sh, that is, group asan with ubsan, etc. This is more of a nit
  • Remove building google-cloud-cpp-dependencies in build-in-docker-cmake.sh; this should be unnecessary when using GOOGLE_CLOUD_CPP_DEPENDENCY_PROVIDER=package
  • Change any build using fedora to use fedora-install instead, so that all of the dependencies are pre-installed; this shouldn't impact build times as the images are cached
  • Don't use DISTRO_VERSION=16.04 for the no-ex build. It seems unnecessary
  • Remove the ci/kokoro/docker CentOS build? If we are only including it to test GCC 4.8, that should already be covered by the readme/install builds
@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

Here is a tentative list of things to be done. Feel free to amend or correct as needed.

  • Remove ci/kokoro/docker/Dockerfile.ubuntu-trusty. It doesn't seem to be used by anything
  • Reorganize the conditionals in ci/kokoro/docker/build.sh, that is, group asan with ubsan, etc. This is more of a nit
  • Remove building google-cloud-cpp-dependencies in build-in-docker-cmake.sh; this should be unnecessary when using GOOGLE_CLOUD_CPP_DEPENDENCY_PROVIDER=package
  • Change any build using fedora to use fedora-install instead, so that all of the dependencies are pre-installed; this shouldn't impact build times as the images are cached

Agree.

  • Don't use DISTRO_VERSION=16.04 for the no-ex build. It seems unnecessary

This sounds unrelated maybe?

  • Remove the ci/kokoro/docker CentOS build? If we are only including it to test GCC 4.8, that should already be covered by the readme/install builds

Agree.

@remyabel

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

This sounds unrelated maybe?

Perhaps, but I still see it as an improvement. There doesn't seem to be any particular reason to use Xenial just for no exceptions, so it'll cut down on one image being downloaded.

@coryan

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

There doesn't seem to be any particular reason to use Xenial just for no exceptions, so it'll cut down on one image being downloaded.

Ack. SGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.