Skip to content

#include cleanup 2 #10220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Jan 10, 2023
Merged

#include cleanup 2 #10220

merged 14 commits into from
Jan 10, 2023

Conversation

MaxKellermann
Copy link
Contributor

See #10216

@MaxKellermann MaxKellermann mentioned this pull request Jan 4, 2023
Merged
@MaxKellermann MaxKellermann force-pushed the include_cleanup2 branch 2 times, most recently from b3a6107 to 1b23fc7 Compare January 4, 2023 16:43
@Girgias
Copy link
Member

Girgias commented Jan 15, 2023

(Can we have a CI test with that option?)

Ideally, we should yes. Maybe just in Nightly? @iluuu1994 @cmb69 what are you opinion on the new CI option?

@iluuu1994
Copy link
Member

We probably have quite a few untested options. A PR would be appreciated, just wouldn't want the number of jobs to balloon up.

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

@iluuu1994 This breakes the master build (with dtrace). I think, this should be reverted now and reconsidered for merging once again after the fix.

@MaxKellermann
Copy link
Contributor Author

Revert 14 commits, just because one #include line was missing? This seems out of proportion. I'm about to submit a PR to re-add this line.

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

@MaxKellermann you are a terrorist :)
This shouldn't be simple reverted now because of some later commit.

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

@MaxKellermann can you provide a quick fix for the problem.

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Jan 16, 2023
@MaxKellermann
Zend/zend_execute: re-add #include zend_dtrace.h
412b4de 
This was removed by commit ecc880f, but that broke builds with
DTrace support; zend_execute.c does not actually need that header, but
zend_vm_execute.h does.

See php#10220 (comment)
for the build failure report.
@MaxKellermann MaxKellermann mentioned this pull request Jan 16, 2023
Closed
@MaxKellermann
Copy link
Contributor Author

@MaxKellermann can you provide a quick fix for the problem.

Here it is: #10334
Unfortunately untested because I have no computer with DTrace.

@shivammathur, can you test, please?

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

#10344 doesn't fix the build, but discloses a number of includes in the middle of the file. Why they are not at the top?

@MaxKellermann
Copy link
Contributor Author

#10344 doesn't fix the build

--verbose, please!

but discloses a number of includes in the middle of the file. Why they are not at the top?

I added the line right before the existing #include "zend_vm_execute.h" which is also not at the top (not by me, but 19 years ago by commit e50a6fd, later moved to the final location by fa30e47 by yourself!), and the dtrace header is only needed by zend_vm_execute.h, so I figured it belongs there.

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

I won't look into this any more.
This already stoles a half of my PHP day, as I'm not able to work on bug fixes.

I'm asking to revert all these include cleanup commits!

This is just a useless permutation. e.g. 68ada76 adds typedef struct _* that we didn't need before. How is this clearly?

@MaxKellermann
Copy link
Contributor Author

This is just a useless permutation. e.g. 68ada76 adds typedef struct _* that we didn't need before.

These are forward declarations, which is a common C trick to avoid header dependencies.

PHP didn't need them before because nobody cared about avoiding header dependencies - everything just included everything with no systematic approach - things just worked by by chance, not by design.

Things like the DTrace breakage is unfortunate fallout of such a code cleanup - sorry for that, I'll try my best to avoid this and clean up if it happens - but I don't think small regressions are a reason to revert everything and to question the whole idea.

@derickr
Copy link
Member

derickr commented Jan 16, 2023

FWIW, I agree with Dmitry here, and these should all be reverted. It adds nothing but clutter.

@cmb69
Copy link
Member

cmb69 commented Jan 16, 2023

Maybe this should go through the RFC process (and have a mostly complete implementation, so every voter can actually see what changes).

@shivammathur
Copy link
Member

@MaxKellermann

@shivammathur, can you test, please?

Thanks for the PR #10334. I can confirm it fixes the build with dtrace.

@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

@cmb69 I would accept the common decision even if I don't like it, but these changes shouldn't came uncontrolled.

@MaxKellermann
Copy link
Contributor Author

Maybe this should go through the RFC process (and have a mostly complete implementation, so every voter can actually see what changes).

Is there anything I can do to help with this process? Submit a PR for some PHP coding style document, and then you vote on it? Or how does this process work?

@cmb69
Copy link
Member

cmb69 commented Jan 16, 2023

Is there anything I can do to help with this process? Submit a PR for some PHP coding style document, and then you vote on it? Or how does this process work?

There is some general info available at https://wiki.php.net/rfc/howto. In this case, there is not necessarily the need for a full coding standard; it can just be about this include handling. This could be added to https://github.com/php/php-src/blob/master/CODING_STANDARDS.md, if accepted.

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Jan 16, 2023
@MaxKellermann
CODING_STANDARDS.md: add rules for #include directives
ed286f9 
This is my proposal for keeping header dependencies small and correct.

See php#10220 for the discussion.
@MaxKellermann MaxKellermann mentioned this pull request Jan 16, 2023
Closed
@MaxKellermann
Copy link
Contributor Author

There is some general info available at https://wiki.php.net/rfc/howto. In this case, there is not necessarily the need for a full coding standard; it can just be about this include handling. This could be added to https://github.com/php/php-src/blob/master/CODING_STANDARDS.md, if accepted.

Draft proposal: #10338 - let's move further discussion over there.

cmb69 added a commit that referenced this pull request Jan 16, 2023
@cmb69
Revert GH-10300
bf1cfc0 
Cf. <#10220 (comment)>.

This reverts commit 68ada76.
his reverts commit 45384c6.
This reverts commit ef7fbfd.
This reverts commit 9b9ea0d.
This reverts commit f15747c.
This reverts commit e883ba9.
This reverts commit 7e87551.
This reverts commit 921274d.
This reverts commit fc1f528.
This reverts commit 0961715.
This reverts commit a93f264.
This reverts commit 72dd94e.
This reverts commit 29b2dc8.
This reverts commit 05c7653.
This reverts commit 5190e5c.
This reverts commit 6b55bf2.
This reverts commit 184b4a1.
This reverts commit 4c31b78.
This reverts commit d44e968.
This reverts commit 4069a5c.
cmb69 added a commit that referenced this pull request Jan 16, 2023
@cmb69
Revert GH-10279
2f4973f 
Cf. <#10220 (comment)>.

This reverts commit 45a128c.
This reverts commit 1eb71c3.
This reverts commit 492523a.
This reverts commit c7a4633.
This reverts commit 308adb9.
This reverts commit cd27d5e.
This reverts commit c593340.
This reverts commit 46371f4.
This reverts commit 623e2e9.
This reverts commit e7434c1.
This reverts commit d28d323.
This reverts commit 1a067b8.
This reverts commit a55c0c5.
This reverts commit b5aeb3a.
This reverts commit f061a03.
This reverts commit b088575.
This reverts commit b1d4877.
This reverts commit 94f9a20.
This reverts commit 4831e48.
This reverts commit cd985de.
This reverts commit 9521d21.
This reverts commit d613615.
cmb69 added a commit that referenced this pull request Jan 16, 2023
@cmb69
Revert GH-10220
c8955c0 
Cf. <#10220 (comment)>.

This reverts commit ecc880f.
This reverts commit 588a07f.
This reverts commit f377e15.
This reverts commit b4ba16f.
This reverts commit 694ec1d.
This reverts commit 6b34de8.
This reverts commit aa1cd02.
This reverts commit 308fd31.
This reverts commit 16203b5.
This reverts commit 738fb5c.
This reverts commit 9fdbefa.
This reverts commit cd4a7c1.
This reverts commit 928685e.
This reverts commit 01e5ffc.
cmb69 added a commit that referenced this pull request Jan 16, 2023
@cmb69
Revert "#include cleanup (#10216)"
2d3427c 
Cf. <#10220 (comment)>.

This reverts commit e628c66.
@dstogov
Copy link
Member

dstogov commented Jan 16, 2023

@MaxKellermann the process assumes, you write an RFC (like https://wiki.php.net/rfc/constants_in_traits) and sends notification to @Internals. It's good if RFC contains a link to implementation PR. Then after a 2-weeks discussion and 2-weeks voting it may be accepted or rejected by 2/3 majority of voters. This is the way like we take decisions in PHP community. Your work doesn't affect PHP language but affects the PHP maintenance process, so may be a slight less restrictive rules should be applied. (I don't remember but everything should be already documented).

@cmb69
Copy link
Member

cmb69 commented Jan 16, 2023

and sends notification to @Internals

Not really to this GH account, but rather to the internals mailing list. :)

(I don't remember but everything should be already documented).

Yes, documented at https://wiki.php.net/rfc/voting (a couple of years ago it has been decided that all RFCs need a super-majority).

MaxKellermann added a commit to CM4all/php-src that referenced this pull request Jan 16, 2023
@MaxKellermann
CODING_STANDARDS.md: add rules for #include directives
a3a3f32 
This is my proposal for keeping header dependencies small and correct.

See php#10220 for the discussion.
@MaxKellermann MaxKellermann mentioned this pull request Jan 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias left review comments

@TimWolla TimWolla TimWolla approved these changes

@remicollet remicollet Awaiting requested review from remicollet

Assignees
No one assigned
Labels
Category: Engine Extension: intl Extension: json Extension: mbstring Extension: mysqlnd Extension: oci8 Extension: opcache Extension: openssl Extension: pcre Extension: pdo_pgsql Extension: pgsql Extension: phar Extension: random Extension: shmop Extension: soap Extension: sockets Extension: spl Extension: standard Extension: sysvmsg SAPI: cgi SAPI: cli SAPI: fpm SAPI: phpdbg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@MaxKellermann @Girgias @shivammathur @iluuu1994 @dstogov @derickr @cmb69 @TimWolla