Bug 170506 - Review Request: grepmail
Review Request: grepmail
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orion Poplawski
David Lawrence
http://grepmail.sourceforge.net/
:
Depends On: 170507
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-10-12 10:22 EDT by Paul Howarth
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-12 12:10:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Paul Howarth 2005-10-12 10:22:40 EDT
Spec Name or Url: http://www.city-fan.org/~paul/extras/grepmail/grepmail.spec
SRPM Name or Url: http://www.city-fan.org/~paul/extras/grepmail/grepmail-5.3032-1.src.rpm

Description:
Grepmail searches a normal or compressed mailbox for a given regular
expression, and returns those emails that match it. Piped input is allowed,
and date and size restrictions are supported, as are searches using logical
operators.
Comment 1 Orion Poplawski 2005-10-12 10:56:08 EDT
- md5sums match
- builds clean
- rpmlint silent
- License checks out


Minor:

- remove the top comments.  We know it's an application.

Approved
Comment 2 Paul Howarth 2005-10-12 11:36:39 EDT
(In reply to comment #1)
> - md5sums match
> - builds clean
> - rpmlint silent
> - License checks out
> 
> 
> Minor:
> 
> - remove the top comments.  We know it's an application.

Will do; 'twas just a note to potential reviewers...


Comment 3 Paul Howarth 2005-10-12 12:10:40 EDT
Build on target development succeeded.
Comment 4 Ralf Corsepius 2005-10-12 12:15:37 EDT
Just a comment:
%{_bindir}/find ...

is pretty much free of sense. %_bindir is a user input parameter to rpmbuild,
not an rpm-internal constant (%__chmod etc. are rpmbuild internal constants).

Try rpmbuild --define '_bindir /tmp' to experience the difference.

I strongly recommend to use find or /usr/bin/find instead.
Comment 5 Paul Howarth 2005-10-13 09:05:32 EDT
(In reply to comment #4)
> Just a comment:
> %{_bindir}/find ...
> 
> is pretty much free of sense. %_bindir is a user input parameter to rpmbuild,
> not an rpm-internal constant (%__chmod etc. are rpmbuild internal constants).

Is this distinction described somewhere?

> Try rpmbuild --define '_bindir /tmp' to experience the difference.

Both %_bindir and %__chmod are defined in /usr/lib/rpm/macros and can be
redefined in the same way on the command-line. I always believed that the macros
were there to provide distribution-portability rather than being "user input
parameters".

> I strongly recommend to use find or /usr/bin/find instead.

I see what you're getting at here, in that redefining %_bindir so that the
binaries are installed in a different place breaks the build when it expects to
find things in the same place. I'd rather not use plain "find" because that
breaks reproducability of builds if someone has a different/broken version of
find earlier in their PATH, so /usr/bin/find looks better to me. But it does
make the assumption that %_bindir was set to /usr/bin when the distribution's
"find" package was built, which may not be true on all distributions (not that
any of this matters for Fedora Extras). Having said that, similar assumptions
are made for programs in /bin, /sbin etc., which don't have corresponding macros
defined for them. Ideally I suppose it would be better if there were additional
macros defined such as %__find etc., but it's a bit late for that now.

I'll probably change it to /usr/bin/find
Comment 6 Ralf Corsepius 2005-10-13 10:45:50 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > Just a comment:
> > %{_bindir}/find ...
> > 
> > is pretty much free of sense. %_bindir is a user input parameter to rpmbuild,
> > not an rpm-internal constant (%__chmod etc. are rpmbuild internal constants).
> 
> Is this distinction described somewhere?
Rhethorical question: Is there any official documentation on rpm?

Not that I am aware about. Only that "%_bindir" etc. are documented as
"configure macros" in /usr/lib/rpm/macros, which means they are meant to be
configuration input to be passed to a package, and do not denote a system
feature (such as presence of /usr/bin/find, or %{__chmod}).

Also, it's common practice for many years that users override the "autoconf"
macros for various purposes. 

%_bindir/find requires you to use "BuildRequires: %_bindir/find" which
unnecessarily prohibits users wanting to rebuild your rpm with a different _bindir.

> Both %_bindir and %__chmod are defined in /usr/lib/rpm/macros and can be
> redefined in the same way on the command-line.
True, but note that these %__XXX macros use hardcoded directories, such as
/usr/bin and do not use %_bindir. 

IMO, this is yet another strong indication for your usage to be incorrect.

However probably only Jeff would be able to give a confirmative answer.

Note You need to log in before you can comment on or make changes to this bug.