Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122421
Precedence: bulk
MIME-Version: 1.0
References: <CAK7R4ay5kE-PGkb2qn8xvjsY0nLfMGcGBTztSeAy_9=X_BwmZg@mail.gmail.com>
 <p2eUiHGGPvgqcEwgk9ISftxabO5Q5sxW6-Eoz-LrbRQBGAPorVnFPKgFaAMeLlfzVQ7Ikwr9vWDa7ctd8t9q_2spBvb2kLW-SyW869R2zQA=@gpb.moe>
 <CAPyj-LA8eDWBGqYcw7S0DTRZXN1=6HFOncwgki-ny7xYA9FnVw@mail.gmail.com>
In-Reply-To: <CAPyj-LA8eDWBGqYcw7S0DTRZXN1=6HFOncwgki-ny7xYA9FnVw@mail.gmail.com>
Date: Sun, 18 Feb 2024 01:41:02 +0100
Message-ID: <CAJmy8YGwskmKQ1DPyEYU5PVJjpHiyGMpRuu9KKf1Z1izdWySLw@mail.gmail.com>
Subject: Re: [PHP-DEV] automatic formatting checks for pull requests?
To: Ilija Tovilo <tovilo.ilija@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: divinity76@gmail.com (Hans Henrik Bergan)

On Sun, 18 Feb 2024 at 00:51, Ilija Tovilo <tovilo.ilija@gmail.com> wrote:
>
> Hi Hans
>
> On Sat, Feb 17, 2024 at 3:31=E2=80=AFPM Gina P. Banyard <internals@gpb.mo=
e> wrote:
> >
> > On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan <hans@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 codebas=
e as it is somewhat the wild west,
> > but as far as my understanding goes is that Clang format struggles to u=
nderstand our codebase (namely macros) and is difficult to set-up for php-s=
rc.
>
> 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-dif=
f)
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 suggesti=
ons,
-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 futu=
re,
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 wo=
rth.
>
> Ilija

Here's how it works, roughly:
for pull requests:
- creates a new dedicated CI job specifically to check for formatting error=
s
- 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 ha=
ppy,
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 error=
s
- 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=3Dhttps://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