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 Abseil dep in cmake and bazel #2309

Closed
devjgm opened this issue Mar 23, 2019 · 9 comments · Fixed by #4087
Closed

Add Abseil dep in cmake and bazel #2309

devjgm opened this issue Mar 23, 2019 · 9 comments · Fixed by #4087
Assignees
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@devjgm
Copy link
Contributor

devjgm commented Mar 23, 2019

Now that Abseil is close to (or done?) adding a cmake install target (abseil/abseil-cpp#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.

@devjgm devjgm added the type: cleanup An internal cleanup or hygiene concern. label Mar 23, 2019
@coryan
Copy link
Contributor

coryan commented Mar 23, 2019

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
Copy link
Contributor Author

devjgm commented Mar 26, 2019

SGTM.

@ghost
Copy link

ghost commented Jul 5, 2019

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
Copy link
Contributor

coryan commented Jul 8, 2019

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
Copy link
Contributor Author

devjgm commented May 10, 2020

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?

@devjgm devjgm self-assigned this May 10, 2020
@coryan
Copy link
Contributor

coryan commented May 11, 2020

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
Copy link
Contributor Author

devjgm commented May 12, 2020

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 #4087. I'm wondering what else we should do before we land that PR. Do we need to add another CI build?

@coryan
Copy link
Contributor

coryan commented May 12, 2020

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
Copy link
Contributor Author

devjgm commented May 12, 2020

Ahh, I see. Makes sense.

I filed #4090 to track that specific issue so we don't need to block the adoption of Abseil.

devjgm added a commit that referenced this issue May 12, 2020
Fixes: #2309

Adds a dep on Abseil. Uses absl::make_unique in one test in Spanner just to show that the Abseil dep is working in all the builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants