Bug 185354 - [PATCH] sysutils/xmbmon: fix binary strip
[PATCH] sysutils/xmbmon: fix binary strip
Status: Closed FIXED
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s)
Latest
Any Any
: Normal Affects Only Me
Assigned To: John Marino
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-30 23:30 UTC by takefu
Modified: 2014-06-19 09:50 UTC (History)
4 users (show)

See Also:


Attachments
xmbmon-205_10.patch (1.90 KB, patch)
2013-12-30 23:30 UTC, takefu
marino: maintainer‑approval-
Details | Diff
Patch for xmbmon (1.42 KB, text/plain)
2014-06-16 11:34 UTC, Muhammad Moinur Rahman
no flags Details
And pointed out places modification, minor modification. (4.68 KB, patch)
2014-06-17 03:04 UTC, takefu
no flags Details | Diff
patch for xmbmon (3.47 KB, patch)
2014-06-17 15:09 UTC, Muhammad Moinur Rahman
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description takefu 2013-12-30 23:30:00 UTC
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)
Comment 1 Edwin Groothuis freebsd_committer 2013-12-30 23:30:07 UTC
State Changed
From-To: open->feedback

Awaiting maintainers feedback (via the GNATS Auto Assign Tool)
Comment 2 Edwin Groothuis freebsd_committer 2013-12-30 23:30:07 UTC
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
Comment 3 John Marino freebsd_committer 2014-06-15 11:24:46 UTC
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
Comment 4 Muhammad Moinur Rahman 2014-06-16 11:33:33 UTC
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.
Comment 5 Muhammad Moinur Rahman 2014-06-16 11:34:01 UTC
Created attachment 143829 [details]
Patch for xmbmon
Comment 6 Muhammad Moinur Rahman 2014-06-16 11:34:48 UTC
> 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:)
Comment 7 John Marino freebsd_committer 2014-06-16 11:39:03 UTC
wth, I'll take it.
Comment 8 takefu 2014-06-17 03:04:30 UTC
Created attachment 143858 [details]
And pointed out places modification, minor modification.

man/man1/mbmon.1 path fix
binary strip
Comment 9 John Marino freebsd_committer 2014-06-17 06:11:29 UTC
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.
Comment 10 John Marino freebsd_committer 2014-06-17 06:30:10 UTC
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.
Comment 11 Muhammad Moinur Rahman 2014-06-17 11:54:17 UTC
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.
Comment 12 Muhammad Moinur Rahman 2014-06-17 15:09:01 UTC
Created attachment 143874 [details]
patch for xmbmon
Comment 13 Muhammad Moinur Rahman 2014-06-17 15:09:26 UTC
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.
Comment 14 John Marino freebsd_committer 2014-06-17 15:14:46 UTC
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?
Comment 15 Muhammad Moinur Rahman 2014-06-17 17:51:58 UTC
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?
Comment 16 takefu 2014-06-18 02:08:22 UTC
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
Comment 17 John Marino freebsd_committer 2014-06-18 20:39:50 UTC
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.
Comment 18 John Marino freebsd_committer 2014-06-18 20:40:53 UTC
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.
Comment 19 John Marino freebsd_committer 2014-06-18 20:56:41 UTC
another issue:
Do *not* mute installation commands (e.g. @${INSTALL_PROGRAM})

It is against the rules.
It is permissible to mute ${MKDIR} though.
Comment 20 commit-hook freebsd_committer 2014-06-18 21:11:28 UTC
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
Comment 21 John Marino freebsd_committer 2014-06-18 21:13:09 UTC
By the way, this passed redports 8x.

I also wrapped some lines to make it respect 80 columns.

Thanks, guys.
Comment 22 takefu 2014-06-18 23:34:37 UTC
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
Comment 23 John Marino freebsd_committer 2014-06-18 23:37:34 UTC
If Muhammed accepts it, it will need a portrevision too
Comment 24 Muhammad Moinur Rahman 2014-06-19 09:27:14 UTC
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
Comment 25 commit-hook freebsd_committer 2014-06-19 09:49:11 UTC
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
Comment 26 John Marino freebsd_committer 2014-06-19 09:50:19 UTC
Okay, done.
takefu's patch violated the 80-column rule so it still has to be over two lines.
Thanks!