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

Include googletest as a submodule #3993

Merged
merged 4 commits into from Mar 26, 2018
Merged

Include googletest as a submodule #3993

merged 4 commits into from Mar 26, 2018

Conversation

@coryan
Copy link
Contributor

@coryan coryan commented Dec 3, 2017

This fixes #236. Currently it patches googletest to support autoconf / automake builds, because google/googletest#776. I have submitted google/googletest#1341 to fix that problem, but until it is accepted the patches work around the underlying googletest issue.

The builds with cmake and bazel do not need such hacks.

-Wno-writable-strings removes 230 "ISO C++11 does not allow conversion from
string literal to 'char *'" warnings from TensorFlow test / build output.
These happen because pyext/ sources pass string literals to Python C API data
structures, e.g. PyGetSetDef, which for some reason were designed to not have
the const qualifier.
@grpc-kokoro
Copy link

@grpc-kokoro grpc-kokoro commented Dec 3, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

1 similar comment
@grpc-kokoro
Copy link

@grpc-kokoro grpc-kokoro commented Dec 3, 2017

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

@xfxyjwf xfxyjwf self-requested a review Dec 6, 2017
@coryan
Copy link
Contributor Author

@coryan coryan commented Jan 10, 2018

@xfxyjwf PTAL. The changes in googletest are in, it works correctly with autoconf/automake now, and the builds are passing.

Copy link
Contributor

@xfxyjwf xfxyjwf left a comment

Please squash the commits into one and update the commit message.

@@ -1,3 +1,7 @@
[submodule "third_party/benchmark"]
path = third_party/benchmark
url = https://github.com/google/benchmark.git
[submodule "third_party/googletest"]
Copy link
Contributor

@xfxyjwf xfxyjwf Jan 10, 2018

Choose a reason for hiding this comment

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

Can you pin this submodule to a specific googletest version (a tag or a commit)?

Copy link
Contributor Author

@coryan coryan Jan 10, 2018

Choose a reason for hiding this comment

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

I believe it is already. The SHA1 of the commit a submodule points to is stored directly in the git database:

https://stackoverflow.com/a/5033973

You can see that it already points at a SHA1 here:

https://github.com/coryan/protobuf/tree/fix-issue-236-gmock-as-submodule/third_party

${protobuf_source_dir}/gmock/include
${googlemock_source_dir}
${googletest_source_dir}
${googletest_source_dir}/include
Copy link
Contributor

@xfxyjwf xfxyjwf Jan 10, 2018

Choose a reason for hiding this comment

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

Remove duplicated lines here.

Copy link
Contributor Author

@coryan coryan Jan 10, 2018

Choose a reason for hiding this comment

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

Sorry, I do not see the duplicates. Line 12 points to googlemock, while line 13 is googletest. Or maybe you wanted be to look elsewhere and I missed it?

@coryan
Copy link
Contributor Author

@coryan coryan commented Jan 10, 2018

PTAL. Ping me if I am just being dumb and do not see what is it you are asking me to do.

@coryan
Copy link
Contributor Author

@coryan coryan commented Mar 22, 2018

Ping, is there something else I should be doing here?

@xfxyjwf xfxyjwf self-assigned this Mar 22, 2018
@xfxyjwf xfxyjwf added this to To Do in Weekly Fixit via automation Mar 26, 2018
@xfxyjwf xfxyjwf merged commit 3c5442a into protocolbuffers:master Mar 26, 2018
30 of 32 checks passed
Weekly Fixit automation moved this from To Do to Done Mar 26, 2018
zhoutwo pushed a commit to zhoutwo/protobuf that referenced this issue Apr 25, 2020
Add googletest as a submodule in third_party/googletest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Weekly Fixit
  
Done
Linked issues

Successfully merging this pull request may close these issues.

6 participants