googleapis / google-cloud-cpp

C++ Client Libraries for Google Cloud Services
https://cloud.google.com/
Apache License 2.0
545 stars 370 forks source link

Add Abseil dep in cmake and bazel #2309

Closed devjgm closed 4 years ago

devjgm commented 5 years ago

Now that Abseil is close to (or done?) adding a cmake install target (https://github.com/abseil/abseil-cpp/issues/111), we may want to consider adding Abseil as a dep and using it where appropriate. I think a good first step for this particular issue would be to:

  1. Add Abseil as a dep to our cmake and bazel builds.

  2. Change some small piece of our internal code to use Abseil. This will demonstrate that the build deps adding in step 1 work in both cmake and build.

It's important that we don't initially change anything that we export in a public interface until we explicitly decide to use Abseil in interfaces. And example of a change for step 2 that might make sense is changing calls to our internal make_unique -> absl::make_unique. That touches about 275 locations, so that may be a bit big. Another idea might be changing the implementation of internal::ParseRfc3339 and internal::FormatRfc3339 to be implemented in terms of the absl/time types and functions. This would be a slightly more involved change, but it would only touch a couple files.

Maybe there are smaller things we could change to satisfy step 2 above also. Feel free to suggest something. The goal is to test that the build deps work correctly.

coryan commented 5 years ago

We should try to find some tests or integration tests that could benefit from using Abseil. That way we can introduce the dependency without complicating the installation instructions for existing users.

devjgm commented 5 years ago

SGTM.

ghost commented 5 years ago

I have this partially (?) working (WIP branch). The easiest solution is to just use add_subdirectory but then we have to vendor abseil and lose all the benefits of using an external project, so this is out.

The current solution requires manually listing all the targets (and I've seen this used in other projects like TensorFlow for example), like so:

    # Create imported target absl::config
    add_library(absl::config INTERFACE IMPORTED)
    set_library_properties_for_external_project(absl::config absl_config
                                                HEADER_ONLY)
    add_dependencies(absl::config abseil_project)
    # ...and so on...

I have managed to get absl::StrJoin working but absl::FromChrono is not working. For some reason, despite me specifying linking against absl::time, the bigtable benchmarks are instead linking against absl::dynamic_annotations and absl::civil_time. So either I made a typo or there is a flaw in my approach somewhere.

A third solution to automatically get all of the imports is to use a secondary external project; install abseil first to get the abslTargets.cmake file, then include it. But this is kind of clunky and might have other issues.

I'm sure there is an easier way that I'm missing.

coryan commented 5 years ago

I think we should wait for Abseil for a couple of reasons: (a) they are working on support for shared objects, which they currently do not have (?), and we use in at least some platforms, and (b) it would be easier to add Abseil when all the dependencies are found via find_package() like we do in the google-cloud-cpp-spanner project.

devjgm commented 4 years ago

I believe it should be possible for us to take a dep on Abseil now. Indeed, newer versions of gRPC will require it. Taking a dep on Abseil should be no problem for bazel users, because as long as they call our google_cloud_cpp_deps function, things should just work (I think).

However, for CMake users, is taking a dep on Abseil will require that either the customer use our super build, which will automatically fetch Abseil, or they must install Abseil themselves. It's no problem to update our packaging instructions, etc to install Abseil.

Q: How do we message to our users that a new release of the library now requires a new dep? Is a simple message in the release notes good enough?

Q: Will there be any changes needed to our vcpkg? I'm guessing that after our next release, our next vpkg definition will need to list Abseil as a dep, then it should Just Work?

Q: Is there anything else to consider before I give us a dep on Abseil?

coryan commented 4 years ago

I believe it should be possible for us to take a dep on Abseil now. Indeed, newer versions of gRPC will require it. Taking a dep on Abseil should be no problem for bazel users, because as long as they call our google_cloud_cpp_deps function, things should just work (I think).

I agree.

However, for CMake users, is taking a dep on Abseil will require that either the customer use our super build, which will automatically fetch Abseil, or they must install Abseil themselves. It's no problem to update our packaging instructions, etc to install Abseil.

Correct.

Q: How do we message to our users that a new release of the library now requires a new dep? Is a simple message in the release notes good enough?

I think so.

Q: Will there be any changes needed to our vcpkg? I'm guessing that after our next release, our next vpkg definition will need to list Abseil as a dep, then it should Just Work?

It is already working. vcpkg upgrade to grpc-1.28.x a while ago.

Q: Is there anything else to consider before I give us a dep on Abseil?

Testing with shared libraries and with static libraries for Linux is already in the matrix. For Windows, the quickstart builds test with DLLs and static libraries but we do not have regular builds that test with both. Since DLLs were a concern for Abseil maybe we should test early instead of waiting a full release cycle.

devjgm commented 4 years ago

Since DLLs were a concern for Abseil maybe we should test early instead of waiting a full release cycle.

I'm not sure I follow what you mean here. What do you mean by testing early rather than waiting a release cycle?

FTR, I have a draft PR that's getting fairly close to working https://github.com/googleapis/google-cloud-cpp/pull/4087. I'm wondering what else we should do before we land that PR. Do we need to add another CI build?

coryan commented 4 years ago

I'm not sure I follow what you mean here. What do you mean by testing early rather than waiting a release cycle?

Well, what I was thinking: the only DLL build we have is windows/quickstart-cmake-dll but that build the latest release, not the current code. So in a sense it lags behind a full release cycle.

OTOH, that has been building against Abseil for a while now.

FTR, I have a draft PR that's getting fairly close to working #4087. I'm wondering what else we should do before we land that PR. Do we need to add another CI build?

It might be a good idea to have a CI build that compiles the current code against DLLs. I do not think we should gate your PR for that.

devjgm commented 4 years ago

Ahh, I see. Makes sense.

I filed https://github.com/googleapis/google-cloud-cpp/issues/4090 to track that specific issue so we don't need to block the adoption of Abseil.