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.

Sign up for GitHub

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

Jump to bottom

Proto libraries in external/googleapis/ are always compiled #8022

Closed
coryan opened this issue Jan 19, 2022 · 5 comments · Fixed by #12506
Closed

Proto libraries in external/googleapis/ are always compiled #8022

coryan opened this issue Jan 19, 2022 · 5 comments · Fixed by #12506
Assignees
@dbolduc
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@coryan
Copy link
Contributor

coryan commented Jan 19, 2022 •
edited
Loading

The proto libraries in external/googleapis/ are always compiled (unless gRPC is completely disabled, but that is Okay).

Most of them are small libraries with only one proto file in them, and many of them (things like "api.proto") are used by almost all libraries. Not particularly onerous, but there are a few (e.g. google_cloud_cpp_cloud_texttospeech, google_cloud_cpp_monitoring) that are larger. We could move them to google/cloud/${service} and compile them only when needed.

We need to have a plan on how to deal with these larger libraries.

We should also consider merging all the smaller libraries into a single .a file. "One library per proto" makes no sense for the CMake build + install model, creates friction for customers, and (if we solve the many other problems) is very inconvenient for DLLs exports. We can be backwards compatible with a series of interface-only targets.
@coryan coryan added the type: cleanup An internal cleanup or hygiene concern. label Jan 19, 2022
@coryan coryan changed the title Proto libraries in external/googleapis/ are always compiled and duplicated Proto libraries in external/googleapis/ are always compiled Jan 30, 2022
@coryan coryan removed this from the All GCP Services have a GA library milestone Jan 31, 2022
@coryan
Copy link
Contributor Author

coryan commented Jul 7, 2022

We still want to clean this up, maybe now that we have CMake >= 3.10 we can fix this.

@dbolduc
Copy link
Member

dbolduc commented Nov 4, 2022

A customer is asking for this. See the discussion in: #10174

@coryan
Copy link
Contributor Author

coryan commented Mar 22, 2023

Realistically we will not have time to work on this. Closing for now.

@coryan coryan closed this as completed Mar 22, 2023
@coryan coryan added the cpp: backlog While desirable, we do not have time to work on this for the foreseeable future. label Mar 22, 2023
@coryan coryan mentioned this issue Aug 18, 2023
Merged
This was referenced Aug 20, 2023
Closed
Merged
Merged
@dbolduc
Copy link
Member

dbolduc commented Aug 23, 2023

We discussed this, and decided that building extra protos is in fact a bug. It is an unnecessary burden on customers who build with cmake and do not want to compile these extra protos.

@dbolduc dbolduc reopened this Aug 23, 2023
@dbolduc dbolduc mentioned this issue Aug 24, 2023
Merged
@dbolduc
Copy link
Member

dbolduc commented Aug 24, 2023

Here is what the changes for dialogflow will look like:

dbolduc@e2b7b0d

Googlers can see this before/after of a full install:

BEFORE: https://paste.googleplex.com/6469566354948096
AFTER: https://paste.googleplex.com/4781980355919872

Also, I verified that if I install other libraries, the dialogflow protos are not installed. yay.

I need to think about how we want to test this... Maybe we...

  1. check that the legacy proto libraries' pkgconfig files are installed
  2. build some executable that we link against all of the legacy proto libraries.

@dbolduc dbolduc mentioned this issue Aug 24, 2023
Merged
@dbolduc dbolduc self-assigned this Aug 24, 2023
This was referenced Aug 24, 2023
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 25, 2023
Merged
Merged
Merged
Merged
Merged
Merged
dbolduc added a commit to dbolduc/google-cloud-cpp that referenced this issue Aug 28, 2023
@dbolduc
doc: CHANGELOG entry for googleapis#8022
4982eaa
This was referenced Aug 28, 2023
Merged
Closed
Merged
Merged
Merged
dbolduc referenced this issue Aug 30, 2023
@scotthart
refactor(iam): move iam_v2_policy_protos to a separate lib (#12413)
4d0271b
@dbolduc dbolduc mentioned this issue Aug 31, 2023
Merged
dbolduc added a commit to dbolduc/google-cloud-cpp that referenced this issue Aug 31, 2023
@dbolduc
doc: CHANGELOG entry for googleapis#8022
e019e07
dbolduc added a commit to dbolduc/google-cloud-cpp that referenced this issue Aug 31, 2023
@dbolduc
doc: CHANGELOG entry for googleapis#8022
b04da84
@coryan coryan mentioned this issue Aug 31, 2023
Merged
This was referenced Aug 31, 2023
Merged
Merged
Merged
dbolduc added a commit that referenced this issue Sep 1, 2023
@dbolduc
doc: CHANGELOG entry for #8022 (#12507)
5727177 
* doc: CHANGELOG entry for #8022

* checkers

* address review comments

* for example for example

* oops
@dbolduc dbolduc removed the cpp: backlog While desirable, we do not have time to work on this for the foreseeable future. label Feb 8, 2024
@dbolduc dbolduc mentioned this issue Apr 22, 2024
Closed
3 tasks
@dbolduc dbolduc mentioned this issue Aug 12, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dbolduc dbolduc

Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(cmake)!: only compile protos if asked - iam admin dbolduc/google-cloud-cpp
2 participants
@coryan @dbolduc