refactor(cmake): asset uses library helper #12433
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #12433 +/- ##
==========================================
- Coverage 93.63% 93.62% -0.01%
==========================================
Files 2035 2035
Lines 180223 180223
==========================================
- Hits 168743 168740 -3
- Misses 11480 11483 +3 ☔ View full report in Codecov by Sentry. |
cmake/GoogleCloudCppLibrary.cmake
Outdated
| @@ -22,6 +22,11 @@ | |||
| # * GOOGLE_CLOUD_CPP_SERVICE_DIRS: a list of service directories within the | |||
| # library. | |||
| # | |||
| # The following conditional arguments can be supplied to handle edge cases: | |||
There was a problem hiding this comment.
Can this be an argument via cmake_parse_arguments()?
There was a problem hiding this comment.
Yes. Done.
This was a bit tricky to document. Hopefully it is clear.
Note that I am still setting DOXYGEN_EXTRA_INCLUDES in the parent scope. I do not intend to add that as an argument or document it in the helper function.
| @@ -19,10 +19,21 @@ | |||
| # * library: the short name of the library, e.g. `kms`. | |||
| # * display_name: the display name of the library, e.g. "Cloud Key Management | |||
| # Service (KMS) API" | |||
| # | |||
| # Additionally, we must set the following **variable** in the parent scope. We | |||
There was a problem hiding this comment.
For a separate PR, maybe we should have a SERVICE_DIRS list parameter and a boolean parameter that adds "" to it, something like WITH_LOCAL_SERVICE_DIR or WITH_FORWARDING_HEADERS
There was a problem hiding this comment.
Eh. I think that would add unnecessary complexity. What we have works and I am not trying to get cmake readability.
There was a problem hiding this comment.
Passing values through global variables in CMake is bad for the same reason that passing values in global variables is bad in all languages. This is not a good habit to get into.
Part of the work for #12428
assetis a weird one. We always compile thatorgpolicyproto. Note that it is just messages. So it is not a huge deal, and I don't expect us to treat it any differently in light of #8022This change is