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

cleanup: Use only find_package to find dependencies. #2967

Merged
merged 18 commits into from
Aug 14, 2019

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Aug 13, 2019

This PR changes the CMake files to use find_package() exclusively when
looking for dependencies. To automatically download the dependencies the
project can be embedded in a larger "super build".

There are still some places where we use find_package() through a helper
file, because different versions of the CMake module (or config file) behave
differently and we want to manage those differences in one place.

This fixes #2802


This change is Reviewable

@coryan coryan added the type: cleanup An internal cleanup or hygiene concern. label Aug 13, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 13, 2019
@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #2967 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2967      +/-   ##
==========================================
+ Coverage   90.66%   90.66%   +<.01%     
==========================================
  Files         299      299              
  Lines       20204    20213       +9     
==========================================
+ Hits        18317    18326       +9     
  Misses       1887     1887
Impacted Files Coverage Δ
google/cloud/storage/well_known_parameters.h 71.11% <0%> (-4.45%) ⬇️
...le/cloud/bigtable/internal/async_retry_unary_rpc.h 64.15% <0%> (-1.24%) ⬇️
google/cloud/bigtable/row_range.h 100% <0%> (ø) ⬆️
...gle/cloud/storage/internal/access_control_common.h 100% <0%> (ø) ⬆️
google/cloud/storage/object_metadata.h 100% <0%> (ø) ⬆️
google/cloud/bigtable/app_profile_config.h 100% <0%> (ø) ⬆️
google/cloud/storage/well_known_headers.h 85.18% <0%> (+7.4%) ⬆️
google/cloud/storage/internal/generic_request.h 86.48% <0%> (+8.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edff1ff...dbb0f75. Read the comment docs.

@coryan
Copy link
Contributor Author

coryan commented Aug 14, 2019

/cc: @tmatsuo @remyabel because you are interested in this type of change.

INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
cmake/FindProtobufWithTargets.cmake Show resolved Hide resolved
@coryan coryan merged commit be6453a into googleapis:master Aug 14, 2019
@coryan
Copy link
Contributor Author

coryan commented Aug 14, 2019

@remyabel @googleapis/yoshi-cpp if you are doing development for google-cloud-cpp note that the build instructions changed. Please check doc/setup-cmake-environment.md.

@coryan coryan deleted the use-only-find-package branch August 14, 2019 16:29
FindProtobufWithTargets
-------------------

A module to use `Protobuf` with less complications.
Copy link

Choose a reason for hiding this comment

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

Nit: since this is following RST syntax, it should use two backticks.

- It fallsback on the ``FindProtobuf`` module if the config files are not found.
- It populates any missing targets and their properties.

The following :prop_tgt:`IMPORTED` targets are defined:
Copy link

Choose a reason for hiding this comment

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

Nit: :prop_tgt is a Sphinx directive. We probably don't care about it, we can just do IMPORTED with two backticks.

@@ -45,7 +45,7 @@ if (TARGET storage-docs AND TARGET cloud-docs)
endif ()

include(IncludeNlohmannJson)
Copy link

Choose a reason for hiding this comment

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

@coryan Can we use the cmake files provided by the nlohmann json package, instead of fetching it manually via cmake?

$out/include
$out/include/nlohmann
$out/include/nlohmann/json.hpp
$out/lib
$out/lib/cmake
$out/lib/cmake/nlohmann_json
$out/lib/cmake/nlohmann_json/nlohmann_jsonConfig.cmake
$out/lib/cmake/nlohmann_json/nlohmann_jsonConfigVersion.cmake
$out/lib/cmake/nlohmann_json/nlohmann_jsonTargets.cmake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2874. We have not had time to do that, but yes, it makes sense.

Copy link

@flokli flokli Oct 5, 2019

Choose a reason for hiding this comment

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

Thanks, subscribed to that issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify CMake files to always use find_package().
4 participants