On Sun, 18 Feb 2024 at 00:51, Ilija Tovilo <tovilo.il...@gmail.com> wrote: > > Hi Hans > > On Sat, Feb 17, 2024 at 3:31 PM Gina P. Banyard <intern...@gpb.moe> wrote: > > > > On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan > > <h...@loltek.net> wrote: > > > > > Can we add automatic formatting checks for pull requests? > > > Made a PR: https://github.com/php/php-src/pull/13417 > > > > It would be nice to have some formatting rules to harmonize the codebase as > > it is somewhat the wild west, > > but as far as my understanding goes is that Clang format struggles to > > understand our codebase (namely macros) and is difficult to set-up for > > php-src. > > Right. Consistent code style is nice, but what we have now is really > not that bad. There are a couple things I'd want if we enforce code > style: > > * Fixing the style should be easy, running a single command without > first pushing to CI. > * It should be fast too, so that I can easily run it for every commit,
Yeah.. Basically clang-format-diff (not clang-format, but clang-format-diff) is difficult to work with, the easiest approach would be to make a full copy of the current working dir, and in that copy instruct git to reset to master and apply the diff between master and the parent working dir, and then have clang-format-diff analyze it and make its formatting suggestions, -but- that will not be fast, that will be slow. Haven't found a fast way to get clang-format-diff to work with a *dirty* workingdir against master, -yet- at least. Feel free to investigate, I'm pretty sure it can be done, but don't know how to do it. > preferably even on-save in my editor. If we can make it fast, we could make some opt-in git-commit-hook to format it (but we need to figure out the dirty-working-dir-speed thing first) > * The new code style should be applied only to newly added sections or > changed code, not entire files. Otherwise, we'll have many changes in > large files, with endless merge conflicts when merging up from lower > branches. Yeah, that's already in place :) > * The formatting tool should work for all php-src code, not just plain > C code. We don't want to be forced to refactor old macros just because > we need to add a single line to some long-standing code. Last time I > tried clang-format, it utterly failed with our macros. Seems to work with macros when I tested it :) Tested it with the following macros as of writing: ZEND_PARSE_PARAMETERS_START Z_PARAM_NUMBER ZEND_PARSE_PARAMETERS_END Z_TYPE_P Z_LVAL_P UNEXPECTED ZEND_ASSERT RETURN_DOUBLE RETURN_LONG RETURN_THROWS While .php and .phpt and whatever formatting can be implemented in the future, I would prefer baby steps, once we have *.[ch] accepted, improving it with .php/whatever should be easier in the future. > > I haven't looked at your PR in detail, so I'm not sure which of these > points it satisfies. It would be great if you could quickly describe > how it works, and what the goals are. > > Essentially, I'm just sceptical that this isn't more trouble than it's worth. > > Ilija Here's how it works, roughly: for pull requests: - creates a new dedicated CI job specifically to check for formatting errors - If formatting errors are detected, the CI fails, with the last message being a command to fix the styling issue, it looks like, and I quote: ``` please run curl https://termbin.com/zdzn | git apply -v ``` running that command, followed by git commit+git push should make the CI happy, git apply will not stage/commit/push automatically however, so in practice people will have to manually do: curl https://termbin.com/zdzn | git apply -v; (optionally run a `git diff` to just check that it looks right) git commit -a --message 'formatting'; git push; - we could add the last 2 steps to the actual error message, to make all 3 steps a copypasta.. would not be opposed. More technically, it - creates a new dedicated CI job specifically to check for formatting errors - Ubuntu 22.04, clang-format-diff, php-cli, git, netcat - creates a diff between current PR code and master branch - does *a bunch of tricks/hacks to get the environment suitable for clang-format-diff* including switching to master, applying the diff to master, commiting the diff under a fake username/email (because CI git doesn't have a username/email by default and git refuse to commit without them) - actually runs clang-format-diff against the last commit to format all the code in the last commit - generate a diff against the changes clang-format-diff just made - revert the last "fake username/email commit" - if that clang-format-diff is empty, it just prints "code formatting check succeeded :)" and the CI finishes successfully - if it's not empty, it prints, and I quote: ``` + cat ../formatted.diff diff --git a/test.c b/test.c index 6df39be994..64cfab717d 100644 --- a/test.c +++ b/test.c @@ -1,5 +1,7 @@ #include <stdio.h> -int main(){ - printf("Hello, World!\n");return 0; +int main() +{ + printf("Hello, World!\n"); + return 0; } \ No newline at end of file + echo 'code formatting check failed :(' code formatting check failed :( ++ cat ../formatted.diff ++ nc termbin.com 9999 /home/runner/work/_temp/3131a261-eaca-4987-9f8c-1b12e4645d76.sh: line 20: warning: command substitution: ignored null byte in input + patch_url=https://termbin.com/ul0f Please run + echo 'Please run' + echo 'curl https://termbin.com/ul0f | git apply -v' + exit 1 curl https://termbin.com/ul0f | git apply -v Error: Process completed with exit code 1. ``` and the CI run fails. I think there is room for improvement with regards to all the hacks/tricks currently done to get the environment suitable for clang-format-diff, but I also think it's already good enough for a CI (but not good enough to comfortably run locally) FWIW many real PHP projects already have something very similar called StyleCI today, where StyleCI will give a link to a patch to fix styling issues after pushing to a pull request, for example chrome-php/chrome ( https://github.com/chrome-php/chrome/ ) use it: https://styleci.io
