Bug 191456 - [stage] www/phpbb3: update to 3.0.12
Summary: [stage] www/phpbb3: update to 3.0.12
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: John Marino
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-28 11:57 UTC by Marco Steinbach
Modified: 2014-08-15 20:12 UTC (History)
2 users (show)

See Also:


Attachments
phpbb3-3.0.12.patch (9.30 KB, patch)
2014-06-28 11:57 UTC, Marco Steinbach
no flags Details | Diff
merged-phpbb3-3.0.12.patch (11.20 KB, patch)
2014-08-10 16:32 UTC, Marco Steinbach
no flags Details | Diff
merged-fixed-phpbb3-3.0.12.patch (11.26 KB, patch)
2014-08-13 11:23 UTC, Marco Steinbach
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marco Steinbach 2014-06-28 11:57:56 UTC
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)
Comment 1 John Marino freebsd_committer 2014-07-22 20:01:08 UTC
It's been over 3 weeks, patch can be committed without maintainer approval now
Comment 2 John Marino freebsd_committer 2014-07-27 11:30:12 UTC
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?
Comment 3 Marco Steinbach 2014-07-28 15:51:56 UTC
John,

thanks for the notification -- working on a new patch.
Comment 4 Bo-Yi Wu 2014-07-29 01:49:48 UTC
Hi Marco,

Please send new patch. Thanks for your help.

Bo-Yi Wu.
Comment 5 Marco Steinbach 2014-08-10 16:32:58 UTC
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
Comment 6 Marco Steinbach 2014-08-11 00:10:51 UTC
(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.
Comment 7 John Marino freebsd_committer 2014-08-11 05:36:38 UTC
Moving to patch-ready
Comment 8 John Marino freebsd_committer 2014-08-13 09:29:46 UTC
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.
Comment 9 Marco Steinbach 2014-08-13 11:23:20 UTC
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/
Comment 10 John Marino freebsd_committer 2014-08-13 20:58:37 UTC
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.
Comment 11 John Marino freebsd_committer 2014-08-13 21:00:45 UTC
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.
Comment 12 John Marino freebsd_committer 2014-08-13 21:03:20 UTC
Ah wait, I misunderstood, but it still doesn't make sense.  either ${WRKSRC}/config.php exists or it doesn't. why would it not?
Comment 13 John Marino freebsd_committer 2014-08-13 21:05:44 UTC
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.
Comment 14 John Marino freebsd_committer 2014-08-13 21:10:04 UTC
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.
Comment 15 John Marino freebsd_committer 2014-08-13 21:13:09 UTC
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.
Comment 16 John Marino freebsd_committer 2014-08-13 21:16:27 UTC
you must have mass replaced "www" with %%WWWOWN%% in the pkg-plist and you changed some text that you shouldn't have.
Comment 17 John Marino freebsd_committer 2014-08-13 21:22:47 UTC
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.
Comment 18 John Marino freebsd_committer 2014-08-13 21:42:58 UTC
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
Comment 19 John Marino freebsd_committer 2014-08-13 22:05:57 UTC
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.
Comment 20 commit-hook freebsd_committer 2014-08-13 22:46:25 UTC
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
Comment 21 John Marino freebsd_committer 2014-08-13 23:02:26 UTC
to follow up, the @sample keyword was actually ok.  .php.sample works fine.
Comment 22 Marco Steinbach 2014-08-15 19:48:57 UTC
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.
Comment 23 John Marino freebsd_committer 2014-08-15 20:12:11 UTC
(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.