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

Add checking the code formatting with astyle during CI builds #1879

Merged
merged 2 commits into from
May 26, 2021

Conversation

aquesnel
Copy link
Contributor

@aquesnel aquesnel commented May 8, 2021

Add checking the code formatting with astyle during CI builds

Example build with the code formatting failed: https://github.com/aquesnel/xrdp/runs/2535294459
Example build with the code formatting succeeded: https://github.com/neutrinolabs/xrdp/pull/1879/checks?check_run_id=2535309754

Note: the i386 builds are likely failing because of a change to the github actions runner images: actions/runner-images#3339 That issue says that it should be resolved next week but the github actions team.

@matt335672
Copy link
Member

Well this looks pretty reasonable to me and all looks fine. The version of astyle is controlled, so we won't be blind-sided by changes in the way astyle functions. Also, just after a release (i.e. now) is a really good time to merge this.

The one fairly big concern I'd have is this recent announcement, which I've just found:-

https://sourceforge.net/p/astyle/bugs/548/

There are alternatives to astyle, with the obvious being clang-format. That's not however a self-contained option in the same way. I haven't tried it yet myself, but there appears to be a bit of impetus behind it - it's used by the linux kernel for example.

Given the existence of clang-format, I think it unlikely that anyone else will pick up the maintenance of astyle.

Personally I'm happy to merge this as-is despite the above, as it give us definite benefits despite the odd formatting oddity. If for some reason it stops working we're no worse off anyway. I think this needs a bit of discussion, and then @metalefty will need to make a decision.

@Nexarian
Copy link
Contributor

I'm going to wait until this is merged to merge my change here: #1895

I'd prefer that we use clang-format if astyle is being deprecated. We really don't want to bet the farm on things that aren't supported.

However, regardless, I have a recommendation on how to make sweeping changes like this in the future. Check in the linter and make it work for one file, and then blacklist all other files. Then slowly, one by one, submit smaller PRs that un-black-list each file.


code_formatting_check:
name: code formatting check
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

This might be unstable because the "latest" version will change as time flows but not a big problem.

Copy link
Member

@metalefty metalefty left a comment

Choose a reason for hiding this comment

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

It is a pretty good idea and LGTM. Let's merge it. We can switch to clang-format later.

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

Successfully merging this pull request may close these issues.

4 participants