Created attachment 153944 [details] svn diff for net/liboping Well, it's been ~7 years since liboping was at 0.3.5. As you might guess; alot has happened/been added, since then. As a result; I have fixed the MASTER_SITES, WWW. tested the newest version (1.8.0) on 11-CURRENT, updated PORTVERSION to 1.8.0. updated pkg-descr to reflect the current status, and since it's been so long; I've taken the liberty of adding myself as MAINTAINER. Please find an svn diff, and complete QA LOG, attached. Thank you for all your time, and consideration. --Chris
Created attachment 153945 [details] QA LOG for net/liboping
Thanks Chris, On initial review: - COMMENT: Match upstream "tagline, comment" as close as possible. See: http://noping.cc/ COMMENT=C library to generate ICMP echo requests https://github.com/octo/liboping COMMENT=Protocol independent ANSI-C ping library and command line utility tldr; The original/previous value of COMMENT looks fine. - Always strip in post-install: - You're using INSTALL_TARGET=install-strip, but manual STRIP_CMD is still used? Use one or the other, ideally the former if it works. - You mention ncurses as an option, but there it doesn't look OPTION'al or conditional. - Try to use LIBS to add -lncurses instead of LDFLAGS - Dont hardcode -lncurses, use the variables provided by Mk/Uses/ncurses.mk - Group and sort USE_*/USES sections - When you update PORTVERSION, PORTREVISION is/should be reset (so remove it)
Don't forget ncurses can come from Base *or* Ports. If you can support both, do so. If you can't, you still want to test both scenarios.
(In reply to Kubilay Kocak from comment #3) > Don't forget ncurses can come from Base *or* Ports. If you can support both, > do so. If you can't, you still want to test both scenarios. Thank you very much for taking the time to review this, Kubilay Kocak. *curses is not an option, and as it is, it will happily use whichever (base, or ports) is available. As to the COMMENT; I modified it so that it better reflected the *current* features/functions. As the original one [currently] "misses the mark". As to: install-target v. ${STRIP_CMD} install-strip wasn't honored (portlint complained) but ${STRIP_CMD} made portlint happy. I'll remove INSTALL_TARGET* as it's redundant. Thanks again, Kubilay Kocak! --Chris
Created attachment 153949 [details] svn diff for net/liboping (UPDATED) I've modified COMMENT, and [hopefully] made it clearer/more concise. removed redundant *STRIP* call. I hope I've fulfilled your requirements, Kubilay Kocak. :-) Thanks! --Chris
(In reply to Chris Hutchinson from comment #5) > Created attachment 153949 [details] > svn diff for net/liboping (UPDATED) > > I've modified COMMENT, and [hopefully] made it > clearer/more concise. > removed redundant *STRIP* call. > > I hope I've fulfilled your requirements, Kubilay Kocak. :-) > > Thanks! > > --Chris Ack! I was attempting to do all this [respond && make changes] on 2 different boxes. As a result; I've missed a couple of points you made. I'm correcting all that now && from only one box this time. Sorry for the bother. I'll have a *correct* patch shortly. --Chris
Created attachment 153950 [details] svn diff for net/liboping CORRECTED OK this I think covers everything. Much cleaner this time, too. Thanks again, Kubilay Kocak, and sorry for the bother. --Chris
Thanks for the update Chris! 1) Add USES=pathfix to fix: pkg-plist: +lib/pkgconfig/liboping.pc The correct location for .pc files is: +libdata/pkgconfig/liboping.pc portlint(8) should have picked that up. See: Mk/Uses/pathfix.mk 2) I still believe the original COMMENT is much better as it matches upstream. Minor issue I know, but I personally have a *very* strong preference to have ports reflect upstream information as closely as possible for user confidence. 3) As per original review: STRIP_CMD should take place in post-install. There is still a pre-install: STRIP_CMD 4) While you're there, and if you haven't already: Add DEVELOPER=yes in /etc/make.conf to enable extra QA/Sanity checks: See: Mk/bsd.sanity.mk
(In reply to Kubilay Kocak from comment #8) > Thanks for the update Chris! No. Thank *you* for taking the time to review this, Kubilay Kocak. :-) > > 1) Add USES=pathfix to fix: > > pkg-plist: +lib/pkgconfig/liboping.pc > > The correct location for .pc files is: > > +libdata/pkgconfig/liboping.pc > > portlint(8) should have picked that up. > > See: Mk/Uses/pathfix.mk Dunno why, but portlint said nothing. But I have it listed now. > > 2) I still believe the original COMMENT is much better as it matches > upstream. Minor issue I know, but I personally have a *very* strong > preference to have ports reflect upstream information as closely as possible > for user confidence. I concede (with *minor* amendment). :-) > > 3) As per original review: STRIP_CMD should take place in post-install. > There is still a pre-install: STRIP_CMD Tried that originally, but there were issues (see additional QA log for details). > > 4) While you're there, and if you haven't already: > > Add DEVELOPER=yes in /etc/make.conf to enable extra QA/Sanity checks: > > See: Mk/bsd.sanity.mk The box I develop on, is only used for development. make.conf(5) has one line: DEVELOPER=yes But, yes. Good advice. :-) Please see the added QA log, that contains a running dialog. I think [hope] this satisfies everything. Thanks, Kubilay Kocak! --Chris
Created attachment 153970 [details] svn diff for net/liboping with modifications as requested
Created attachment 153971 [details] QA LOG for net/liboping (with running dialog) This QA log is interspersed with comments in regards to requests for modifications.
(In reply to Chris Hutchinson from comment #9) >> 3) As per original review: STRIP_CMD should take place in post-install. >> There is still a pre-install: STRIP_CMD > > Tried that originally, but there were issues (see additional QA log for > details). This recording allows you to transfer strip of "pre-install:" in "post-install:" ${STRIP_CMD} ${STAGEDIR}${PREFIX}/lib/liboping.so.0.2.11
Created attachment 154025 [details] svn diff for net/liboping with all modifications as, requested Right you are. I should have known better. :-{ I've changed it, and [of course] it works as intended. Thanks! --Chris
(In reply to Kubilay Kocak from comment #3) > Don't forget ncurses can come from Base *or* Ports. If you can support both, > do so. If you can't, you still want to test both scenarios. Thank you very much for taking the time to review this, Kubilay Kocak. *curses is not an option, and as it is, it will happily use whichever (base, or ports) is available. As to the COMMENT; I modified it so that it better reflected the *current* features/functions. As the original one [currently] "misses the mark". As to: install-target v. ${STRIP_CMD} install-strip wasn't honored (portlint complained) but ${STRIP_CMD} made portlint happy. I'll remove INSTALL_TARGET* as it's redundant. Thanks again, Kubilay Kocak! --Chris (In reply to f0andrey from comment #12) > (In reply to Chris Hutchinson from comment #9) > >> 3) As per original review: STRIP_CMD should take place in post-install. > >> There is still a pre-install: STRIP_CMD > > > > Tried that originally, but there were issues (see additional QA log for > > details). > > This recording allows you to transfer strip of "pre-install:" in > "post-install:" > ${STRIP_CMD} ${STAGEDIR}${PREFIX}/lib/liboping.so.0.2.11 I submitted an updated svn diff to reflect the changes in response to this comment. But bugzilla crashed, complaining about being unable to send an email to the maintainer (matt@peterson.org). So I'm just replying again, in hopes bugzilla doesn't crash this time. Thanks! --Chris
(In reply to Chris Hutchinson from comment #14) > (In reply to Kubilay Kocak from comment #3) > > Don't forget ncurses can come from Base *or* Ports. If you can support both, > > do so. If you can't, you still want to test both scenarios. > > Thank you very much for taking the time to review > this, Kubilay Kocak. > *curses is not an option, and as it is, it will happily > use whichever (base, or ports) is available. > > As to the COMMENT; I modified it so that it better > reflected the *current* features/functions. As the > original one [currently] "misses the mark". > > As to: > install-target v. ${STRIP_CMD} > install-strip wasn't honored (portlint complained) > but ${STRIP_CMD} made portlint happy. I'll remove > INSTALL_TARGET* as it's redundant. > > Thanks again, Kubilay Kocak! > > --Chris > (In reply to f0andrey from comment #12) > > (In reply to Chris Hutchinson from comment #9) > > >> 3) As per original review: STRIP_CMD should take place in post-install. > > >> There is still a pre-install: STRIP_CMD > > > > > > Tried that originally, but there were issues (see additional QA log for > > > details). > > > > This recording allows you to transfer strip of "pre-install:" in > > "post-install:" > > ${STRIP_CMD} ${STAGEDIR}${PREFIX}/lib/liboping.so.0.2.11 > I submitted an updated svn diff to reflect the changes in > response to this comment. > But bugzilla crashed, complaining about being unable to > send an email to the maintainer (matt@peterson.org). > So I'm just replying again, in hopes bugzilla doesn't > crash this time. > > Thanks! > > --Chris Does this look alright now? Thanks! --Chris
Comment on attachment 154025 [details] svn diff for net/liboping with all modifications as, requested Maintainer timeout. Open for commit.
testing@work
A commit references this bug: Author: pi Date: Sun Mar 29 16:44:19 UTC 2015 New revision: 382613 URL: https://svnweb.freebsd.org/changeset/ports/382613 Log: net/liboping: 0.3.5 -> 1.8.0 Changes: http://noping.cc/#documentation PR: 198379 Submitted by: Chris Hutchinson <portmaster@bsdforge.com> (new maintainer) Approved by: maintainer timeout Changes: head/net/liboping/Makefile head/net/liboping/distinfo head/net/liboping/pkg-descr head/net/liboping/pkg-plist
Build tested on 10.1a, 9.3a, 8.4i, looks fine. Committed, thanks!