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

De-duplicate .proto file processing #20537

Merged
merged 2 commits into from Oct 25, 2019
Merged

De-duplicate .proto file processing #20537

merged 2 commits into from Oct 25, 2019

Conversation

KyleFromKitware
Copy link
Contributor

@KyleFromKitware KyleFromKitware commented Oct 9, 2019 •
edited

.proto files that are used by more than one target have their code
files generated multiple times, which causes issues with
add_custom_command(). De-duplicate them so they're only generated
once.

Fixes: #12379

@nicolasnoble
@zackgalbreath

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 9, 2019 •
edited

CLA Check
The committers are authorized under a signed CLA.

@jtattermusch jtattermusch self-assigned this Oct 11, 2019
@jtattermusch jtattermusch added lang/c++ kind/enhancement release notes: yes Indicates if PR needs to be in release notes kokoro:run labels Oct 11, 2019
@jtattermusch
Copy link
Contributor

a ton of tests are failing

CMake Error at CMakeLists.txt:3671 (target_link_libraries):
  Object library target "grpc++_reflection_pb" may not link to anything.


-- Configuring incomplete, errors occurred!
See also "/var/local/git/grpc/cmake/build/CMakeFiles/CMakeOutput.log".
See also "/var/local/git/grpc/cmake/build/CMakeFiles/CMakeError.log".

@zackgalbreath
Copy link
Contributor

We will also have to make corresponding changes to templates/CMakeLists.txt.template. Let me know if you need help with this part.

@KyleFromKitware
Copy link
Contributor Author

It looks like this is happening because target_link_libraries() could not be used with object libraries until CMake 3.12. I will try building it as a static library instead to work around this restriction, and using the object files from that.

@KyleFromKitware
Copy link
Contributor Author

Argh. $<TARGET_OBJECTS:tgt> doesn't work on static libraries until 3.15.

@KyleFromKitware
Copy link
Contributor Author

And it doesn't look like there's a good way to merge two static libraries into one.

Unfortunately, this means I will probably have to revert to the original plan of using add_custom_target() to do the source file generation.

@KyleFromKitware KyleFromKitware changed the title Fix parallel build with generated Protobuf files De-duplicate .proto file processing Oct 15, 2019
@KyleFromKitware
Copy link
Contributor Author

Looks like part of the problem was that the .proto files in question were being generated by three different add_custom_command() calls. I've reworked the CMakeLists.txt.template logic to de-duplicate them.

@KyleFromKitware
Copy link
Contributor Author

@tudor Please try this and let me know if it works for you.

jtattermusch
jtattermusch reviewed Oct 18, 2019
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

I spawned the tests - let's see what they say.
Also added a few comments.

templates/CMakeLists.txt.template Show resolved Hide resolved
templates/CMakeLists.txt.template Show resolved Hide resolved
jtattermusch
jtattermusch reviewed Oct 24, 2019
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

One last nit and then we're good to go I think.

@jtattermusch
Copy link
Contributor

Ok, and sanity check fails because CMakeLists.txt hasn't been regenerated? https://source.cloud.google.com/results/invocations/10729965-08e3-4676-8987-b5451dc045e7/targets/github%2Fgrpc%2Frun_tests%2Fsanity_linux_dbg_native%2Ftools%2Fbuildgen%2Fgenerate_projects.sh/tests

@KyleFromKitware
De-duplicate .proto file processing
ecdd1c9 
.proto files that are used by more than one target have their code
files generated multiple times, which causes issues with
add_custom_command(). De-duplicate them so they're only generated
once.
@KyleFromKitware
Regenerate CMakeLists.txt
b94dd62
@KyleFromKitware
Copy link
Contributor Author

Ok, and sanity check fails because CMakeLists.txt hasn't been regenerated?

Weird. I'm guessing this was probably because it was several hundred commits behind on master. I've rebased onto the latest master now, so it should be good to go.

jtattermusch
jtattermusch approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM (once tests pass)

@jtattermusch
Copy link
Contributor

Known failures:
#20694
#20545
#20703

@jtattermusch jtattermusch merged commit 4284d30 into grpc:master Oct 25, 2019
hcaseyal
hcaseyal reviewed Oct 25, 2019
View reviewed changes
Copy link
Contributor

@hcaseyal hcaseyal left a comment

Choose a reason for hiding this comment

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

LGTM

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hcaseyal hcaseyal hcaseyal left review comments

@jtattermusch jtattermusch jtattermusch approved these changes

Assignees

@jtattermusch jtattermusch

Labels
kind/enhancement lang/c++ release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parallel cmake builds may fail with .../reflection.pb.h: No such file or directory
5 participants
@KyleFromKitware @jtattermusch @zackgalbreath @hcaseyal @grpc-kokoro