On Fri, Jan 20, 2023 at 12:58 PM Max Kellermann <max+...@blarg.de> wrote:
> On 2023/01/20 13:41, Jakub Zelenka <bu...@php.net> wrote: > > I think you should clarify in the RFC how this is going to be split and > > what's the time frame for getting that all in. > > The RFC is not the actual cleanup, it's for reaching an agreement on > whether it is a desirable goal to have minimal includes instead of the > chaotic mess we currently have. This was negated by Dmitry Stogov and > Derick Rethans (but by nobody else so far; the discussion here was > mostly about other aspects, but not about this basic questions). > > After this disagreement among PHP maintainers, Christoph M. Becker > suggested doing an RFC: > > https://github.com/php/php-src/pull/10220#issuecomment-1383789100 > > The splitting and time frame are out of the RFC's scope. I'll do > whatever reviewers ask me to do, that's why I splitted my first PRs > (after being asked to do so). > > I'm aware of all of this. What I'm trying to do is to help you to find a middle ground so the changes can be eventually accepted in some way. I think the main problem with this is the maintenance and stability impact and if you don't find answers for that, it will most likely not get accepted judging the mood of core devs about this. > > Your commits are separated by files and are really too small. I didn't > mean > > that. What I was talking about it is to do that single change and apply > all > > dependent changes on it so it still compiles / works and can be proposed > as > > a separate chunk of work. It means if you remove "zend_gc.h" from > "zend.h", > > then your PR would also contain changes that add "zend_gc.h" to > "zend_gc.c" > > and "zend_objects_API.h". So to get "zend.h" to this state in you PR, it > > would require 9 PR's if I count correctly. There would be some other > > changes after that obviously but it should be nowhere near to 104... > > I did it the other way round, an approach which makes much more sense > IMHO: > > (For example) I created a stgit patch which cleans up zend.h to only > include headers that are needed by zend.h itself. > > That broke the build of various other libraries, for example > "zend_gc.c". For me, this was clearly a sign that "zend_gc.c" is > buggy; adding the missing "#include" here had actually nothing to do > with removing one from "zend.h". So I popped the "zend.h" patch from > the stack, created a new patch to fix up "zend_gc.c". > > Back to the "zend.h" patch, build again, and create more patches to > fix libraries that got broken. > > That way, each patch fixes the bugs in one library, a bug that was > invisible before because of the header bloat, but gets later revealed > by header fixups. > > Each per-library patch is simple to review: it adds #includes to the > ".c" file which were missing there; the diff shows which are added, > and a reviewer can verify that those headers are indeed needed there. > The same patch may remove unneeded #includes from the ".h" file, which > the reviewer can also easily verify. > > Each per-library patch is buildable. You can go back and forth, "git > checkout" each of my commits and you have a working PHP source tree, > with commits that are self-contained, are reviewable and make sense. > > Your approach is harder to review, because there is nothing a reviewer > can do to verify the correctness, other than building PHP. They > cannot see whether the list of added #includes is really complete, > because they see only those that got added, but cannot see those that > are still missing. > > Well the issue with your approach is that 104 PR's is too much to review and reviewing that in one go is not good either. So finding some better separation and a plan forward could again help you to get this accepted. Regards Jakub
