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

Introduce clang-format as part of CI using Github Actions #2017

Closed
wants to merge 18 commits into from

Conversation

Jakuje
Copy link
Member

@Jakuje Jakuje commented Apr 24, 2020

Testing of #1641

@frankmorgner
Copy link
Member

The build fails with missing api_token. I believe I've created GH_TOKEN with the access rights for pushing to OpenSC/Nightly only. Maybe you need to create an other one for pushing to master

@Jakuje
Copy link
Member Author

Jakuje commented Apr 29, 2020

Thanks for pointer. I am testing the actions in my fork. Now, I generated my own GH_TOKEN in the fork's PR so lets see how it will work. I will probably push some testing code to be able to check how does it actually work with real code.

Jakuje#1

@Jakuje
Copy link
Member Author

Jakuje commented Apr 29, 2020

@frankmorgner the above is already tested with one of the commits from my Fuzz branch, successfully reporting some diffs through github actions (as I am not that precise with formatting ;) ). The travis does not work because there is probably some old version of clang in the osx builder (does not know AlignConsecutiveMacros). I tried with xcode10 but it failed to build altogether so I would probably not mind going only with the github actions

Copy link
Member

@frankmorgner frankmorgner left a comment

Choose a reason for hiding this comment

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

great, if the formatting matches the majority of the code base, we're good to go

@frankmorgner
Copy link
Member

I think we're good for merging, right?

@Jakuje
Copy link
Member Author

Jakuje commented May 11, 2020

Not yet. I wanted to review some of more obscure options of the code formatter, but did not get to do that yet. Please, keep this one open before we will finalize the format in #1641.

@frankmorgner
Copy link
Member

What's the status of this pull request?

@Jakuje
Copy link
Member Author

Jakuje commented Dec 14, 2020

What's the status of this pull request?

It is awfully outdated. But I did not get much feedback on my comments in #1641 during last year so I did not push that, especially when we were in need of release. It is now out so we can try to focus to this agian. I saw @dengert to test this, coming to some issues, but already using something in his PRs.

I will get it up to date and probably try the Github action version as the travis is getting awfully slow recently.

@popovec
Copy link
Member

popovec commented Jan 10, 2024

Apparently, this PR is not in a condition to be merged as it is, but I would ask if at least the single .clang-format file from this package could not be merged into the master. Thank you.

@Jakuje
Copy link
Member Author

Jakuje commented Jan 10, 2024

Apparently, this PR is not in a condition to be merged as it is, but I would ask if at least the single .clang-format file from this package could not be merged into the master. Thank you.

Yeah. I spent already too much time reformatting the code, while trying to understand some of the options and their behavior. I moved the non-problematic changes into separate PR #2977. Some less disruptive changes on top of them are in https://github.com/Jakuje/OpenSC/tree/clang-format for preview how it tries to format some larger code changes (the leftovers marked as clang format now are mostly the changes that I do not like or missed).

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.

None yet

5 participants