fix binary strip manpage Update LICENSE Port maintainer (5u623l20@gmail.com) is cc'd. Generated with FreeBSD Port Tools 0.99_11 (mode: change, diff: ports)
State Changed From-To: open->feedback Awaiting maintainers feedback (via the GNATS Auto Assign Tool)
Maintainer of sysutils/xmbmon, Please note that PR ports/185354 has just been submitted. If it contains a patch for an upgrade, an enhancement or a bug fix you agree on, reply to this email stating that you approve the patch and a committer will take care of it. The full text of the PR can be found at: http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/185354 -- Edwin Groothuis via the GNATS Auto Assign Tool edwin@FreeBSD.org
Muhammad, What do you want to do about this old PR? John P.S. You email address is very confusing. It looks like the number 623120, but it's really 623-ell-20. It's very easy to make a mistake
1. It is not BSD3CLAUSE it should be BSD2CLAUSE. Please check ${WRKSRC}/CopyRight 2. While STAGE support is on we have to install DOCS to ${STAGEDIR}/${DOCSDIR} not only ${DOCSDIR}. However in my version there was a typo and hence ${STAGEIR} should be replaced with ${STAGEDIR}. 3. Manpages are properly installing. There is a condition based on OPTIONS CLI and X11. And hence they are being installed properly. Please find the attached patch.
Created attachment 143829 [details] Patch for xmbmon
> P.S. You email address is very confusing. It looks like the number 623120, > but it's really 623-ell-20. It's very easy to make a mistake It's intentional. 3:)
wth, I'll take it.
Created attachment 143858 [details] And pointed out places modification, minor modification. man/man1/mbmon.1 path fix binary strip
If you guys are patching the vendor makefile anywhy, you should be replacing stuff like "$(INSTALL) -o root -g wheel -m 4555 -c -p" with "${BSD_INSTALL_DATA}", "${BSD_INSTALL_PROGRAM}". And I doubt the @mode change in PLIST is necessary either. If the file is installed in stage correctly with the right mode, you don't need to change it via plist. Just glancing at this, I think it's probably full of hacks from to compensate from installing directly on the system -- hacks that are no longer necessary. Can you guys look at that? the vendor makefile and removing mode from plist? hint: "make -V MAKE_ENV" will show you which install macros are available to use.
Thinking about this more: The PLIST is very short right? So it seems to me that a better approach is just forget patching the vendor makefile and override it with a custom "do-install:" target in the ports makefile. I would not patch a vendor makefile for a single file, I'd just install that file to stagedir directly, exactly how I want it.
This sounds a good approach. Let me work on it. (In reply to John Marino from comment #10) > Thinking about this more: > > The PLIST is very short right? > So it seems to me that a better approach is just forget patching the vendor > makefile and override it with a custom "do-install:" target in the ports > makefile. I would not patch a vendor makefile for a single file, I'd just > install that file to stagedir directly, exactly how I want it.
Created attachment 143874 [details] patch for xmbmon
Have submitted another patch. Please check it. (In reply to John Marino from comment #10) > Thinking about this more: > > The PLIST is very short right? > So it seems to me that a better approach is just forget patching the vendor > makefile and override it with a custom "do-install:" target in the ports > makefile. I would not patch a vendor makefile for a single file, I'd just > install that file to stagedir directly, exactly how I want it.
at first glance, that looks much better. I'm not really a fan of the use of "${PORTNAME}" in the do-install target though. If the PORTNAME changed for some reason, the do-install target would fail, right? I find it more correct and actually more readable if "xmbmon" was used instead of a variable. I just don't see the advantage of using a variable here. (Don't worry, similar things seem to be done commonly, I just don't understand why variables are used when the variable can only have one value). Assuming that's the only thing of note, would you mind if I changed that?
No. Go ahead. (In reply to John Marino from comment #14) > at first glance, that looks much better. > > I'm not really a fan of the use of "${PORTNAME}" in the do-install target > though. If the PORTNAME changed for some reason, the do-install target > would fail, right? > > I find it more correct and actually more readable if "xmbmon" was used > instead of a variable. I just don't see the advantage of using a variable > here. > > (Don't worry, similar things seem to be done commonly, I just don't > understand why variables are used when the variable can only have one > value). > > Assuming that's the only thing of note, would you mind if I changed that?
The setuid Though it is not necessary that it's in the stage, Do not needed after installation? "$ {STAGEDIR}" in the text of MAN is this feature is not necessary? (In reply to Muhammad Moinur Rahman from comment #12) > Created attachment 143874 [details] > patch for xmbmon
I'm having a hard time understanding the last comment. Everything looks fine to me right now except MANPREFIX should be used instead of PREFIX. I didn't see any reference to setuid in the original or the new versions, so I didn't understand this question.
Oh, I meant to mention: If you define "do-install:" target, there's no need for a separate "post-install:" target. You'd just combine these.
another issue: Do *not* mute installation commands (e.g. @${INSTALL_PROGRAM}) It is against the rules. It is permissible to mute ${MKDIR} though.
A commit references this bug: Author: marino Date: Wed Jun 18 21:10:47 UTC 2014 New revision: 358319 URL: http://svnweb.freebsd.org/changeset/ports/358319 Log: sysutils/xmbmon: Strip installed binary Move installation from vendor makefile to do-install target to properly install the executables, man pages, and documentation. PR: 185354 Submitted by: takefu (airport.fm) Reworked by: maintainer (Mohammad M. Rahman) tweaked by: marino Changes: head/sysutils/xmbmon/Makefile head/sysutils/xmbmon/files/patch-Makefile.in
By the way, this passed redports 8x. I also wrapped some lines to make it respect 80 columns. Thanks, guys.
Take a look at the following patch is that you want to say of me. > post-patch: > - @${REINPLACE_CMD} -e 's+/usr/share/doc/mbmon+${STAGEDIR}${DOCSDIR}+' \ > - ${WRKSRC}/mbmon.1 > + @${REINPLACE_CMD} -e 's+/usr/share/doc/mbmon+${DOCSDIR}+' ${WRKSRC}/mbmon.1
If Muhammed accepts it, it will need a portrevision too
Hmm .. Got it this time. Yes it should be removed. (In reply to John Marino from comment #23) > If Muhammed accepts it, it will need a portrevision too
A commit references this bug: Author: marino Date: Thu Jun 19 09:48:42 UTC 2014 New revision: 358371 URL: http://svnweb.freebsd.org/changeset/ports/358371 Log: sysutils/xmbmon: Fix manpage replacement error Spotted by: takefu (airport.fm) Approved by: Muhammad Rahman PR: 185354 Changes: head/sysutils/xmbmon/Makefile
Okay, done. takefu's patch violated the 80-column rule so it still has to be over two lines. Thanks!