Re: [PHP-DEV] automatic formatting checks for pull requests?
On Sun, Feb 18, 2024 at 4:11 PM Gina P. Banyard wrote: > > On Saturday, 17 February 2024 at 22:18, Ilija Tovilo > wrote: > > > * 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. > > Surely the best way is to apply the formatting tool on all branches that are > supported (even in security support). > Have them be merged upwards, and then add the revisions of the commits to a > .git-blame-ignore-revs file so that git blame doesn't care about them. > > This should resolve the issue of making future merges difficult. Presumably, this would lead to merge conflicts in every open pull request. Maybe a resolution strategy could be automated, not ideal nonetheless. Additionally, given that the PR has discovered a clang-format bug that changes behavior (https://github.com/php/php-src/pull/13417#issuecomment-1950920114), I'd be wary of applying the formatting blindly to our stable branches.
Re: [PHP-DEV] automatic formatting checks for pull requests?
On Saturday, 17 February 2024 at 22:18, Ilija Tovilo wrote: > 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, > preferably even on-save in my editor. > * 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. Surely the best way is to apply the formatting tool on all branches that are supported (even in security support). Have them be merged upwards, and then add the revisions of the commits to a .git-blame-ignore-revs file so that git blame doesn't care about them. This should resolve the issue of making future merges difficult. Best regards, Gina P. Banyard
Re: [PHP-DEV] automatic formatting checks for pull requests?
i have tested running clang-format against the entire php-src codebase, and there is only 1 file it breaks: ext/spl/spl_directory_arginfo.h more details at https://github.com/php/php-src/pull/13417#issuecomment-1950920114 On Sun, 18 Feb 2024 at 01:45, Derick Rethans wrote: > > On 17 February 2024 22:18:05 GMT, Ilija Tovilo wrote: > >Hi Hans > > > >On Sat, Feb 17, 2024 at 3:31 PM Gina P. Banyard wrote: > >> > >> On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan > >> 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, > >preferably even on-save in my editor. > >* 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. > >* 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. > > > >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 > > IMO, clang-format isn't really suitable. Its untunable style is often far > from the coding style that we currently have, and it makes some really odd > choices as to when and how to wrap lines, making code definitely less > readable. > > cheers > Derick > IMO, clang-format isn't really suitable. Its untunable style is often far > from the coding style that we currently have, and it makes some really odd > choices as to when and how to wrap lines, making code definitely less > readable. it is far from "untunable", the manpage for tuning is huge: https://clang.llvm.org/docs/ClangFormatStyleOptions.html
Re: [PHP-DEV] automatic formatting checks for pull requests?
On Sun, 18 Feb 2024 at 00:51, Ilija Tovilo wrote:
>
> Hi Hans
>
> On Sat, Feb 17, 2024 at 3:31 PM Gina P. Banyard wrote:
> >
> > On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan
> > 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
-int main(){
- printf("Hello, World!\n");return 0;
+int main()
+{
+ printf("Hello, World!\n");
+ return 0;
}
\ No newline at end of file
+ echo 'code
Re: [PHP-DEV] automatic formatting checks for pull requests?
On 17 February 2024 22:18:05 GMT, Ilija Tovilo wrote: >Hi Hans > >On Sat, Feb 17, 2024 at 3:31 PM Gina P. Banyard wrote: >> >> On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan >> 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, >preferably even on-save in my editor. >* 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. >* 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. > >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 IMO, clang-format isn't really suitable. Its untunable style is often far from the coding style that we currently have, and it makes some really odd choices as to when and how to wrap lines, making code definitely less readable. cheers Derick
Re: [PHP-DEV] automatic formatting checks for pull requests?
Hi Hans On Sat, Feb 17, 2024 at 3:31 PM Gina P. Banyard wrote: > > On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan > 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, preferably even on-save in my editor. * 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. * 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. 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
Re: [PHP-DEV] automatic formatting checks for pull requests?
On Sat, Feb 17, 2024, 15:27 Gina P. Banyard 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 > > > > php-src use "tabs" instead of "spaces", that is... quite unusual, > > and I'm probably not the first person to accidentally use spaces > > instead of tabs, ref > > > https://github.com/php/php-src/pull/13401/files/d64a8ccdc1d21576827059ee86c0fa073c95ffcc#r1492699756 > > That's quite standard in the C world AFAIK, yes it's not what is standard > for PHP written code but that's kinda irrelevant. > > 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. > > Best regards, > > Gina P. Banyard > It worked great with the macros I've tested so far: 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 Is there any specific macros I can test?
Re: [PHP-DEV] automatic formatting checks for pull requests?
On Saturday, 17 February 2024 at 11:24, Hans Henrik Bergan wrote: > Can we add automatic formatting checks for pull requests? > Made a PR: https://github.com/php/php-src/pull/13417 > > php-src use "tabs" instead of "spaces", that is... quite unusual, > and I'm probably not the first person to accidentally use spaces > instead of tabs, ref > https://github.com/php/php-src/pull/13401/files/d64a8ccdc1d21576827059ee86c0fa073c95ffcc#r1492699756 That's quite standard in the C world AFAIK, yes it's not what is standard for PHP written code but that's kinda irrelevant. 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. Best regards, Gina P. Banyard
[PHP-DEV] automatic formatting checks for pull requests?
Can we add automatic formatting checks for pull requests? Made a PR: https://github.com/php/php-src/pull/13417 php-src use "tabs" instead of "spaces", that is... quite unusual, and I'm probably not the first person to accidentally use spaces instead of tabs, ref https://github.com/php/php-src/pull/13401/files/d64a8ccdc1d21576827059ee86c0fa073c95ffcc#r1492699756
