You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/12 10:59:01 UTC
[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #45: Fix code format check using clang-format 10.0.0
BewareMyPower opened a new pull request, #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45
### Motivation
The `docker-build.sh` cannot build this library because the code format check failed. The reason is that the Docker image's `clang-format` version is 10.0.0, while the default clang-format version of Ubuntu-20.04 is 11.0.0-2~ubuntu20.04.1.
### Modifications
Fix the format check by running `docker-format.sh`.
For the CI, configure `-DCLANG_FORMAT_VERSION=10` so that CMake will find `/usr/bin/clang-format-10` instead of the default `clang-format`.
Modify the documents to note that the repo uses clang-format 10.0.0.
I also found another file `gf2.hpp` is not well formatted because the suffix is `*.hpp`, which cannot be found by CMake. Simply change the suffix to `*.h`.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [ ] `doc-not-needed`
(Please explain why)
- [x] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1001317746
##########
README.md:
##########
@@ -222,6 +222,8 @@ cd tests
## Requirements for Contributors
-It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` 6.0+ to format files. `make format` automatically formats the files.
+It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` **11** to format files. `make format` automatically formats the files.
Review Comment:
Thanks for your suggestions, I will address them later after since the CI is very unstable now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat commented on pull request #45: Fix code format check using clang-format 10.0.0
Posted by GitBox <gi...@apache.org>.
merlimat commented on PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#issuecomment-1276794519
When installing clang-format on current macos & ubuntu, the default is now 11.0. We don't need to worry about the `docker-format.sh` which is already removed. I think we should stick with that and we should set the format in the `CMakeFiles.txt` too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#issuecomment-1285123998
@RobertIndie The previous `docker-build.sh` and `docker-test.sh` scripts were removed because now we build and run tests on a Ubuntu based GitHub runner. `docker-format.sh` is only used to format code. However, it also uses a manually maintained pre-built Docker image that contains all dependencies, like other `docker-xxx` scripts. It's too heavy.
Though this PR adds `docker-format.sh` back, it's just convenient for non-Ubuntu users. It's lightweight because it only installs a `clang-format-11` based on a official `ubuntu:20.04` image and doesn't run `cmake`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #45: Fix code format check using clang-format 10.0.0
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#issuecomment-1276948836
@merlimat Yeah, removing these scripts looks good to me too.
> I think we should stick with that and we should set the format in the CMakeFiles.txt too.
So this PR would aim at sticking the clang-format to 11.0.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] momo-jun commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
momo-jun commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1001306812
##########
README.md:
##########
@@ -222,6 +222,8 @@ cd tests
## Requirements for Contributors
-It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` 6.0+ to format files. `make format` automatically formats the files.
+It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` **11** to format files. `make format` automatically formats the files.
Review Comment:
```suggestion
It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ clients use `clang-format` **11** to format files. `make format` automatically formats the files.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1001318497
##########
README.md:
##########
@@ -222,6 +222,8 @@ cd tests
## Requirements for Contributors
-It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` 6.0+ to format files. `make format` automatically formats the files.
+It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` **11** to format files. `make format` automatically formats the files.
+
+For Ubuntu users, you can install `clang-format-11` via `apt install clang-format-11`. For other users, run `./build-support/docker-format.sh` if you have Docker installed.
We welcome contributions from the open source community, kindly make sure your changes are backward compatible with GCC 4.8 and Boost 1.53.
Review Comment:
The dependency compatibility is hard to test. line 229 is added in a very early time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1001238813
##########
Dockerfile.format:
##########
@@ -0,0 +1,35 @@
+#
Review Comment:
Moved. But I don't get why will the image be recreated each time any cpp file is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1000812350
##########
Dockerfile.format:
##########
@@ -0,0 +1,35 @@
+#
Review Comment:
Maybe we could move this into `build-support`. That would also help to not recreate the image each time any cpp file is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat merged pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1000302183
##########
README.md:
##########
@@ -222,6 +222,8 @@ cd tests
## Requirements for Contributors
-It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` 6.0+ to format files. `make format` automatically formats the files.
+It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` **11** to format files. `make format` automatically formats the files.
+
+For Ubuntu users, you can install `clang-format-11` via `apt install clang-format-11`. For other users, run `./docker-format.sh` if you have Docker installed.
Review Comment:
@RobertIndie See here. Even if I am a Ubuntu user now, using `./docker-format.sh` could be faster now because I don't need to run `cmake`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#issuecomment-1283747137
I have updated this PR, PTAL again @merlimat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] momo-jun commented on a diff in pull request #45: Stick clang-format version to 11 and add back docker-format.sh
Posted by GitBox <gi...@apache.org>.
momo-jun commented on code in PR #45:
URL: https://github.com/apache/pulsar-client-cpp/pull/45#discussion_r1001306604
##########
README.md:
##########
@@ -222,6 +222,8 @@ cd tests
## Requirements for Contributors
-It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` 6.0+ to format files. `make format` automatically formats the files.
+It's required to install [LLVM](https://llvm.org/builds/) for `clang-tidy` and `clang-format`. Pulsar C++ client use `clang-format` **11** to format files. `make format` automatically formats the files.
+
+For Ubuntu users, you can install `clang-format-11` via `apt install clang-format-11`. For other users, run `./build-support/docker-format.sh` if you have Docker installed.
We welcome contributions from the open source community, kindly make sure your changes are backward compatible with GCC 4.8 and Boost 1.53.
Review Comment:
Do we have requirements on the version number of Boost? If yes, it needs to be shown in the [Requirements](https://github.com/apache/pulsar-client-cpp#requirements) section.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org