Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [VOTE] include cleanup »

[VOTE] include cleanup

php.internals

Max Kellermann

3 years ago
On 2023/01/30 11:26, Max Kellermann <max+php@blarg.de> wrote:
> If nobody objects, I'll announce the start of voting on February 1st.
That's today. Voting starts now, please vote on my RFC: https://wiki.php.net/rfc/include_cleanup Original discussion: https://news-web.php.net/php.internals/119272 [Reposting this to create a new thread; thanks Tim for the hint.]

Tim Düsterhus

3 years ago
Hi On 2/1/23 13:13, Max Kellermann wrote:
> On 2023/01/30 11:26, Max Kellermann <max+php@blarg.de> wrote: >> If nobody objects, I'll announce the start of voting on February 1st. > > That's today. > > Voting starts now, please vote on my RFC: > https://wiki.php.net/rfc/include_cleanup > > Original discussion: https://news-web.php.net/php.internals/119272 >
I just notice that the position in the overview page will also need to be updated: https://wiki.php.net/rfc Best regards Tim Düsterhus

Tim Düsterhus

3 years ago
Hi On 2/1/23 13:13, Max Kellermann wrote:
> On 2023/01/30 11:26, Max Kellermann <max+php@blarg.de> wrote: >> If nobody objects, I'll announce the start of voting on February 1st. > > That's today. > > Voting starts now, please vote on my RFC: > https://wiki.php.net/rfc/include_cleanup > > Original discussion: https://news-web.php.net/php.internals/119272 >
Something about the vote as set up has bothered me since voting started and I believe I just realized what it is: I feel like what's being voted on is ambiguous and depending on how the voter interprets it, the vote also violates the voting rules. This is specifically about the first vote titled "Should #include directives be cleaned up?": The vote as it is worded technically does not make a statement on the inverse: If the vote is declined, it *does not* mean that #include directives *may not* be cleaned up. Instead the status quo would be preserved. To my understanding the status quo is "depends on a case-by-case basis", because we do not currently have any guidelines regarding #include. However based on the discussion of the RFC I believe that voters may have assumed that a "No" means "A cleanup is not allowed", because several participants expressed an active aversion to a cleanup during the discussion. As for myself I've certainly had that understanding when casting my vote. This interpretation would be in violation of the voting process, because the status quo would be changed no matter the results of the RFC, but the two options would not be equal: Disallowing a clean-up would require 33% of votes, whereas allowing clean-up would require 66% of votes. The status quo "decide on a case by case basis" would no longer be legal even without a clear agreement. If the results of the RFC are going to be interpreted according to the second possible interpretation, then I believe that to be actively harmful: It would disallow removing #include directives entirely and more generally it would prevent any type of refactoring. I find it only natural to "clean up after myself" when moving stuff around. An example would be PHP 8.2's ext/random where several functions moved into a different extension. Any includes specific to those moved functions would need to stay where they were. As one of the persons listed as a maintainer of ext/random I was also thinking about splitting the 'php_random_uint128_t' implementation into a separate header file to keep php_random.h neat and tidy, because the 128 Bit integer operations are only used for the pcgoneseq128xslrr64 engine. Of course I would've coordinated that change with zeriyoshi as the other listed maintainer. Best regards Tim Düsterhus

Max Kellermann

3 years ago
On 2023/02/09 19:04, Tim Düsterhus <tim@bastelstu.be> wrote:
> However based on the discussion of the RFC I believe that voters may have > assumed that a "No" means "A cleanup is not allowed", because several > participants expressed an active aversion to a cleanup during the > discussion. As for myself I've certainly had that understanding when casting > my vote.
Voting "NO" means no change - and currently, cleanup is not allowed, which you can see from the fact that all of my code cleanups were either rejected or reverted.
> Disallowing a clean-up would require 33% of votes, whereas allowing > clean-up would require 66% of votes. The status quo "decide on a > case by case basis" would no longer be legal even without a clear > agreement.
It is indeed unfortunate that a supermajority is required for all primary votes, because in this case, requiring only a simple majority would be favorable IMO. It is not clear whether the current rule is "decide on a case by case basis"; it has been argued that my code cleanup shall be rejected/reverted because that would make merging branches harder. - If that alone is reason enough to reject/revert a code cleanup change, then this applies to all kinds of code cleanup, and no code cleanup is currently allowed. -> "case by case" doesn't count. - If that alone is NOT reason enough to reject/revert a code cleanup, then more reasons need to be brought forward to hold my code cleanups off. Max

Matthew Weier O'Phinney

3 years ago
On Thu, Feb 9, 2023 at 1:33 PM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/09 19:04, Tim Düsterhus <tim@bastelstu.be> wrote: > > However based on the discussion of the RFC I believe that voters may have > > assumed that a "No" means "A cleanup is not allowed", because several > > participants expressed an active aversion to a cleanup during the > > discussion. As for myself I've certainly had that understanding when > casting > > my vote. > > Voting "NO" means no change - and currently, cleanup is not allowed, > which you can see from the fact that all of my code cleanups were > either rejected or reverted. >
That's a poor interpretation of what happened. As Tim alluded, the current status quo is that cleanup is allowed on a case-by-case basis. The particular cases resulting from your PRs were rejected, but this doesn't mean all cases will be. I'm not directly involved in maintenance, but my take on the scenario was that these were rejected and reverted because they caused breakage, whether that was in compiling a spare PHP build, or in extensions that were assuming that using certain headers would slurp in everything they needed. This breakage was unacceptable without an RFC. I saw chatter from a number of folks after the changes were merged about builds no longer compiling; considering the stability of the php-src tree, inability to build will always be a source of alarm. What needs clarification in the RFC you've presented is that a "No" vote means "no change to current processes". Personally, I'd halt the current vote, make the change, and re-start the vote at this point to ensure everyone voting is clear on that point.
> > Disallowing a clean-up would require 33% of votes, whereas allowing > > clean-up would require 66% of votes. The status quo "decide on a > > case by case basis" would no longer be legal even without a clear > > agreement. > > It is indeed unfortunate that a supermajority is required for all > primary votes, because in this case, requiring only a simple majority > would be favorable IMO. >
A supermajority is required on any change that would lead to backwards incompatibility for either end-users or extension writers. Your proposal is something that would do the latter. Sure a simple majority is ALWAYS more favorable, but the hurdle exists to ensure that everyone pay attention to the BC implications when they vote.
> It is not clear whether the current rule is "decide on a case by case > basis"; it has been argued that my code cleanup shall be > rejected/reverted because that would make merging branches harder. > > - If that alone is reason enough to reject/revert a code cleanup > change, then this applies to all kinds of code cleanup, and no code > cleanup is currently allowed. -> "case by case" doesn't count. > > - If that alone is NOT reason enough to reject/revert a code cleanup, > then more reasons need to be brought forward to hold my code > cleanups off. >
As I pointed out earlier, the changes previously merged led to breakages when compiling the project. How is that not enough? And dumping a huge bunch of PRs with such changes without first discussing it with maintainers means a lot of effort reviewing — why are your proposed changes more important than any of the other work the various maintainers are doing? This is why they asked for an RFC; something of this magnitude needs discussion, because it impacts everybody already touching the project, the people most familiar with it. The other point that has been brought up multiple times is that it introduces breaking changes for extension maintainers. Should these extensions be relying on one or more "god" headers instead of the specific headers for the symbols they use? Probably not. Will forcing the issue without giving them a chance to review and understand the changes, and have a roadmap for when and how those changes occur be a net positive? No; it will cause a lot of busy work for a lot of people, almost all of whom are volunteers and most of whom would rather be building out user-requested features or fixing user-reported bugs. I'm unsure why that's unclear or not enough for you.
-- Matthew Weier O'Phinney mweierophinney@gmail.com https://mwop.net/ he/him

Max Kellermann

3 years ago
On 2023/02/09 23:09, Matthew Weier O'Phinney <mweierophinney@gmail.com> wrote:
> I'm not directly involved in maintenance, but my take on the scenario was > that these were rejected and reverted because they caused breakage
Your take is not quite correct. No PR was rejected due to breakage. There was exactly one (internal) breakage that occurred after merging, the DTrace build failure; it was a rather stupid mistake, but immediately after I learned about it, I submitted a trivial fix for it. Dmitry's demand for revert due to DTrace build failure: https://github.com/php/php-src/pull/10220#issuecomment-1383658247 My fix PR: https://github.com/php/php-src/pull/10334 Dmitry claimed it doesn't fix the build: https://github.com/php/php-src/pull/10220#issuecomment-1383706602 I asked him to explain why he thinks it didn't fix the build: https://github.com/php/php-src/pull/10220#issuecomment-1383714708 Dmitry refused to explain: https://github.com/php/php-src/pull/10220#issuecomment-1383739816 Reporter confirms my PR does indeed fix the build (proving Dmitry wrong): https://github.com/php/php-src/pull/10220#issuecomment-1383802334 Instead of applying my fix, everything was reverted. Does that sound reasonable to anybody? Do you always revert stuff when a merge breaks something instead of fixing the actual bug? If yes, why was https://github.com/php/php-src/commit/a21195650e53e3426680 not reverted after it caused a build failure AND a runtime crash?
> This breakage was unacceptable without an RFC. I saw chatter from a number > of folks after the changes were merged about builds no longer compiling;
I never heard of that. Can you give me a link to that chatter, please? If there was a problem with my work, it's important for me to learn about it.
> A supermajority is required on any change that would lead to backwards > incompatibility for either end-users or extension writers. Your proposal is > something that would do the latter.
No, it doesn't lead to backwards incompatibility. Did you see this sentence in my RFC? "If staying compatible with defective extensions is deemed important, these includes may be re-added to a header such as “php_compat.h”, possibly with a way to opt-out. That way, broken extensions still build, but PHP itself still benefits from a smaller and more correct set of #includes." And check the latest PR linked in my RFC: https://github.com/php/php-src/pull/10410 "One new commit adds compatibility #includes to main/php.h, because people on php-internals have suggested that source compatibility with third-party extensions should be retained, even if those extensions are buggy (e.g. forgetting to include errno.h even though they use errno)." There will be no backwards incompatibility, not even with broken third-party extensions. And if, by mistake, a backwards incompatibility gets reported, I'll take care to fix it. Did you see that my proposal to include third-party extensions in the nightly CI build got merged? https://github.com/php/php-src/pull/10404 This PR aims at ensuring best compatibility with third-party extensions, to be able to notice unintentional breakages as early as possible. What I'm trying to do is make PHP more reliable/stable/compatible than before.
> As I pointed out earlier, the changes previously merged led to breakages > when compiling the project. How is that not enough?
And as I pointed out earlier, I'd like to learn about those breakages. I hadn't heard of any. Please give me links so I can adjust my draft PRs to fix all known problems.
> And dumping a huge bunch of PRs with such changes without first > discussing it with maintainers means a lot of effort reviewing
That's not what happened. I did discuss these changes (on GitHub, not on this mailing list) tbefore I submitted the first cleanup PRs, and got very positive feedback. And my PRs got merged quickly, because those maintainers saw the obvious value in my cleanups.
> — why are your proposed changes more important than any of the other > work the various maintainers are doing?
I didn't say anything about my proposed changes being more important than something else. Such a comparison seems meaningless to me. Why bring it up?
> This is why they asked for an RFC; something of this magnitude needs > discussion, because it impacts everybody already touching the > project, the people most familiar with it.
After I posted on this mailing list, there was a lot of negative feedback on secondary questions (include comments, forward declarations). But the feedback on primary question, i.e. whether to clean up includes, was nearly 100% positive. The only counter-argument (against the cleanup) was that it would make merging branches harder.
> The other point that has been brought up multiple times is that it > introduces breaking changes for extension maintainers.
It does not.
> Should these extensions be relying on one or more "god" headers instead of > the specific headers for the symbols they use? Probably not.
I ... don't understand. What headers do extension authors rely on currently? Is that documented somewhere? What will, in your understanding, change about that after my cleanup?
> I'm unsure why that's unclear or not enough for you.
I tried my best to explain what's unclear. Please clarify by answering my questions. Max

Girgias

3 years ago
On Thu, 9 Feb 2023 at 22:09, Matthew Weier O'Phinney < mweierophinney@gmail.com> wrote:
> I'm not directly involved in maintenance, but my take on the scenario was > that these were rejected and reverted because they caused breakage, whether > that was in compiling a spare PHP build, or in extensions that were > assuming that using certain headers would slurp in everything they needed. > This breakage was unacceptable without an RFC. I saw chatter from a number > of folks after the changes were merged about builds no longer compiling; > considering the stability of the php-src tree, inability to build will > always be a source of alarm. > > [...] > > As I pointed out earlier, the changes previously merged led to breakages > when compiling the project. How is that not enough? And dumping a huge > bunch of PRs with such changes without first discussing it with maintainers > means a lot of effort reviewing [...]. >
The other point that has been brought up multiple times is that it
> introduces breaking changes for extension maintainers. > > Should these extensions be relying on one or more "god" headers instead of > the specific headers for the symbols they use? Probably not. Will forcing > the issue without giving them a chance to review and understand the > changes, and have a roadmap for when and how those changes occur be a net > positive? No; it will cause a lot of busy work for a lot of people, almost > all of whom are volunteers and most of whom would rather be building out > user-requested features or fixing user-reported bugs. > > I'm unsure why that's unclear or not enough for you. > > -- > Matthew Weier O'Phinney > mweierophinney@gmail.com > https://mwop.net/ > he/him >
I'm going to ignore Max's subpar behaviour and frustration in the aftermath of this. However, as the reviewer of these PRs, they did NOT completely break the build, I was working on php-src and compiling master without ANY issues during the weekend after landing the changes. The *only* build being broken was the DTrace build, which would have been fixed by a follow-up PR. We have had completely broken builds for longer days due to some other random changes, and we didn't revert them but fixed them as a follow-up. We still, for over 6 months now, have a "broken" ASAN build due to phpdbg messing up the analyser and crashing the test runner on 8.2 and master, something that multiple core devs, me included, need to work around by monkey patching the run-test.php file. These changes were initially agreed upon and multiple people were in favour, even by Dmitry. I will also say, that I spend a significant amount of time reviewing those PRs, and I would have continued, as I think they are beneficial to the project. But my opinion and the work I put into the reviews is apparently worthless. Therefore, I maintain that, IMHO, these commits should not have been reverted. The unique complaint, that should be addressed to me, is in how I merged the PRs in that I didn't squash them. The fact external extensions were broken and that we should add all relevant headers to php.h is a fair complaint and would have been fixed quite easily in a single commit and did not affect anyone effectively at this stage because we are, checks notes, 6 months at minimum before the first alpha builds of PHP 8.3 are released. The only reason they were reverted is that Dmitry demanded them as they broke the DTrace build, which he is apparently the only one to use and is not tested on CI, a failure of the project's CI infrastructure. As I previously complained, the RFC process for this kind of change is completely inadequate and demonstrates that the PHP project has a massive issue with governance and how to handle cases like this. As a final note, if the complaint had been made by anyone else other than Dmitry, I doubt these changes would have been reverted, and can we please stop pretending otherwise. Sincerely, George P. Banyard

Max Kellermann

3 years ago
On 2023/02/13 01:58, "G. P. B." <george.banyard@gmail.com> wrote:
> We have had completely broken builds for longer days due to some other > random changes, and we didn't revert them but fixed them as a follow-up. > We still, for over 6 months now, have a "broken" ASAN build due to phpdbg > messing up the analyser and crashing the test runner on 8.2 and master, > something that multiple core devs, me included, need to work around by > monkey patching the run-test.php file.
I had a feeling there are double standards at play. The way my work was dealt with is unprecedented! In the git history, I could not find any other set of PRs that was reverted completely just for a minor one-line issue. Stuff breaks all the time, and every breakage is, of course, a mistake that should have been handled with more care, and something to learn from. Sometimes, a revert is the right solution, but in my case, the (demand of a) revert was unreasonable and hasty.
> As a final note, if the complaint had been made by anyone else other than > Dmitry, I doubt these changes would have been reverted, and can we please > stop pretending otherwise.
I forget one include, break an exotic build in master branch, Dmitry demands complete revert of 4 PRs / 61 commits (60 of which are unrelated to the breakage). Dmitry breaks the whole build for everybody (including the CI) and introduces a crash bug, merged through all three branches (https://github.com/php/php-src/commit/a21195650e53), no revert. (Don't misunderstand, everybody makes mistakes, I just point out the obvious double standard.) Then everybody is raving about my include comments and forward declarations, yet the PHP code base has many of these. Turns out those who complained the loudest have authored some of these in the past. Max

Dmitry Stogov

3 years ago
On Mon, Feb 13, 2023 at 11:35 AM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/13 01:58, "G. P. B." <george.banyard@gmail.com> wrote: > > We have had completely broken builds for longer days due to some other > > random changes, and we didn't revert them but fixed them as a follow-up. > > We still, for over 6 months now, have a "broken" ASAN build due to phpdbg > > messing up the analyser and crashing the test runner on 8.2 and master, > > something that multiple core devs, me included, need to work around by > > monkey patching the run-test.php file. > > I had a feeling there are double standards at play. The way my work > was dealt with is unprecedented! > > In the git history, I could not find any other set of PRs that was > reverted completely just for a minor one-line issue. > > > Stuff breaks all the time, and every breakage is, of course, a mistake > that should have been handled with more care, and something to learn > from. Sometimes, a revert is the right solution, but in my case, the > (demand of a) revert was unreasonable and hasty. > > > > As a final note, if the complaint had been made by anyone else other than > > Dmitry, I doubt these changes would have been reverted, and can we please > > stop pretending otherwise. > > I forget one include, break an exotic build in master branch, Dmitry > demands complete revert of 4 PRs / 61 commits (60 of which are > unrelated to the breakage). > > Dmitry breaks the whole build for everybody (including the CI) and > introduces a crash bug, merged through all three branches > (https://github.com/php/php-src/commit/a21195650e53), no revert. > > (Don't misunderstand, everybody makes mistakes, I just point out the > obvious double standard.) > > Then everybody is raving about my include comments and forward > declarations, yet the PHP code base has many of these. Turns out > those who complained the loudest have authored some of these in the > past. > > Max >
Max, It's OK when commits are reverted. You are working in a common repository, and if your commits become stoppers for others they have to be reverted. Some of my commits were reverted as well. Having too many dependent commits and inability to revert a single one became an additional trouble and drew more attention to things you are doing... I didn't care about a single header change, but I do care about 100 dependent commits. After all, this includes cleanup is really questionable, and the current vote result shows that is not my sole opinion. Personally, I think this work might be very welcome during PHP-7.0 development together with other re-factoring(s). Massive permutation changes in a minor release are not acceptable for me. Maybe it makes sense to target them to PHP-9.0 Thanks. Dmitry.

Max Kellermann

3 years ago
On 2023/02/13 10:28, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> It's OK when commits are reverted. > You are working in a common repository, and if your commits become stoppers > for others they have to be reverted. > Some of my commits were reverted as well.
That doesn't explain why you demanded to revert everything instead of applying my trivial single-line fix.
> Having too many dependent commits and inability to revert a single one > became an additional trouble and drew more attention to things you are > doing... > I didn't care about a single header change, but I do care about 100 > dependent commits.
I don't get it, what's your point here? The fact that these dependencies exist is a result of the unclean state of the PHP code base, something my work aims to improve. The number of dependencies after my PRs was lower than before, and that's good. The "inability to revert a single one" is not a problem that needed to be solved, because a revert was never necessary - there was a trivial fix (that did work, contrary to your assertion).
> After all, this includes cleanup is really questionable, and the current > vote result shows that is not my sole opinion. > Personally, I think this work might be very welcome during PHP-7.0 > development together with other re-factoring(s). > Massive permutation changes in a minor release are not acceptable for me. > Maybe it makes sense to target them to PHP-9.0
The vote is not about WHEN this cleanup can be done - it's about WHETHER at all. Voting "yes" does not imply that this must be done for 8.3. The RFC suggests that it could be 8.3 or 9.0, but that decision is not part of the vote. Voting "no" means you never want this cleanup to happen, ever, not for 8.3 and not for 9.0. So if you're not really opposed to such a kind of cleanup in general, don't vote "no". Max

Dmitry Stogov

3 years ago
On Mon, Feb 13, 2023 at 12:51 PM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/13 10:28, Dmitry Stogov <dmitrystogov@gmail.com> wrote: > > It's OK when commits are reverted. > > You are working in a common repository, and if your commits become > stoppers > > for others they have to be reverted. > > Some of my commits were reverted as well. > > That doesn't explain why you demanded to revert everything instead of > applying my trivial single-line fix. > > > Having too many dependent commits and inability to revert a single one > > became an additional trouble and drew more attention to things you are > > doing... > > I didn't care about a single header change, but I do care about 100 > > dependent commits. > > I don't get it, what's your point here? > > The fact that these dependencies exist is a result of the unclean > state of the PHP code base, something my work aims to improve. The > number of dependencies after my PRs was lower than before, and that's > good. > > The "inability to revert a single one" is not a problem that needed to > be solved, because a revert was never necessary - there was a trivial > fix (that did work, contrary to your assertion). > > > After all, this includes cleanup is really questionable, and the current > > vote result shows that is not my sole opinion. > > Personally, I think this work might be very welcome during PHP-7.0 > > development together with other re-factoring(s). > > Massive permutation changes in a minor release are not acceptable for me. > > Maybe it makes sense to target them to PHP-9.0 > > The vote is not about WHEN this cleanup can be done - it's about > WHETHER at all. > > Voting "yes" does not imply that this must be done for 8.3. The RFC > suggests that it could be 8.3 or 9.0, but that decision is not part of > the vote. > > Voting "no" means you never want this cleanup to happen, ever, not for > 8.3 and not for 9.0. > > So if you're not really opposed to such a kind of cleanup in general, > don't vote "no". >
The RFC proposes merging into the "master" branch. And I voted exactly against this. Thanks. Dmitry.

Arvids Godjuks

3 years ago
Good day dear Internals! I've been following this thread/RFC from its inception to the current moment. I have watched the situation deteriorate and at this point, I have major concerns about the events that transpired, the motivations of some decisions and the apparent major lack of communication. But the worst thing, in my opinion, is that the mistakes that were made about a decade ago are happening in this thread: Cubstomping and alienating new contributors who are willing to take on major amounts of work and are willing to take on the long-term maintenance. I'm not alone in these concerns, but those people can do their own messaging. What I do not understand the most is the apparent "Not in my backyard" attitude to this RFC that seems to be happening. Improving the codebase ALWAYS requires breaking things and to be absolutely frank, I do not see how "merging conflicts" can be a sound reason for refusing header file shuffling. Or, in this case, "breaking extensions". I mean, the fixes are simple - update the header files. The author of the RFC is willing to provide the documentation/comments so it is easy to do. PHP 5 => PHP 7 engine transition was a major multi-year endeavour that was multiple orders of magnitude harder and broader problem, but everyone stuck to it and it was done. This RFC is nowhere even close to that but offers long-term improvements to the project. I am concerned about the apparent unwillingness to take on code cleanup projects. Can everybody involved cool their heads for a week, come back and talk this one out and make a plan for how to make the improvements in a way that parties can agree it should be handled?
-- Arvīds Godjuks +371 26 851 664 arvids.godjuks@gmail.com Telegram: @psihius https://t.me/psihius

Max Kellermann

3 years ago
On 2023/02/13 11:05, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> The RFC proposes merging into the "master" branch. > And I voted exactly against this.
If not "master", what branch would you prefer? That's a dumb question, of course, because "master" is where all future versions branch off, don't they? Saying "no" to master means saying "no" forever. At least "no" to 8.3 and 9.0, because these are the versions explicitly mentioned in my RFC (with a question mark because I didn't know much about PHP's release management). Max

Peter Kokot

3 years ago
On Wed, 1 Feb 2023 at 13:14, Max Kellermann <max+php@blarg.de> wrote:
> > On 2023/01/30 11:26, Max Kellermann <max+php@blarg.de> wrote: > > If nobody objects, I'll announce the start of voting on February 1st. > > That's today. > > Voting starts now, please vote on my RFC: > https://wiki.php.net/rfc/include_cleanup > > Original discussion: https://news-web.php.net/php.internals/119272 > > [Reposting this to create a new thread; thanks Tim for the hint.] > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php
Hello, I've voted in favor of the RFC because of the code-cleaning, tech-debt-reducing improvements to code readability. Additionally, PHP-SRC should adopt some suitable Clang-format rules and gradually integrate them into the code. BTW, merging from PHP 8.1 up is not problematic. Git diff only looks at a few lines of code above and below. Not the top of the file.

Max Kellermann

3 years ago
On 2023/02/11 17:14, Peter Kokot <petk@php.net> wrote:
> I've voted in favor of the RFC because of the code-cleaning, > tech-debt-reducing improvements to code readability.
Exactly my point, and I'm surprised by the resistance. Not only surprised, but also disappointed that many have voted against code cleanup, but where have those people been when this was being discussed? Matthew said there had been "chatter from a number of folks after the changes were merged about builds no longer compiling", but was not able to render that more precisely. None of that was discussed on GitHub nor here on php-internals. I have to question whether these build breakages even exist. (Other than the DTrace build failure which happened because one line was missing, but that's a fact and not "chatter", and one bug reporter and not "a number of folks". Let's put this dead horse to rest.)
> BTW, merging from PHP 8.1 up is not problematic. Git diff only looks > at a few lines of code above and below. Not the top of the file.
This was the only counter-argument ever discussed here, and I'm puzzled that the imagination of merge conflicts scares so many people. About a kind of change that is unlikely to cause one. Any code change can cause a merge conflict, but include cleanups are the least likely cause of all, because include directives are almost never touched in bugfix-only branches. Is that all, or is there another, yet unnamed reason why there's so much resistance? The hearsay about build failures? There are 3 more days to vote, and it's a tie currently - means 9 "YES" votes missing for supermajority or else the RFC gets rejected. That rejection would not only be a missed chance to modernize the PHP code base, but also a sign to potential PHP contributors that the PHP maintainers don't value clean code. This is unsettling. Imagine how this will overshadow future attempts to remove historical cruft from a decades-old code base, because the counter-arguments apply the same to any kind of code cleanup. As any decades-old code base, there's a lot of historical cruft in PHP which gets in the way all the time, much more than a hypothetical one-time merge conflict. Historical cruft keeps piling up if you don't keep cutting it down all the time. Cleaner code is easier to read and understand, which makes existing bugs easier to fix and makes new bugs less likely to be added. That outweighs, in my opinion, all the possible disadvantages that the process of code cleanup could possibly have, by orders of magnitude. Max

Peter Kokot

3 years ago
On Sun, 12 Feb 2023 at 09:31, Max Kellermann <max+php@blarg.de> wrote:
> > On 2023/02/11 17:14, Peter Kokot <petk@php.net> wrote: > > I've voted in favor of the RFC because of the code-cleaning, > > tech-debt-reducing improvements to code readability. > > Exactly my point, and I'm surprised by the resistance. > > Not only surprised, but also disappointed that many have voted against > code cleanup, but where have those people been when this was being > discussed? > > Matthew said there had been "chatter from a number of folks after the > changes were merged about builds no longer compiling", but was not > able to render that more precisely. > > None of that was discussed on GitHub nor here on php-internals. I > have to question whether these build breakages even exist. > > (Other than the DTrace build failure which happened because one line > was missing, but that's a fact and not "chatter", and one bug reporter > and not "a number of folks". Let's put this dead horse to rest.) > > > > BTW, merging from PHP 8.1 up is not problematic. Git diff only looks > > at a few lines of code above and below. Not the top of the file. > > This was the only counter-argument ever discussed here, and I'm > puzzled that the imagination of merge conflicts scares so many people. > About a kind of change that is unlikely to cause one. > > Any code change can cause a merge conflict, but include cleanups are > the least likely cause of all, because include directives are almost > never touched in bugfix-only branches. > > > Is that all, or is there another, yet unnamed reason why there's so > much resistance? The hearsay about build failures? > > > There are 3 more days to vote, and it's a tie currently - means 9 > "YES" votes missing for supermajority or else the RFC gets rejected. > That rejection would not only be a missed chance to modernize the PHP > code base, but also a sign to potential PHP contributors that the PHP > maintainers don't value clean code. This is unsettling. > > Imagine how this will overshadow future attempts to remove historical > cruft from a decades-old code base, because the counter-arguments > apply the same to any kind of code cleanup. As any decades-old code > base, there's a lot of historical cruft in PHP which gets in the way > all the time, much more than a hypothetical one-time merge conflict. > Historical cruft keeps piling up if you don't keep cutting it down all > the time. > > Cleaner code is easier to read and understand, which makes existing > bugs easier to fix and makes new bugs less likely to be added. That > outweighs, in my opinion, all the possible disadvantages that the > process of code cleanup could possibly have, by orders of magnitude. > > Max
Well, the PHP long-term maintainers are just a bit stubborn when it comes to such changes and probably what concerns them is writing code styles in stone. We shouldn't conclude that the PHP team as a whole doesn't care about clean code based on this voting. People just need a bit of time to grasp the changes and discuss these things over a longer period. They just care more about the stability of the current repo and the extensions out there than the cosmetics of the ASCII characters in files. I agree though, that both sides need to be improved. One more question here. Is the iwyu (include-what-you-use) tool used here to clean up these header files? I'm assuming something like this could be one day executed: iwyu --no_fwd_decls --no-comments Based on this, this is an awesome tool and can improve the code. I'd say to go hand in hand here and one day later discuss things like this again to integrate these gradually. Automating is always tricky on the other hand. Like adding the iwyu step in the build checks.

Max Kellermann

3 years ago
On 2023/02/01 13:13, Max Kellermann <max+php@blarg.de> wrote:
> Voting starts now, please vote on my RFC: > https://wiki.php.net/rfc/include_cleanup
Hi, voting of https://wiki.php.net/rfc/include_cleanup has ended today at 15 UTC. The majority of voters (52%) voted "Yes" on the primary vote - "Should #include directives be cleaned up?" - but the required supermajority for a primary vote was not met. Therefore, the primary vote is declined. On the secondary vote "Is it allowed to document an #include line with a code comment?", 90% of all voters do not want to allow code comments on #include lines. To fix the PHP code base according to this decision, please consider merging https://github.com/php/php-src/pull/10472 The secondary vote "Is it allowed to forward-declare structs/unions/typedefs?" was clearly rejected as well; 87.5% of all voters thought forward declarations should not be allowed. There are numerous unnecessary forward declarations; several of these are removed by https://github.com/php/php-src/pull/10494 - please consider merging this PR for compliance with this decision. Interestingly, of all things, the most intrusive vote ("Is it allowed to split a large header to reduce dependencies?") got accepted by a supermajority. I'll assemble a PR with just the header splitting commits and submit it for merging. From my minimal #include cleanup PR (https://github.com/php/php-src/pull/10410), I have removed all include comments. The RFC failed to meet the supermajority, but I'm not sure if that means that #include cleanups are now (or still?) forbidden. Having a majority, but no supermajority sounds like it's inconclusive, but I don't know what that means and how to proceed. Max

Derick Rethans

3 years ago
On 15 February 2023 15:18:31 GMT, Max Kellermann <max+php@blarg.de> wrote:
>On 2023/02/01 13:13, Max Kellermann <max+php@blarg.de> wrote: >> Voting starts now, please vote on my RFC: >> https://wiki.php.net/rfc/include_cleanup > >Hi, > >voting of https://wiki.php.net/rfc/include_cleanup has ended today at >15 UTC. > >The majority of voters (52%) voted "Yes" on the primary vote - "Should >#include directives be cleaned up?" - but the required supermajority >for a primary vote was not met. Therefore, the primary vote is >declined.
(snip)
>Interestingly, of all things, the most intrusive vote ("Is it allowed >to split a large header to reduce dependencies?") got accepted by a >supermajority. I'll assemble a PR with just the header splitting >commits and submit it for merging.
Secondary votes are irrelevant if the primary one doesn't pass. cheers Derick

Max Kellermann

3 years ago
On 2023/02/16 08:59, Derick Rethans <derick@php.net> wrote:
> Secondary votes are irrelevant if the primary one doesn't pass.
You may be formally correct (or maybe not, because https://wiki.php.net/rfc/voting doesn't really say that). In any case, a vote that reaches supermajority (i.e. it would have been accepted if it had been a separate RFC) is an unambiguous expression on how the community wants the PHP source code to look like. It is safe to say that the PHP community doesn't want any include comments and forward declarations, but wants to split large headers in order to reduce header dependencies. I guess we both don't like the outcome of the vote (for different reasons), but let's not start lawyering pointlessly, and accept the community's will. Max

Tim Düsterhus

3 years ago
Hi On 2/16/23 09:28, Max Kellermann wrote:
>> Secondary votes are irrelevant if the primary one doesn't pass. > > You may be formally correct (or maybe not, because > https://wiki.php.net/rfc/voting doesn't really say that). > > In any case, a vote that reaches supermajority (i.e. it would have > been accepted if it had been a separate RFC) is an unambiguous > expression on how the community wants the PHP source code to look > like.
Not necessarily. It might've been the case that a voter believes that include cleanups should not happen, but at the same time believes that *if* cleanups happen, then splitting a header is a natural part of such a cleanup. The same is true for the secondary vote of the include comments. My understanding is that the primary concern of the "no" votes is the churn in the code base. Removing the existing include comments will just create additional churn and provide no value-add at all. It is perfectly possible to be both against "include comments" and "actively remove include comments". That said I can only summarize the entire RFC, vote and result as "unfortunate". As I've said in my previous email from Feb 9, as a maintainer I'm not sure what a "declined vote" effectively means for me, because the RFC text and vote description is pretty broad and unspecific. May I perform a scoped clean up within a single extension (ext/random in my case)? May I not? Do I need an explicit RFC? I feel like the vote actually made the situation less clear for me. Without this RFC I might've just proposed a PR in the future, made sure to check that I don't unnecessarily break compatibility, requested two or three reviews and it would likely have been approved, merged and shipped with whatever version comes next. Now it is much less obvious what to do or not to do. Best regards Tim Düsterhus

Max Kellermann

3 years ago
On 2023/02/16 17:52, Tim Düsterhus <tim@bastelstu.be> wrote:
> Not necessarily. It might've been the case that a voter believes that > include cleanups should not happen, but at the same time believes that *if* > cleanups happen, then splitting a header is a natural part of such a > cleanup.
Maybe, but that seems unlikely to me. 1. There is exactly one person who voted "NO" to the primary vote but "YES" to splitting headers. 2. If include cleanups happen, then splitting headers is the only proposed change that is likely to ever cause a merge conflict. If somebody fears "code churn", it wouldn't make sense to accept splitting headers as "natural part". I trust that the voters knew what they were voting for, and if there's a supermajority for splitting headers, then that's the will of the community.
> It is perfectly possible to be both against "include comments" and > "actively remove include comments".
What you're replying to is just explaining why I believe the secondary votes are not "irrelevant". Your reply doesn't disagree with that, it only speculates how some hypothetical reviewer could reasonably argue to reject PR 10472. That misses the point I tried to make, and I'd rather wait for those hypothetical reviewers to post actual reviews. There hasn't been any so far, even though I posted the PR before voting even began.
> I feel like the vote actually made the situation less clear for me.
Yes, I feel the same. Requiring a supermajority for this kind of decision doesn't appear to make sense. For some intrusive changes, requiring a supermajority makes sense (if there are serious downsides for the "losing" minority), but IMO not here. The downsides are as minimal as they can be. We're now in a situation where the majority of voters want the code cleanup, but the fact that I asked for the vote made it LESS likely that the majority gets what they want. That is backwards! The cast of a vote must never worsen the situation for the majority. Max