Created attachment 144219 [details] phpbb3-3.0.12.patch - Enable staging - Update to 3.0.12 - Changes to pkg-plist (@sample, @owner, etc) - More proper permission fixing during installation - Handle DOCS and pkg-message differently - Other minor fixes I am interested in taking maintainership of this port, if the original maintainer is unavailable or has lost interest. Port maintainer (appleboy.tw@gmail.com) is cc'd. Generated with FreeBSD Port Tools 0.99_7 (mode: update, diff: SVN)
It's been over 3 weeks, patch can be committed without maintainer approval now
Marco, The same day that you submitted this PR, the port was staged through bug 189963 (which was approved by maintainer). Can you please resubmit this patch so that it applies on the current version?
John, thanks for the notification -- working on a new patch.
Hi Marco, Please send new patch. Thanks for your help. Bo-Yi Wu.
Created attachment 145613 [details] merged-phpbb3-3.0.12.patch - Take maintainership (many thanks to Bo-Yi Wu <appleboy.tw@gmail.com> for giving this port a good home) - Update to 3.0.12 - Add DOCS knob - Move permission handling to pkg-plist
(https://redports.org/changeset/30270) www/phpbb3 3.0.12 8.4-RELEASE/amd64 success 9.2-RELEASE/amd64 success 11-CURRENT/amd64 success 10.0-RELEASE/amd64 success 11-CURRENT/i386 success 10.0-RELEASE/i386 success 9.2-RELEASE/i386 success 8.4-RELEASE/i386 success EXP1 success QATty success 9.3-RELEASE/amd64 success 9.3-RELEASE/i386 success coco@x2q:~/redports/coco/www/phpbb3 % portlint . WARN: Makefile: only one MASTER_SITE configured. Consider adding additional mirrors. WARN: Makefile: no ftp/http mirror in MASTER_SITES for users behind a proxy. 0 fatal errors and 2 warnings found.
Moving to patch-ready
looking at the diff, two things stand out: 1) you have ".include <bsd.port.options.mk>" in the middle of the targets. It usually comes before all the targets. 2) You have both do-install and post-install targets. This is not necessary, it should be combined into a single "do-install" target. post-install is for when the installation is handle by the vendor makefile and it needs extra help.
Created attachment 145754 [details] merged-fixed-phpbb3-3.0.12.patch Thank you -- I fixed the two mistakes. coco@x2q:~/redports/coco/www/phpbb3 % portlint . WARN: Makefile: only one MASTER_SITE configured. Consider adding additional mirrors. WARN: Makefile: no ftp/http mirror in MASTER_SITES for users behind a proxy. 0 fatal errors and 2 warnings found. coco@x2q:~/redports/coco/www/phpbb3 % https://redports.org/buildarchive/20140813110600-65470/
I'll fix this for you, but the install commands can't be masked by "@" in the do-install target. This rule will probably be added to portlint in the future.
also, the config.sample instruction is not right, it's a leftover from pre-stage stages. You are installing it in stage directory, which was just empty, so config file will of course not be present. ever. No need to check for its existence.
Ah wait, I misunderstood, but it still doesn't make sense. either ${WRKSRC}/config.php exists or it doesn't. why would it not?
likewise, why is there an exclusion on config.php when it's impossible that there is a config.php since you just moved it (if it existed)? The condition can never be satisfied, so no need to check for it.
hmmm, I'm a big fan of "PORTDOCS=*" which you removed and caused %%PORTSDOC%% lines to have to be added to the pkg-plist. Did you change this intentionally? Because that increased your maintenance burden (docs are html which change a lot) and I don't recommend it, the html docs should be dynamically generated in my opinion.
oh no, all the "@dirrm " got incorrectly changed to "@dirrmtry " meaning this plist probably came from "make makeplist" and the @dirrmtry lines weren't modified. To use @dirrmtry in place of @dirrm is considered wrong even though it passes checks. It generates a lot of commands internally, so you need to @dirrm over @dirrmtry wherever possible.
you must have mass replaced "www" with %%WWWOWN%% in the pkg-plist and you changed some text that you shouldn't have.
okay, the very first patch converted config.php.sample to @sample keyword. in the meantime, @sample keyword was introduced to the port in the final version, you actually remove the @sample keyword that is now in the port. I think that was probably unintentional.
This PR is cursed. I got the commit message wrong (I referenced the previous PR). Okay, so I was expecting a cakewalk after those poudriere runs but there were plenty of things I thought to be mistakes. Please check all my work in case I made one myself. I did a lot of changes to the pkg-plist as I mentioned in my running commentary. John Author: marino Date: Wed Aug 13 21:36:55 2014 New Revision: 364811 URL: http://svnweb.freebsd.org/changeset/ports/364811 QAT: https://qat.redports.org/buildarchive/r364811/ Log: Stage www/phpbb3 and pass maintainership to submitter Upgrade version from 3.0.11 => 3.0.12 while here PR: 189963 Submitted by: Marco Steinbach Approved by: former maintainer (Bo-Yi Wu) Lotsa TLC: marino Modified: head/www/phpbb3/Makefile head/www/phpbb3/distinfo head/www/phpbb3/files/pkg-message.in head/www/phpbb3/pkg-plist
actually, the @sample is broken isn't it? <something>.conf.sample works but <something>.php.sample doesn't IIRC I might need to review that.
A commit references this bug: Author: marino Date: Wed Aug 13 22:45:46 UTC 2014 New revision: 364819 URL: http://svnweb.freebsd.org/changeset/ports/364819 Log: www/phpbb3: Replace @sample keyword The @sample keyword got used here but it didn't work because the extension was php.sample instead of conf.sample. Fix it with @unexec and @exec tricks. By the way, the previous commit to this port and the one that should have provided this @sample fix is: PR: 191456 Changes: head/www/phpbb3/Makefile head/www/phpbb3/pkg-plist
to follow up, the @sample keyword was actually ok. .php.sample works fine.
First: Thanks for not simply bouncing this back to me, but rather spending time on fixing and (even more important to me) explaining on the way. I'll try to reply in order: - Comment 10 Didn't know that, didn't find anything in the porters handbook (or ports/StageDir wiki page) to that effect. - Comment 11, 12 and 13 Routinely distrust things -- made sure it really isn't copied, if for whatever reason the system lied to me about the move having succeeded. - Comment 14 That's the way I understood the PORTDOCS PORTEXAMPLES section in the ports/StageDir wiki page (use %%PORTDOCS%% in pkg-plist), and 7.4 in the Porters Handbook (... maintainers should use static package lists wherever possible ...). I'd have prefered to keep it that way for this port, since it's just a directory instead of a boatload of option dependand files splattered around in various locations. - Comment 15 My bad. As a sidenote, 'pkg delete -yf' silences the warnings completely, while the man page says, that it'll just remove the port despite leaving unresolved dependencies. - Comments 16 and 17 My original patches were correct, and then I obviously fscked that up during the merge M( Please reset Bug 192566 -- it also needs some of the changes you mentioned. I'll provide a new patch. I'll check my other ports for the things you listed and fix them accordingly.
(In reply to Marco Steinbach from comment #22) > First: Thanks for not simply bouncing this back to me, but rather spending > time on fixing and (even more important to me) explaining on the way. Thanks, I know that helps although running commentary can be hard to follow. > I'll try to reply in order: > - Comment 10 > Didn't know that, didn't find anything in the porters handbook (or > ports/StageDir wiki page) to that effect. It was supposed to be there, and it was only recently noticed it's not there. It might be there now. Anyway, now you know. > - Comment 11, 12 and 13 > Routinely distrust things -- made sure it really isn't copied, if for > whatever reason the system lied to me about the move having succeeded. I think you can assume every operation succeeds. In other words, if the preceding command unconditionally moves a file, you can assuming that move succeeded. This is especially true for extract and manipulate; that should be repeatable all day long. This is the norm for ports btw. > - Comment 14 > That's the way I understood the PORTDOCS PORTEXAMPLES section in the > ports/StageDir wiki page (use %%PORTDOCS%% in pkg-plist), and 7.4 in the > Porters Handbook (... maintainers should use static package lists wherever > possible ...). > > I'd have prefered to keep it that way for this port, since it's just a > directory instead of a boatload of option dependand files splattered around > in various locations. There are two methods and now you have seen them both. If you define PORTDOCS, you don't need them in the pkg-plist because these entries will be automatically generated. I personally like this better because A) the pkg-plist is shorter B) not only that, but the contents are non-documentation, stuff I'm more interested in, C) it makes future maintenance easier because docs is likely to change significantly from release to release. The other way is what you changed it to which has all the opposite characteristics. PORTDOCS=<something> is an exception to the static plist directive. Having static pkg-plist + PORTSDOCS autogeneration is perfectly acceptable. > > - Comments 16 and 17 > My original patches were correct, and then I obviously fscked that up during > the merge M( okay, it happens. > Please reset Bug 192566 -- it also needs some of the changes you mentioned. > I'll provide a new patch. done. > I'll check my other ports for the things you listed and fix them accordingly. sounds good. Thanks for the feedback.