Skip to content

Added the clang-format formatter for generated C++ files [CTT-713]#558

Closed
ihsandemir wants to merge 2 commits into
hazelcast:masterfrom
ihsandemir:clang-format
Closed

Added the clang-format formatter for generated C++ files [CTT-713]#558
ihsandemir wants to merge 2 commits into
hazelcast:masterfrom
ihsandemir:clang-format

Conversation

@ihsandemir
Copy link
Copy Markdown
Collaborator

@ihsandemir ihsandemir commented Oct 21, 2025

Added the clang-format formatter for generated C++ files.

Also, fixed a duplication in type mapping for type Version.

Copied the file .clang-format from cpp client repo.

Also, fixed a duplication in type mapping for type `Version`.
@hz-devops-test
Copy link
Copy Markdown

The job Client-Protocol-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file
Traceback (most recent call last):
File "/usr/local/lib/python3.7/site-packages/pip/_internal/cli/base_command.py", line 179, in main
status = self.run(options, args)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/commands/install.py", line 315, in run
resolver.resolve(requirement_set)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/resolve.py", line 131, in resolve
self._resolve_one(requirement_set, req)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/resolve.py", line 294, in _resolve_one
abstract_dist = self._get_abstract_dist_for(req_to_install)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/resolve.py", line 242, in _get_abstract_dist_for
self.require_hashes
File "/usr/local/lib/python3.7/site-packages/pip/_internal/operations/prepare.py", line 349, in prepare_linked_requirement
abstract_dist.prep_for_dist(finder, self.build_isolation)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/operations/prepare.py", line 109, in prep_for_dist
self.req.load_pyproject_toml()
File "/usr/local/lib/python3.7/site-packages/pip/_internal/req/req_install.py", line 489, in load_pyproject_toml
str(self)
File "/usr/local/lib/python3.7/site-packages/pip/_internal/pyproject.py", line 66, in load_pyproject_toml
pp_toml = pytoml.load(f)
File "/usr/local/lib/python3.7/site-packages/pip/_vendor/pytoml/parser.py", line 11, in load
return loads(fin.read(), translate=translate, object_pairs_hook=object_pairs_hook, filename=getattr(fin, 'name', repr(fin)))
File "/usr/local/lib/python3.7/site-packages/pip/_vendor/pytoml/parser.py", line 24, in loads
ast = _p_toml(src, object_pairs_hook=object_pairs_hook)
File "/usr/local/lib/python3.7/site-packages/pip/_vendor/pytoml/parser.py", line 340, in _p_toml
s.expect_eof()
File "/usr/local/lib/python3.7/site-packages/pip/_vendor/pytoml/parser.py", line 125, in expect_eof
return self._expect(self.consume_eof())
File "/usr/local/lib/python3.7/site-packages/pip/_vendor/pytoml/parser.py", line 165, in _expect
raise TomlError('msg', self._pos[0], self._pos[1], self._filename)
pip._vendor.pytoml.core.TomlError: /tmp/pip-install-972c8w0z/clang-format/pyproject.toml(38, 1): msg�[0m
�[33mYou are using pip version 19.0.3, however version 24.0 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.�[0m

Copy link
Copy Markdown
Contributor

@JackPGreen JackPGreen left a comment

Choose a reason for hiding this comment

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

I didn't realise you were working on this - I was working on the same problem in https://github.com/JackPGreen/hazelcast-client-protocol/tree/format-cpp-with-clang-format. Yours works just as well though.

Comment thread .clang-format
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An alternative to duplicating the file is passing root_dir into generate_codecs and then using just referencing it (e.g. os.path.join(root_dir, ".clang-format")

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

verify

@hz-devops-test
Copy link
Copy Markdown

The job Client-Protocol-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

verify

@hz-devops-test
Copy link
Copy Markdown

The job Client-Protocol-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

@JackPGreen
Copy link
Copy Markdown
Contributor

@ihsandemir I suspect the Python runner is too old.

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

ihsandemir commented Oct 22, 2025

@ihsandemir I suspect the Python runner is too old.

I think we should also switch to github actions rather than jenkins. Well I just saw your PR.

@ihsandemir ihsandemir changed the title Added the clang-format formatter for generated C++ files Added the clang-format formatter for generated C++ files [CTT-713] Oct 22, 2025
@yuce
Copy link
Copy Markdown
Collaborator

yuce commented Oct 23, 2025

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed.
Why not add it to the hazelcast-cpp-client repo?

@yuce
Copy link
Copy Markdown
Collaborator

yuce commented Oct 23, 2025

Note that we use a similar tool for Python called black to check/format all of the Python Client code (not just the codecs) in the hazelcast-python-client repo.

JackPGreen added a commit that referenced this pull request Oct 23, 2025
#558 failed
the PR builder because it runs on Jenkins, inside a custom Python-3.7
Docker image.

[Python 3.7 has been end-of-life for 2
years](https://devguide.python.org/versions/), and is too old to support
the proposed changes.

We _could_ upgrade to a [newer Docker
image](https://hub.docker.com/r/hazelcast/jenkins-python-client-3.9) (or
even [make a new
one](https://github.com/hazelcast/DevOps/tree/master/dockerfiles/clients/python)),
but _I think_ it would be easier to migrate to GitHub Actions instead -
the runner _already includes_ a recent Python installation.

Changes:
- create new GitHub action
- dynamically determine languages to execute/test - previously markdown
was missed

Post-merge actions:
- [ ] disable [Jenkins
job](https://jenkins.hazelcast.com/job/Client-Protocol-pr-builder/)
@ihsandemir
Copy link
Copy Markdown
Collaborator Author

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed. Why not add it to the hazelcast-cpp-client repo?

We have it in the cpp repo as well.

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

Note that we use a similar tool for Python called black to check/format all of the Python Client code (not just the codecs) in the hazelcast-python-client repo.

we can integrate it into generator in a similar way.

@yuce
Copy link
Copy Markdown
Collaborator

yuce commented Oct 23, 2025

We have it in the cpp repo as well.
we can integrate it into generator in a similar way.

Why is that necessary? The code is already formatted in the client repos.

@JackPGreen
Copy link
Copy Markdown
Contributor

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed.

What do you mean?

The original issue was that the hazelcast-client-protocol generated sources don't match what's in the C++ repo (because of formatting) and this was one way to address that - specifically, it was harder for me to check that the generated code was actually correct and not outdated/malicious.

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed. Why not add it to the hazelcast-cpp-client repo?

We have it in the cpp repo as well.

Maybe an alternative is to:

  • make the C++ repo autoformat (e.g. a post commit hook)
  • assert the code is correctly formatted in the PR builder

@yuce
Copy link
Copy Markdown
Collaborator

yuce commented Oct 23, 2025

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed.

What do you mean?

The generated files are not committed to "this" repo, so it's not very useful to format them.
Also, since the .cpp files are generated, it would be better to change the template that is used to generate them, so they match a certain style.

assert the code is correctly formatted in the PR builder

Yes, that's the approach we have for the Python Client.

@JackPGreen
Copy link
Copy Markdown
Contributor

I think adding clang-formatter to this repo is the wrong approach, since generated .cpp files are not committed.

What do you mean?

The generated files are not committed to "this" repo, so it's not very useful to format them. Also, since the .cpp files are generated, it would be better to change the template that is used to generate them, so they match a certain style.

This isn't possible - the formatting is adjusted based on the line length - e.g. a long method name (the method name being dynamic based on the service) may have it's parameters wrapped, a short one may not.

@ihsandemir
Copy link
Copy Markdown
Collaborator Author

Closing this PR since it was rejected by @yuce . Carried the small fix for duplicate type mapping to another PR.

@ihsandemir ihsandemir closed this Oct 28, 2025
@ihsandemir ihsandemir deleted the clang-format branch October 28, 2025 10:18
gareth-johnston pushed a commit to gareth-johnston/hazelcast-client-protocol that referenced this pull request Jan 14, 2026
hazelcast#558 failed
the PR builder because it runs on Jenkins, inside a custom Python-3.7
Docker image.

[Python 3.7 has been end-of-life for 2
years](https://devguide.python.org/versions/), and is too old to support
the proposed changes.

We _could_ upgrade to a [newer Docker
image](https://hub.docker.com/r/hazelcast/jenkins-python-client-3.9) (or
even [make a new
one](https://github.com/hazelcast/DevOps/tree/master/dockerfiles/clients/python)),
but _I think_ it would be easier to migrate to GitHub Actions instead -
the runner _already includes_ a recent Python installation.

Changes:
- create new GitHub action
- dynamically determine languages to execute/test - previously markdown
was missed

Post-merge actions:
- [ ] disable [Jenkins
job](https://jenkins.hazelcast.com/job/Client-Protocol-pr-builder/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants