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

Use GH action for code style check #1828

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Use GH action for code style check #1828

merged 3 commits into from
Apr 14, 2023

Conversation

jubalh
Copy link
Member

@jubalh jubalh commented Apr 14, 2023

We should use a newer version of clang-format.
Note: https://github.com/marketplace/actions/clang-format-check

Instead of running clang-format outselves on the old Ubuntu version.
This let's us easily configure which version of clang-format we want to
execute.

Used action:
https://github.com/marketplace/actions/clang-format-check

Properly fix:
#1774
@jubalh jubalh changed the title WIP Use GH action for code style check Apr 14, 2023
@jubalh jubalh marked this pull request as ready for review April 14, 2023 19:47
@jubalh jubalh self-assigned this Apr 14, 2023
@jubalh jubalh added the tests label Apr 14, 2023
@jubalh jubalh added this to the next milestone Apr 14, 2023
@jubalh jubalh merged commit 6eacfcb into master Apr 14, 2023
@jubalh jubalh deleted the newclang branch April 14, 2023 20:01
@sjaeckel
Copy link
Member

Is there a way to run clang-format v16 easily? w/o installing it in your host system? I imagine we're going to have some PRs coming up now which are wrong formatted, since v16 isn't that common yet.

@jubalh
Copy link
Member Author

jubalh commented Apr 15, 2023

Is there a way to run clang-format v16 easily? w/o installing it in your host system?

Running it without installing it? I don't get what that means :)

I imagine we're going to have some PRs coming up now which are wrong formatted, since v16 isn't that common yet.

The whole point of the coding style configuration file is to define against what format clang-format should check against. But it seems that for some of the things they don't have definitions in there it leads to this mess...

I think there is not much that we can do here. People could run clang-format in a container but sure it's a hassle just to get formatting right.

They should just introduce config options for those parts that changed now AFAIK.

@sjaeckel
Copy link
Member

Is there a way to run clang-format v16 easily? w/o installing it in your host system?

Running it without installing it? I don't get what that means :)

This second questions should've been "Maybe even w/o installing..."

Running clang-format v16 is more important than having a full clang v16 installation. A container, a fat binary, something... clang v16 isn't readily available in a lot of distros right now, so I'll expect several PR's failing CI, simply because it's impossible for the contributor to easily get a clang-format v16.

They should just introduce config options for those parts that changed now AFAIK.

Yeah, that's the long term solution and the best possible outcome, but when will it happen if it even happens?

I also would want this change to happen, but I think it's not nice towards our contributor base.

@jubalh
Copy link
Member Author

jubalh commented Apr 15, 2023

Yeah, that's the long term solution and the best possible outcome, but when will it happen if it even happens?

They don't plan on doing this at all AFAIK. Their only comment was: we never promised to be backwards compatible.

@jubalh
Copy link
Member Author

jubalh commented Apr 15, 2023

I also would want this change to happen, but I think it's not nice towards our contributor base.

I actually don't see much of a problem tbh.
We have our clang-format config file where we define the style. All the style should be defined there. The only thing that the upstream guys are missing is that pointer thing on that struct. So far it seems this is only present in one file in colors.c.
So if people run clang-format like described in CONTRIBUTING.md then it will only format the files they actually edited.
And obviously it can be that color.c is among them and then some extra work needs to be done.

But the case that: a new contributor arrives + runs a system that only offers old clang + edits color.c is not the most likely situation. And even then they could see the CI check and edit this one case by hand.

I think this change improves more than it hurts us. And anyways I fail to see another solution since if we don't use this way we will just have more problems as it appears that so far actually each contributor (in the last 2 weeks 2 or 3 people) complained about old clang in the CI. So exactly the other way around.

jubalh added a commit that referenced this pull request Apr 15, 2023
Make this clear to new users. Since sjaeckel had reservations
on #1828.
@sjaeckel
Copy link
Member

I think this change improves more than it hurts us. And anyways I fail to see another solution since if we don't use this way we will just have more problems as it appears that so far actually each contributor (in the last 2 weeks 2 or 3 people) complained about old clang in the CI. So exactly the other way around.

The only thing I want to say is: I think we should choose a sensible version of tools to depend on.

clang v16 isn't available yet in most (and the most popular) linux distributions, therefore it's no sensible choice.

@jubalh
Copy link
Member Author

jubalh commented Apr 15, 2023

therefore it's no sensible choice.

Yet in 2 weeks 2 out of 2 new contributors complained that we run clang < 16 on the CI.
I'm not sure how to do it better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants