[VOTE] include cleanup

3 years ago by Max Kellermann — view source — reply

unread

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.]

3 years ago by tim@bastelstu.be — view source — reply

unread

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

3 years ago by tim@bastelstu.be — view source — reply

unread

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

3 years ago by Peter Kokot — view source — reply

unread

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.]

--

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.

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by Peter Kokot — view source — reply

unread

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.

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by Derick Rethans — view source — reply

unread

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

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by tim@bastelstu.be — view source — reply

unread

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

3 years ago by Matthew Weier O'Phinney — view source — reply

unread

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

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by G. P. B. — view source — reply

unread

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

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by Dmitry Stogov — view source — reply

unread

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.

3 years ago by Max Kellermann — view source — reply

unread

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

3 years ago by Dmitry Stogov — view source — reply

unread

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.

3 years ago by Arvids Godjuks — view source — reply

unread

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

3 years ago by Clint Priest — view source — reply

unread

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.
This happened to me years ago and while I'm still here and reading the
threads, I backed off from contributing any further. Many weeks of
getting up early to work on the code for an hour before I had to head to
work thrown away without even any better reason than "long-term
maintenance" without me sticking around to do so.

Just turned me off from the whole situation.

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?

3 years ago by Alex Wells — view source — reply

unread

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.
This happened to me years ago and while I'm still here and reading the
threads, I backed off from contributing any further. Many weeks of
getting up early to work on the code for an hour before I had to head to
work thrown away without even any better reason than "long-term
maintenance" without me sticking around to do so.

Just turned me off from the whole situation.

Agreed. There are clearly major issues with communication.

If Dmitry was against merging into 8.x and instead wanted to postpone it to
9.x, he should have said so on the merge request. The PR should not have
been reverted simply on the basis of not being able to review the PR in
time. It's your responsibility (even on a project like PHP) to review the
PRs; if you don't want them to be merged without your approval then there
are merge rules on GitHub just for that.

The amount of roadblocks and the resistance for contributions is what
throws off the little number of new people wanting to contribute :/

3 years ago by Max Kellermann — view source — reply

unread

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

[VOTE] include cleanup

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?

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?