php: whitespace and cleanup #8374
Conversation
|
Notifying maintainers: |
|
Travis Build #13910 Passed. Lint resultsPort php74 success on xcode10.3. Log The build timed out. |
www/apache2/Portfile
Outdated
| @@ -211,7 +211,7 @@ variant eventmpm conflicts preforkmpm workermpm description {Use event MPM (expe | |||
| } | |||
|
|
|||
| if {![variant_isset workermpm] && ![variant_isset eventmpm]} { | |||
| default_variants +preforkmpm | |||
| default_variants +preforkmpm +mpms_shared_all | |||
There was a problem hiding this comment.
This will not make +mpms_shared_all a default variant if the user selected +workermpm or +eventmpm. Is that what you intended?
Is there any reason for the mpms_shared_all variant to exist at all -- is there any reason why a user would want to disable that functionality? Why not build it all the time?
(This is the first time I've noticed that this variant was added to the port and I wasn't aware of this apache functionality before.)
If you add a new default variant, you must increase the revision. See https://guide.macports.org/#reference.keywords
There was a problem hiding this comment.
This will not make +mpms_shared_all a default variant if the user selected +workermpm or +eventmpm. Is that what you intended?
Ah, ok. I'll remove it then.
Is there any reason for the mpms_shared_all variant to exist at all -- is there any reason why a user would want to disable that functionality? Why not build it all the time
We added it here: #3384. It's a really great function and you don't need to rebuild Apache when you want to change mpm, + you can use all mpms at the same time (i diff vhost though).
Yes, if that's ok, I'd much more prefer that as a default arg. I guess it wouldn't be needed to req any variant for the apache2handler subport since it's already been built, put perhaps not enabled. Just need a note for that, to make sure they use it.
There was a problem hiding this comment.
Actually, with mpms_shared_all as a default arg in the build, one could also add prefork and then remove all the mpm variants. It'll just be a matter of configuring for the user.
--enable-mpms-shared=all \
--with-mpm=prefork \
…will build all 3, and add all 3 to httpd.conf, with the prefork version uncommented.
lang/php/Portfile
Outdated
| ui_error " | ||
| ${subport} @${version} requires ${php} @${version} but you have\ | ||
| ${php} @${php_version}. | ||
| " |
There was a problem hiding this comment.
This commit will be extremely difficult to review due to its size, so I'll just start with this "reformatting" of ui_error, which is not merely a reformatting: it changes the output that the user will see (introducing a newline before and after the error message). I don't really want that; that's not something I'm aware of us doing in any other port. Let's leave ui_errors formatted the way they were.
iefdev
force-pushed
the
php-whitespace-and-cleanup
branch
2 times, most recently
from
59a412d
to
78b2a19
Compare
|
Travis Build #13930 Passed. Lint resultsPort php74 success on xcode10.3. Log The build timed out. |
iefdev
force-pushed
the
php-whitespace-and-cleanup
branch
from
78b2a19
to
a0d1152
Compare
Since 3089477 gave a small conflict on this one, which I solved - but I updatded the PR instead, while taking your advice to split up the changes in to smaller ones.
I hope you'll find it easier to go through them now, as the changes doesn't make the code move around as much as before. Tried to keep the indentation changes to 3-4 blocks per commit. — Hm… Don't know why they've been split up in 2 blocks (12 and 2 days ago). I made all last night. |
|
Travis Build #14130 Passed. Lint resultsPort php74 success on xcode10.3. Log The build timed out. |
|
Only person who will commit this one is Ryan, so you'll negotiate it with him, @iefdev |
Yes, I know - and he knows: #8374 (comment) Looks like I have a few conflicts to solve now. |
- Clean up the whitespace
- Make sure the intendation is correct throughout
the file
- See: https://trac.macports.org/ticket/61155
This also shortens a few long lines (ie notes, variants)
Also shortens a few long lines.
- breaking a few long lines
- mysql/postgresql variants:
breaking up hte long lines, and also formatting the variants for
better readability. (ie conflicts and desc on separate lines with
small indentation)
Also, php8 is rc1 now.
iefdev
force-pushed
the
php-whitespace-and-cleanup
branch
from
a0d1152
to
49916c7
Compare
|
Solved the conflicts and repushed. |
|
Travis Build #14404 Passed. Lint resultsPort php74 success on xcode10.3. Log The build timed out. |
|
Eric -- I think all these ports are Ryan's right? If he wanted to commit this, it would have been committed. Whitespace and cleanup are very personal things -- I think Ryan will undertake that someday if he wants to. Whaddaya say we just close this and leave this for Ryan to do as he wishes in the ports he maintains? |
|
closing as per above comment |
Description
commit 1
php: whitespace and cleanup
the file
commit 2
php: fix postgresql variants
commit 3
php-apache2handler: req mpms_shared_all instead of preforkmpm
When installing the php-apache2handler it requires the preforkmpm to
be installed, which prevents the use of the other variant on the server
due to conflicts. That was what the mpms_shared_all variant was added
for, to be able to use all different mpms on the same apache2 install -
to be able to set up and run different virtualhosts with different mpms
&/or php versions.
So, it's better to require mpms_shared_all, and then add to the notes
they need to run the php-apache2handler in a preforkmpm mode.
Type(s)
Tested on
macOS 10.7.5 11G63
Xcode 4.6.3 4H1503
Verification
Have you
port lint?sudo port -vst install php74-postgresql +postgresql95?Notes
This is a PR I've wanted to do for some time now. And after the update yesterday I thought it was a good opportunity to do it now when I already had it open. It's quite a lot, but it's also a 2000+ lines file. (also see the trac ticket)
First commit is really a mess to look at in the diff, since it's moving part/blocks all around beacause of the change of intendetion. And it looks a lot more than it really is. It's easier to just look at the files side-by-side to follow along. Perhaps using GUI like Meld could be better - to spot the whitespace changes, etc.
2'nd commit I'm really pleased with, and I've made a test install to make sure it worked (
php74-postgresql +postgresql95).3'rd one is a small change I personally need to be able to upgrade/change my custom server to MP. It doesn't change anything, just makes it more flexible to use (see commit msg)
Hopefully it'll be a bit easier to both read and manage the file now. 🤞