CPP: Fix cmake build failure + export libphonenumber-config.cmake by temideoye · Pull Request #2818 · google/libphonenumber

temideoye · 2022-09-09T05:10:50Z

Currently, the project fails to build when BUILD_GEOCODER=OFF due to a missing dependency. The issue is described here. This push fixes the bug and proposes the following additional features to enhance the CMake build process overall:

Expose BUILD_SHARED_LIBS and BUILD_TESTING as user configurable options
Add functionality to build and export CMake package config files so downstream projects can consume the library with a simple find_package(libphonenumber), without creating a FindLibPhonenumber.cmake file
Issue: https://issuetracker.google.com/issues/239501100

…library without creating a FindLibPhonenumber

google-cla · 2022-09-09T05:10:53Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

temideoye · 2022-12-10T20:27:06Z

Hey, @penmetsaa! Just checking in, have you been able to review this?

penmetsaa · 2022-12-11T00:54:07Z

Hi @temideoye ,

Thanks so much for your patience. We are planning to review this in next 4 weeks.

cpp/CMakeLists.txt

penmetsaa · 2023-01-03T11:05:56Z

tools/cpp/CMakeLists.txt


 project (generate_geocoding_data)

-# Helper functions dealing with finding libraries and programs this library


As we have moved the download functionality to main library's CMAKE, we were unable to build the offline geocoder individually. Could we not remove this from here OR have this as a common build rule file?

CMake Error at CMakeLists.txt:39 (target_link_libraries): Target "generate_geocoding_data" links to: absl::strings but the target was not found. Possible reasons include: * There is a typo in the target name. * A find_package call is missing for an IMPORTED target. * An ALIAS target is missing.

Note: I am not expertise in CPP builds, please suggest and do what you think is best :)

Can we do anything about this..?

@penmetsaa, I had assumed (erroneously) that tools/cpp is merely a dependency of the main project, but now I understand you build it independently for other purposes as well.

There are 2, or 3 possible ways to resolve this:

Copy the find abseil functions back to tools/cpp. The demerit of this is obviously the maintenance overhead.

Create a new shared cmake module for the find abseil functions. This is DRYer than 1, but still a bit of a dance.

Build tools/cpp via the main project with an option like -DBUILD_TOOLS_ONLY=ON. This should simplify the interface in that all dependency resolutions happen in the same file. It would be my preference too.

I will share a pull request in a moment. Let me know if it works for you, or if you'd rather resolve it a different way.

@penmetsaa, please see 2869.

penmetsaa · 2023-01-03T11:08:07Z

cpp/CMakeLists.txt

+option (USE_LITE_METADATA "Use lite metadata" OFF)
+option (USE_RE2 "Use RE2" OFF)
+option (USE_STD_MAP "Force the use of std::map" OFF)
+option (BUILD_STATIC_LIB "Build static libraries" ON)


About these new options, could you please add some notes in cpp/README file.
It helps readers to know about these options.

penmetsaa

Thanks so much for meticulous work.
Mentioned some minor suggestions / doubts I have, please take a look. :)

temideoye · 2023-01-04T02:09:51Z

Hiya, @penmetsaa.

I've gone through your comments. Thanks.

First, it looks like you ran your tests against each of the commits in the pull request. Only the last commit is designed to be whole and complete, please limit your test to that. The preceding commits are mostly of documentary value and not guaranteed to be stable.

If you could test the repo at this point, I expect it to build without errors with the only necessary change being bumping the version number to 8.13.0. Please confirm and let me know.

With regards to the readme; sure I'm happy to send a pull request that documents what's new.

Happy new year!

penmetsaa · 2023-01-04T07:18:05Z

Thanks! +1 to what you mentioned.. Some pending AIs:

Reply to my comment on: tools/cpp/CMakeLists.txt
Documentation update (a new PR) regarding the new oprions

penmetsaa · 2023-01-04T12:13:34Z

Hi,
There is a breakage in CPP build after this merge, could you please look into this?

  - Performing Test CMAKE_HAVE_LIBC_PTHREAD
  -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
  -- Found Threads: TRUE
  -- Found Boost: /usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfig.cmake (found suitable version "1.74.0", minimum required is "1.40.0") found components: date_time system thread
  -- Checking for module 'protobuf>=2.4'
  --   Found protobuf, version 3.12.4
  -- Checking for module 'icu-uc>=4.4'
  --   Found icu-uc, version 71.1
  -- Checking for module 'icu-i18n>=4.4'
  --   Found icu-i18n, version 71.1
  -- Looking for C++ include tr1/unordered_map
  -- Looking for C++ include tr1/unordered_map - found
  -- Configuring done
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber" which requires target "absl_node_hash_set" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber" which requires target "absl_strings" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber" which requires target "absl_synchronization" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding" which requires target "absl_node_hash_set" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding" which requires target "absl_strings" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding" which requires target "absl_synchronization" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber-shared" which requires target "absl_node_hash_set" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber-shared" which requires target "absl_strings" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "phonenumber-shared" which requires target "absl_synchronization" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding-shared" which requires target "absl_node_hash_set" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding-shared" which requires target "absl_strings" that is not in any export set.
  CMake Error: install(EXPORT "libphonenumber-targets" ...) includes target "geocoding-shared" which requires target "absl_synchronization" that is not in any export set.
  -- Generating done
  CMake Generate step failed.  Build files cannot be regenerated correctly.
  (END)

Is this something related to https://stackoverflow.com/questions/5378528/install-export-problem-for-library-with-dependencies, should we link_libraries?, if not feasible to export these external deps..

temideoye · 2023-01-04T20:21:28Z

@penmetsaa, that looks like cmake couldn't find your abseil lib directory. Do you have the installation path of abseil in CMAKE_PREFIX_PATH? You may try something like cmake -S "path/to/libphonenumber/cpp" -B "path/to/build/directory" -DCMAKE_PREFIX_PATH="path/to/abseil;path/to/other/dependencies" .

penmetsaa · 2023-01-05T05:02:19Z

Hi @temideoye

When I tried installing abseil at system level and then mentioning path in the CMAKE_PREFIX_PATH -> it is working correctly.
When there is no abseil found, we know that FetchContent() dowloads and builds the abseil to temp/cache dir and referring from there -> This is working before our/this commit.

Is it that after the change, the cached abseil binaries are not been able to found?

temideoye · 2023-01-05T07:25:10Z

Oh dang, I see what you mean. The issue seems to be that abseil disables its install rules in superbuilds:

https://github.com/abseil/abseil-cpp/blob/b0cc11b9763d50f6ecd160719b0f78fde9712a7f/CMakeLists.txt#L60-L66

Setting ABSL_ENABLE_INSTALL=ON should fix the errors. My bad, I should have also tested the build without a prebuilt abseil. Will send you a patch to test in a moment.

temideoye · 2023-01-05T07:43:57Z

@penmetsaa, I've added a fix for the install errors to 2869. Let me know if it resolves the issue for you.

penmetsaa · 2023-01-06T13:07:09Z

Thanks much for the patch @temideoye..
Closing the thread as that fix resolved the issue

nekopsykose · 2023-01-10T04:30:04Z

cpp/CMakeLists.txt

-  "test_metadata"
+  "metadata"


this rename conflicts with the metadata.h generated above in the non-test METADATA_TARGET. they both output a metadata.h like this, which means the build has two rules that generate the same output file (one for metadata_target, and one for test_metadata_target).

this breaks the build for cmake -G Ninja, because the .ninja spec forbids two distinct rules for the same output:

$ ninja -C build/ ninja: entering directory 'build/' ninja: multiple rules generate '/home/demon/src/aports/community/libphonenumber/src/libphonenumber-8.13.4/cpp/src/phonenumbers/metadata.h'

@nekopsykose, thanks for the catch. The bug was fixed here, but was reintroduced by a clumsy merge. @penmetsaa, will send in a quick pull request to restore the fix if that's OK.

Regression fixed with 2874.

penmetsaa · 2023-01-10T08:04:27Z

Thanks for correcting the thing that we overlooked @nekopsykose and @temideoye. Merged the #2874 PR

nekopsykose · 2023-01-10T08:04:41Z

@penmetsaa thanks!

temideoye added 7 commits September 8, 2022 13:33

Find and add abseil even when BUILD_GEOCODER=OFF

6daab69

Treat boolean variables as conditions

ce7a59c

Expose BUILD_SHARED_LIBS as a user configurable option

5d7d3a6

Add BUILD_TESTING option

18032fc

Wrap test dependencies and targets in if (BUILD_TESTING)

44e200b

Refactor library build procedure to respect user defined configurations

2695ee0

Export cmake package config files so downstream projects can use the …

2126de7

…library without creating a FindLibPhonenumber

penmetsaa self-requested a review September 21, 2022 07:11

penmetsaa mentioned this pull request Sep 21, 2022

Use a system-installed abseil when available #2791

Merged

penmetsaa mentioned this pull request Jan 3, 2023

Move find_package call for absl to global scope #2868

Closed

Merge branch 'master' into cpp_fix-cmake-build-issues

bd954a2

penmetsaa reviewed Jan 3, 2023

View reviewed changes

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

penmetsaa reviewed Jan 3, 2023

View reviewed changes

cpp/CMakeLists.txt Outdated Show resolved Hide resolved

penmetsaa reviewed Jan 3, 2023

View reviewed changes

penmetsaa suggested changes Jan 3, 2023

View reviewed changes

Bump version number to 8.13.0

3ce80e8

penmetsaa approved these changes Jan 4, 2023

View reviewed changes

penmetsaa merged commit 88c175c into google:master Jan 4, 2023

penmetsaa mentioned this pull request Jan 5, 2023

CPP: Add BUILD_TOOLS_ONLY option #2869

Merged

mrowrpurr mentioned this pull request Jan 7, 2023

[vcpkg] Update CMake to 3.25.1 microsoft/vcpkg#28126

Closed

nekopsykose reviewed Jan 10, 2023

View reviewed changes


		project (generate_geocoding_data)

		# Helper functions dealing with finding libraries and programs this library

Conversation

temideoye commented Sep 9, 2022 • edited by penmetsaa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Sep 9, 2022

Uh oh!

temideoye commented Dec 10, 2022

Uh oh!

penmetsaa commented Dec 11, 2022

Uh oh!

Uh oh!

Uh oh!

penmetsaa Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

penmetsaa Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

temideoye Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

temideoye Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

penmetsaa Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

penmetsaa left a comment

Choose a reason for hiding this comment

Uh oh!

temideoye commented Jan 4, 2023

Uh oh!

penmetsaa commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penmetsaa commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

temideoye commented Jan 4, 2023

Uh oh!

penmetsaa commented Jan 5, 2023

Uh oh!

temideoye commented Jan 5, 2023

Uh oh!

temideoye commented Jan 5, 2023

Uh oh!

penmetsaa commented Jan 6, 2023

Uh oh!

nekopsykose Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

temideoye Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

temideoye Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

penmetsaa commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nekopsykose commented Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

temideoye commented Sep 9, 2022 •

edited by penmetsaa

Loading

penmetsaa commented Jan 4, 2023 •

edited

Loading

penmetsaa commented Jan 4, 2023 •

edited

Loading

temideoye Jan 10, 2023 •

edited

Loading

penmetsaa commented Jan 10, 2023 •

edited

Loading