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

Stick clang-format version to 11 and add back docker-format.sh #45

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Stick clang-format version to 11 and add back docker-format.sh #45

merged 2 commits into from
Oct 24, 2022

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Oct 12, 2022 •
edited
Loading

Motivation

Currently the code is formatted by clang-format 11. If it's formatted
by other versions of clang-format, the style might be a little
different.

Modifications

  • Add back docker-format.sh and the associated Dockerfile.format.
    This script build the image with python3 and clang-format-11
    installed and the entrypoint is formatting the code.
  • Explain the clang-format version in README.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower BewareMyPower added documentation Improvements or additions to documentation build release/3.0 labels Oct 12, 2022
@BewareMyPower BewareMyPower added this to the 3.0.0 milestone Oct 12, 2022
@BewareMyPower BewareMyPower self-assigned this Oct 12, 2022
@merlimat
Copy link
Contributor

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.

@BewareMyPower BewareMyPower modified the milestones: 3.0.0, 3.1.0 Oct 13, 2022
@BewareMyPower
Copy link
Contributor Author

@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.

@BewareMyPower BewareMyPower marked this pull request as draft October 13, 2022 02:33
@BewareMyPower BewareMyPower force-pushed the bewaremypower/fix-docker-build branch from de01549 to 239473e Compare October 19, 2022 09:59
@BewareMyPower BewareMyPower changed the title Fix code format check using clang-format 10.0.0 Stick clang-format version to 11 and add back docker-format.sh Oct 19, 2022
@BewareMyPower BewareMyPower force-pushed the bewaremypower/fix-docker-build branch from 239473e to d7cc680 Compare October 19, 2022 10:01
@BewareMyPower BewareMyPower marked this pull request as ready for review October 19, 2022 10:02
@BewareMyPower
Copy link
Contributor Author

I have updated this PR, PTAL again @merlimat

RobertIndie
RobertIndie reviewed Oct 20, 2022
View reviewed changes
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

Yeah, removing these scripts looks good to me too.

According to the discussion, seems we will not worry about the docker-format.sh. But this file still be added in this PR. Is there some context I have missed?

@BewareMyPower
Copy link
Contributor Author

@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.

BewareMyPower
BewareMyPower commented Oct 20, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
merlimat
merlimat reviewed Oct 20, 2022
View reviewed changes
Dockerfile.format Outdated
@@ -0,0 +1,35 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could move this into build-support. That would also help to not recreate the image each time any cpp file is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved. But I don't get why will the image be recreated each time any cpp file is changed.

momo-jun
momo-jun reviewed Oct 21, 2022
View reviewed changes
README.md
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have requirements on the version number of Boost? If yes, it needs to be shown in the Requirements section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dependency compatibility is hard to test. line 229 is added in a very early time.

momo-jun
momo-jun reviewed Oct 21, 2022
View reviewed changes
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestions, I will address them later after since the CI is very unstable now.

@BewareMyPower
Stick clang-format version to 11 and add back docker-format.sh
bd80d94 
### Motivation

Currently the code is formatted by `clang-format` 11. If it's formatted
by other versions of `clang-format`, the style might be a little
different.

### Modifications

- Add back `docker-format.sh` and the associated `Dockerfile.format`.
  This script build the image with `python3` and `clang-format-11`
  installed and the entrypoint is formatting the code.
- Explain the `clang-format` version in README.
@BewareMyPower BewareMyPower force-pushed the bewaremypower/fix-docker-build branch from 68155b8 to e17157f Compare October 24, 2022 19:02
@merlimat merlimat merged commit ecc1995 into apache:main Oct 24, 2022
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-docker-build branch October 25, 2022 03:13
BewareMyPower added a commit that referenced this pull request Nov 1, 2022
@BewareMyPower
Stick clang-format version to 11 and add back docker-format.sh (#45)
e557cca 
* Stick clang-format version to 11 and add back docker-format.sh

### Motivation

Currently the code is formatted by `clang-format` 11. If it's formatted
by other versions of `clang-format`, the style might be a little
different.

### Modifications

- Add back `docker-format.sh` and the associated `Dockerfile.format`.
  This script build the image with `python3` and `clang-format-11`
  installed and the entrypoint is formatting the code.
- Explain the `clang-format` version in README.

* Move Dockerfile and docker-format.sh to build-support

(cherry picked from commit ecc1995)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@momo-jun momo-jun momo-jun left review comments

@shibd shibd shibd approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@merlimat merlimat merlimat approved these changes

@Demogorgon314 Demogorgon314 Awaiting requested review from Demogorgon314

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

Assignees

@BewareMyPower BewareMyPower

Labels
build cherry-picked/3.0 documentation Improvements or additions to documentation
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@BewareMyPower @merlimat @RobertIndie @shibd @momo-jun