Clang-formatted the C++ SDK files. #716

devjgm · 2019-04-17T00:06:30Z

This addresses part of #713. This seemed fairly uncontroversial. This PR makes zero semantic changes. It's only a whitespace/formatting change applied by clang-format -style=Google ....

While I expect no tests to fail from this change, I ran all the tests with cd build; make test as the instructions suggest and they did pass. However, I also introduced an intentional syntax error in the C++ code, and the tests still passed, so I'm not convinced that they're running. Is there some other way that I should/could run the C++ build/test on my local machine before sending a PR?

This change is

Also updated the CONTRIBUTING.md file to reflect a new rule about following the Google Style guide.

roberthbailey · 2019-04-17T00:14:23Z

/assign @dsazonoff @markmandel

roberthbailey · 2019-04-17T00:17:29Z

Did you try cd sdks/cpp; make build to at verify that the code compiles (for me that tells me I need to install cmake...)?

devjgm · 2019-04-17T00:28:02Z

That gives me a different error:

$ cd sdk/cpp
$ make build
...
-- Performing Test HAVE_POSIX_REGEX -- success
-- Performing Test HAVE_STEADY_CLOCK
-- Performing Test HAVE_STEADY_CLOCK -- success
CMake Error at CMakeLists.txt:177 (install):
  install TARGETS given target "address_sorting" which does not exist in this
  directory.


CMake Error at CMakeLists.txt:195 (install):
  install TARGETS given target "address_sorting" which does not exist in this
  directory.


-- Configuring incomplete, errors occurred!
See also "/home/jgm/github/devjgm/agones/sdks/cpp/.build/CMakeFiles/CMakeOutput.log".
See also "/home/jgm/github/devjgm/agones/sdks/cpp/.build/CMakeFiles/CMakeError.log".
Makefile:26: recipe for target 'build' failed
make: *** [build] Error 1

So, I suspect that the C++ build is currently broken and might not be running in the CI? Or possibly it's just that I don't know how to run it.

Note: the CMakeLists.txt file is currently requiring an unnecessarily modern version of cmake (more recent than are contained int apt repos). So I tweaked it[1] to allow cmake version 3.10.0, which should be more than recent enough. As a separate issue, I think the required cmake version should be relaxed.

[1]: My diff was:

diff --git a/sdks/cpp/CMakeLists.txt b/sdks/cpp/CMakeLists.txt
index 3fe83e63..f3b0d29a 100644
--- a/sdks/cpp/CMakeLists.txt
+++ b/sdks/cpp/CMakeLists.txt
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-cmake_minimum_required (VERSION 3.13.0)
+cmake_minimum_required (VERSION 3.10.0)
 
 project(agones VERSION 0.9.0 HOMEPAGE_URL https://github.com/GoogleCloudPlatform/agones LANGUAGES C CXX)

agones-bot · 2019-04-17T00:29:42Z

Build Failed 😱

Build Id: a7d2de66-4883-45e8-800b-003bcb91f62f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

dsazonoff · 2019-04-17T08:21:49Z

@devjgm what is your current build configuration (OS, cmake version, is gRPC pre-installed)? I'll take a look, because on Mac / Win everything is OK.

devjgm · 2019-04-17T12:55:52Z

@dsazonoff
I'm building on Debian Linux #1 SMP Debian 4.19.28-2rodete1 (2019-03-18 > 2018), cmake 3.12.1, gRPC is not pre-installed.

Also, should I commit this PR even though there appears to be a build failure (though I think it's unrelated).

dsazonoff · 2019-04-17T14:45:56Z

Also, should I commit this PR even though there appears to be a build failure (though I think it's unrelated).

I think yes, we can merge.
About failed build - probably there are some issues with updating submodules for gRPC repo that is downloaded by cmake? After cmake step you may check .build/grpc folder to be up to date with all gRPC submodules. I'll download a vm with Linux soon.
Another way - use main build scripts with Docker. You may try SDK_FOLDER=cpp make gen-sdk-grpc from build folder.

@markmandel please restart tests, I don't understand the reason why they failed. And approve/reject this pull request.

markmandel · 2019-04-17T18:57:42Z

If we are going to enforce Clang formatting, can we put the tooling in the C++ SDK image, and then create a make format target (or similar) so we can make this easy for developers who are working on the project?

agones-bot · 2019-04-17T19:15:09Z

Build Succeeded 👏

Build Id: 5c5a512a-8364-4aa4-aeef-d52075fd6db2

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.10.0-d497a61
image: gcr.io/agones-images/agones-sdk:0.10.0-d497a61
image: gcr.io/agones-images/agones-ping:0.10.0-d497a61
Linux C++ SDK (build): agonessdk-0.10.0-d497a61-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.10.0-d497a61.zip

A preview of the website (the last 30 builds are retained):

https://d497a61-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/716/head:pr_716 && git checkout pr_716
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-d497a61

agones-bot · 2019-04-17T20:24:53Z

Build Succeeded 👏

Build Id: aa0e7055-5cbc-4015-b761-cbb56a6a60ba

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.10.0-8aca53d
image: gcr.io/agones-images/agones-sdk:0.10.0-8aca53d
image: gcr.io/agones-images/agones-ping:0.10.0-8aca53d
Linux C++ SDK (build): agonessdk-0.10.0-8aca53d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.10.0-8aca53d.zip

A preview of the website (the last 30 builds are retained):

https://8aca53d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/716/head:pr_716 && git checkout pr_716
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-8aca53d

dsazonoff · 2019-04-17T20:51:53Z

If we are going to enforce Clang formatting, can we put the tooling in the C++ SDK image, and then create a make format target (or similar) so we can make this easy for developers who are working on the project?

I think that it should be as a test during a build, as was originally proposed. Because different developers uses different IDE's, and a lot for them already supports clang formatting. For example, most of time I use CLion, and it supports formatting on the fly, without need of running any build steps.

markmandel · 2019-04-17T22:06:13Z

Are we happy to merge this as is? Happy to approve, but deferring to people with more C++ experience.

devjgm · 2019-04-18T13:23:26Z

fyi, I'm trying to merge this. but every time I try to, the branch is out of date, so I update the branch, then the tests need to re-run, I wait for those, then the branch needs to be updated again :-) Trying again. I'll work one of these times.

markmandel

👍

agones-bot · 2019-04-18T13:43:34Z

Build Succeeded 👏

Build Id: 3b728d52-2151-4d81-b262-c03900e3ca04

The following development artifacts have been built, and will exist for the next 30 days:

image: gcr.io/agones-images/agones-controller:0.10.0-ebf8221
image: gcr.io/agones-images/agones-sdk:0.10.0-ebf8221
image: gcr.io/agones-images/agones-ping:0.10.0-ebf8221
Linux C++ SDK (build): agonessdk-0.10.0-ebf8221-linux-arch_64.tar.gz
SDK Server: agonessdk-server-0.10.0-ebf8221.zip

A preview of the website (the last 30 builds are retained):

https://ebf8221-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/716/head:pr_716 && git checkout pr_716
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-ebf8221

Clang-formatted the C++ SDK files.

d497a61

Also updated the CONTRIBUTING.md file to reflect a new rule about following the Google Style guide.

devjgm mentioned this pull request Apr 17, 2019

C++ SDK should follow Google Style #713

Closed

dsazonoff approved these changes Apr 17, 2019

View reviewed changes

Merge branch 'master' into clang-format-cpp

8aca53d

Merge branch 'master' into clang-format-cpp

ebf8221

markmandel approved these changes Apr 18, 2019

View reviewed changes

markmandel merged commit 17fc6d5 into googleforgames:master Apr 18, 2019

devjgm deleted the clang-format-cpp branch April 18, 2019 13:48

devjgm mentioned this pull request Apr 18, 2019

Adds a .clang-format file making Google style the default #724

Merged

markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Apr 27, 2019

markmandel added this to the 0.10.0 milestone Apr 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang-formatted the C++ SDK files. #716

Clang-formatted the C++ SDK files. #716

devjgm commented Apr 17, 2019 •

edited

Loading

roberthbailey commented Apr 17, 2019

roberthbailey commented Apr 17, 2019

devjgm commented Apr 17, 2019

agones-bot commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

devjgm commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

markmandel commented Apr 17, 2019

agones-bot commented Apr 17, 2019

agones-bot commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

markmandel commented Apr 17, 2019

devjgm commented Apr 18, 2019

markmandel left a comment

agones-bot commented Apr 18, 2019

Clang-formatted the C++ SDK files. #716

Clang-formatted the C++ SDK files. #716

Conversation

devjgm commented Apr 17, 2019 • edited Loading

roberthbailey commented Apr 17, 2019

roberthbailey commented Apr 17, 2019

devjgm commented Apr 17, 2019

agones-bot commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

devjgm commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

markmandel commented Apr 17, 2019

agones-bot commented Apr 17, 2019

agones-bot commented Apr 17, 2019

dsazonoff commented Apr 17, 2019

markmandel commented Apr 17, 2019

devjgm commented Apr 18, 2019

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Apr 18, 2019

devjgm commented Apr 17, 2019 •

edited

Loading