This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 187610 - Review Request: crm114 - CRM114 Bayesian Spam Detector
Review Request: crm114 - CRM114 Bayesian Spam Detector
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jima
Fedora Package Reviews List
:
Depends On: 187609
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-01 13:47 EST by Dominik 'Rathann' Mierzejewski
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: 2006-08-17 19:08:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dominik 'Rathann' Mierzejewski 2006-04-01 13:47:14 EST
Spec Name or Url: http://rpm.greysector.net/extras/crm114.spec
SRPM Name or Url: http://rpm.greysector.net/extras/crm114-0-1.20060118.src.rpm
Description:
CRM114 is a system to examine incoming e-mail, system log streams,
data files or other data streams, and to sort, filter, or alter the
incoming files or data streams according to the user's wildest
desires. Criteria for categorization of data can be by satisfaction of
regexes, by sparse binary polynomial matching with a Bayesian Chain
Rule evaluator, or by other means.

Note: BuildRequires tre-devel, which I submitted as #187609 .
Comment 1 Jima 2006-06-26 14:51:24 EDT
Taking ownership of this after a LUG member noted it wasn't in Fedora. Review is
pending resolution of bug #187609; reporter explained current situation to me
via IRC.
Comment 2 Patrice Dumas 2006-08-08 11:10:19 EDT
If I'm not wrong this bug should block FE-REVIEW and be in 
ASSIGNED state...
Comment 3 Dominik 'Rathann' Mierzejewski 2006-08-08 15:56:25 EDT
Yes. Jima said on IRC that he'll do it shortly, no worries.
Comment 4 Jima 2006-08-08 21:13:04 EDT
Patrice: Don't worry, Rathann and I are in active communication; he knows
exactly what my status on this review is. :-)

As for why I hadn't blocked FE-REVIEW yet, I was waiting until its dependent bug
(187609) cleared review.  There were some issues, but they've, well, sort of
been cleared up.  Other issues cropped up which delayed my turn in this review,
but Rathann was understanding.  (Thanks!)

Rathann: Off the cuff, since tre has 'ExcludeArch: x86_64', shouldn't crm114
have it, too?  I'd also welcome a newer version, if it's available. :-)

I'll start poking this a little.  Let me know if you have any revisions.
Comment 5 Patrice Dumas 2006-08-09 05:16:29 EDT
(In reply to comment #4)
> Patrice: Don't worry, Rathann and I are in active communication; he knows
> exactly what my status on this review is. :-)

You both know, but setting FE-REVIEW helps others know :-), so it is 
important to set it when you are sure that you'll review the package 
such that it appears as such, for example here:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-REVIEW&hide_resolved=1
and also don't appear anymore in FE-NEW as a package to be reviewed...

not a big deal, though.
Comment 6 Paul F. Johnson 2006-08-14 17:26:49 EDT
Release: %{rel}.%{cvsver} - you need the %{?dist} tag adding

%build
%{__make} %{?_smp_mflags} INSTALL_DIR=$RPM_BUILD_ROOT%{_bindir}
CFLAGS="$RPM_OPT_FLAGS"

does the install dir need to be here (and the CFLAGS)?

install -d $RPM_BUILD_ROOT{%{_bindir},%{_datadir}/%{name}}

the make install should create these directories for you. If they don't, mkdir
-p is the way to go.

%{__make} BINDIR=${RPM_BUILD_ROOT}%{_bindir} install

Isn't make DEST_DIR=%{buildroot} install more usual?

%{_datadir}/%{name}/*.crm

This just needs to be %{_datadir}/%{name}/

The binary files should already be 755, so the %defattr before them shouldn't be
required.
Comment 7 Dominik 'Rathann' Mierzejewski 2006-08-15 17:27:55 EDT
http://rpm.greysector.net/extras/crm114.spec
http://rpm.greysector.net/extras/crm114-0-1.20060704a.src.rpm

New version, fixed parallel make and rpmlint problems.
Comment 8 Jima 2006-08-15 21:56:09 EDT
Going down my own review checklist:
http://beer.tclug.org/fedora-extras/review-checklist.txt

1. One rpmlint error; will cover below.
2. I have some concerns about the version/release of this package. Will discuss
below.
3. Spec is named crm114.spec, check.
4. Package meets Packaging Guidelines, AFAICT.
5. Licensed under GPL, check.
6. License: GPL, check.
7. n/a, no LICENSE/COPYING file in tarball.
8. Spec appears to be American English.
9. Spec seems legible.
10. md5sum on crm114-20060704a-BlameRobert.no-TRE.src.tar.gz does NOT match
upstream. WTF?
11. Compiles and builds on i386/ppc (my two supported build platforms).
12. x86_64 excluded, as per dependency on tre-devel.  You should know the drill.
13. Builds under Plague, so I imagine all of its dependencies are listed.
14. n/a, I think.
15. n/a (no shared libs)
16. n/a
17. crm114-emacs does not own %{_datadir}/emacs/site-lisp/; I believe emacs-el
does.  Please add a dependency.
18. No duplicate %files entries.
19. Not 100% certain on the defattr for the main package.
20. Has valid %clean section.
21. Macro use appears consistent.
22. Package contains code, not content.
23. The doc directory makes up more than half the package's size; you may want
to consider splitting it off to a -doc subpackage. (As discussed on IRC.)
24. I don't see anything in %doc affecting runtime.
25. No header files or static libraries.
26. No .pc files.
27. No library files, much less ones with suffixes.
28. n/a (no -devel subpackage)
29. No .la files.
30. No GUI applications.
31. Doesn't own any directories owned by other packages (to the best of my
knowledge).
32. Packager should poke upstream to include a LICENSE file.
33. I'm not sure there are any description/summary translations available.
34. Package builds as i386 and ppc in Plague (and thus Mock).
35. Package won't build on x86_64 due to dependency's ExcludeArch: x86_64; other
architectures, yes.
36. I can't verify full functionality, but the binary doesn't segfault on i386/ppc.
37. No scriptlets.
38. The -emacs subpackage doesn't Requires: %{name} = %{version}-%{release};
submitter may want to consider doing so.

 So the standing issues, as I see them (some of which we discussed), are:

- rpmlint returns:
W: crm114-emacs no-documentation
 Not sure it's worth remedying.  I welcome more experienced reviewers to chime
in on this.
- I'm concerned about the versioning scheme.  How will you be able to re-release
20060704a if there's a bug?  Incrementing Release to 2.20060704a will preclude
resetting the first digit to 1 with the next release.  Also, I believe the 'a'
violates the Naming Guidelines.  Again, I welcome outside feedback on this.
- #10 concerns me greatly.  Did you repack the tarball or something?
- I think you're missing ExcludeArch: x86_64 (I wouldn't notice, as I don't have
an x86_64 buildsys, but this has been a subject of discussion).
- Maybe add Req: emacs-el for -emacs, due to that package owning the directory
the file is in.  (I believe you did this already, offline.)
- Is %defattr(-,root,root,-) well-defined enough?
- You might want to split out the documentation to a subpackage.
- Maybe ask upstream to provide a LICENSE file.
- Maybe add Req in #38.

 I'm sure we'll be in touch.
Comment 9 Jima 2006-08-15 22:02:35 EDT
Hmm, I guess Firefox's focus was on Component when I hit PageUp.  Oops!
Comment 10 Dominik 'Rathann' Mierzejewski 2006-08-16 19:49:16 EDT
http://rpm.greysector.net/extras/crm114.spec
http://rpm.greysector.net/extras/crm114-0-0.1.20060704.src.rpm

Changed the versioning like we discussed on IRC.
Fixed all of the above, too.
Added %check.
Referenced the bug in tre which prevents x86_64 version from working.
Comment 11 Jima 2006-08-17 17:02:47 EDT
Going down my checklist again...

1. One rpmlint warning (W: crm114-emacs no-documentation), which was deemed
acceptable.
2. Package appears to meet Package Naming Guidelines.
3. Spec is named crm114.spec, check.
4. Package meets Packaging Guidelines, AFAICT.
5. Licensed under GPL, check.
6. License: GPL, check.
7. %doc contains GPL-License.txt, which I missed on my first pass.
8. Spec appears to be American English.
9. Spec seems legible.
10. md5sum on tarball matches upstream now (not sure what was up with that).
11. Compiles and builds on i386/ppc (my two supported build platforms).
12. x86_64 excluded, as per dependency on tre-devel.  You noted bug #202893, the
blocker.  Good.
13. Builds under Plague, so I imagine all of its dependencies are listed.
14. n/a, I think.
15. n/a (no shared libs)
16. n/a
17. You changed crm114-emacs' Req to emacs-el, resolving this issue.
18. No duplicate %files entries.
19. Defattr seems valid.
20. Has valid %clean section.
21. Macro use appears consistent.
22. Package contains code, not content.
23. Documentation makes up over 50% of the package's size, but that's still not
that much.
24. I don't see anything in %doc affecting runtime.
25. No header files or static libraries.
26. No .pc files.
27. No library files, much less ones with suffixes.
28. n/a (no -devel subpackage)
29. No .la files.
30. No GUI applications.
31. Doesn't own any directories owned by other packages (to the best of my
knowledge).
32. n/a, I overlooked GPL-License.txt
33. I'm not sure there are any description/summary translations available.
34. Package builds as i386 and ppc in Plague (and thus Mock).
35. Package won't build on x86_64 due to dependency's ExcludeArch: x86_64; other
architectures, yes.
36. I can't verify full functionality, but the binary doesn't segfault on i386/ppc.
37. No scriptlets.
38. The -emacs subpackage doesn't depend on the main package, ergo no listed Req.

Unless I screwed something up, it looks like crm114 is APPROVED.  Go forth and
import. :-)
Comment 12 Dominik 'Rathann' Mierzejewski 2006-08-17 19:08:50 EDT
Imported and built for devel. FC5 branch requested. Should I try and convince
FESCo to allow FC4 branch? tre managed to get in...
Comment 13 Dominik 'Rathann' Mierzejewski 2007-10-27 10:42:46 EDT
Package Change Request
======================
Package Name: foobar
New Branches: EL-5
Comment 14 Dominik 'Rathann' Mierzejewski 2007-10-27 10:43:53 EDT
Package Change Request
======================
Package Name: crm114
New Branches: EL-5

Of course, I meant this. :)

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