Bug 1265685

Summary: Review Request: sylfilter - A generic message filter library and command-line tools
Product: [Fedora] Fedora Reporter: Ranjan Maitra <itsme_410>
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: rawhideCC: dan, i, package-review, rc040203, zbyszek
Target Milestone: ---Flags: zbyszek: fedora-review+
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-04-02 15:55:51 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Ranjan Maitra 2015-09-23 13:21:44 UTC
This is a review request for sylfilter.

here are the files for the packages:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-0.1.fc22.src.rpm

SylFilter is a generic message filter library and command-line tools.
SylFilter provides a Bayesian filter which is very popular as a spam filtering
algorithm.  
SylFilter is also internationalized and can be applied to any languages.

SylFilter library provides simple but powerful C APIs and can be used from C
programs. 

SylFilter can be used as a command-line tool inside a junk filter mail program
similar to major tools such as bogofilter and bsfilter etc.

For further

Comment 1 Ranjan Maitra 2015-09-24 14:03:47 UTC
Updated spec and SRPM files are here:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-1.fc22.src.rpm

Comment 2 Michael Schwendt 2015-12-01 13:26:38 UTC
Not blocking bug 177841 means that the ticket has been visible only in the queue of NEW reviews: http://fedoraproject.org/PackageReviewStatus/NEW.html

That nobody has commented on it implies that nobody from the community is interested in it.

Comment 3 Ranjan Maitra 2015-12-01 15:13:33 UTC
Updated spec and SRPM files are here:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-1.fc23.src.rpm

Comment 4 Ranjan Maitra 2015-12-01 15:24:02 UTC
Fedora Account System Username: maitra

Comment 5 Ralf Corsepius 2015-12-01 15:26:49 UTC
Ranjan, your "rant" on users@ has provoked me enough to add some 
informal comments on this package (based on *0.8-1 from comment#1):

* *.spec is cluttered with rpm anachronisms, 
  which should not be used in new *.spec:
- Remove %clean
- Remove "rm -rf $RPM_BUILD_ROOT" from %install
- Remove BuildRoot
- Remove %defattr

* Packages should not be installed using %makeinstall, but be using DESTDIR, e.g. use %make_install instead.

* Package wastes resources on building a static library.
- Append --disable-static to %configure
- Remove "rm ..../*.a"

* AFAIS, this package seems to bundle a library (libsylpheed) which already is in Fedora (package sylpheed). You should be using the unbundled version.
(%configure --with-libsylph=sylpheed --with-libsylph-dir=/usr)

* COPYING is duplicated in %doc and %license. Remove the copy from %doc.

* The package should be split into a *-devel and non-devel subpackages.

(In reply to Ranjan Maitra from comment #3)
> Updated spec and SRPM files are here:
> 
> SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
> SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-1.fc23.src.rpm
Increment the %release each time you change something in your *spec. Not incrementing Release avoidably complicates reviews.

Comment 6 Ranjan Maitra 2015-12-01 15:58:33 UTC
Thanks! I tried this, but now I get errors, indeed lots of it. Can I send you personal e-mail?

Btw, my communication with Hiroyuki led me to believe that if I had sylpheed installed as a binary, then I should use --with-libsylph=builtin.

Comment 7 Ralf Corsepius 2015-12-01 16:09:21 UTC
(In reply to Ranjan Maitra from comment #6)
> Thanks! I tried this, but now I get errors, indeed lots of it. Can I send
> you personal e-mail?
I don't have much spare time left today, and can't promise, but I can try to do so, likely tomorrow.

> Btw, my communication with Hiroyuki led me to believe that if I had sylpheed
> installed as a binary, then I should use --with-libsylph=builtin.
No, unless sysfilter is crappily designed, there should not be any technical reason for bundling.

Comment 8 Ranjan Maitra 2015-12-01 16:18:25 UTC
Here is the updated spec and rpm file. They compile. They do not compile if I remove 

% clean

and if I replace 

--with-libsylph=sylpheed 

in place of 

--with-libsylph=builtin

Updated spec and SRPM files are here:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-.fc23.src.rpm

Not sure how to split a package into itself and a devel but is that important?

Comment 9 Ranjan Maitra 2015-12-01 16:31:12 UTC
Never mind. I was able to correct it some more. Updated files are now here. (I think that I was supposed to remmove the entire %clean section).

I now recall why I prefer builtin. This makes it independent of sylpheed. If it is not there, it can not be used. But someone may want to use sylfilter which is fast and lightweight using a different e-mailer. Indeed, I do so myself, usning procmail with sylfilter and also sylpheed with sylfilter, the first to do an initial processing and the second to classify mails that have not been correctly classified.  Any suggestions as to how to handle this?

Btw, the new files are here (sorry for the typo in the previous post):

Updated spec and SRPM files are here:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-2.fc23.src.rpm

Comment 10 Ralf Corsepius 2015-12-01 16:35:51 UTC
(In reply to Ranjan Maitra from comment #8)
> Not sure how to split a package into itself and a devel but is that
> important?
Yes. This is a MUSTFIX.

Somewhat oversimplyfied nutshell: 
* Move %includedir/*.h, %libdir/*.so and devel docs into *-devel
* Move %bindir/*, %libdir/*.so.* and user docs into <main>

Comment 11 Ralf Corsepius 2015-12-01 16:38:48 UTC
(In reply to Ranjan Maitra from comment #9)
> I now recall why I prefer builtin. This makes it independent of sylpheed.
That's short-sighted, but explaining this would be beyond the scope of a review.

> If
> it is not there, it can not be used.
Correct. Taking care about this is called "packaging".

Comment 12 Ranjan Maitra 2015-12-01 16:57:28 UTC
(In reply to Ralf Corsepius from comment #10)
> (In reply to Ranjan Maitra from comment #8)
> > Not sure how to split a package into itself and a devel but is that
> > important?
> Yes. This is a MUSTFIX.
> 
> Somewhat oversimplyfied nutshell: 
> * Move %includedir/*.h, %libdir/*.so and devel docs into *-devel
> * Move %bindir/*, %libdir/*.so.* and user docs into <main>

OK I will look into this. Still not sure about the value but will do.

(In reply to Ralf Corsepius from comment #11)
> (In reply to Ranjan Maitra from comment #9)
> > I now recall why I prefer builtin. This makes it independent of sylpheed.
> That's short-sighted, but explaining this would be beyond the scope of a
> review.
> 
> > If
> > it is not there, it can not be used.
> Correct. Taking care about this is called "packaging".

But then if sylpheed is not there, then it will be pulled in even if the user does not use the e-mailer, but only the filter. Is that a good move? It does not affect me personally because I use both sylpheed and sylfilter.

Comment 13 Michael Schwendt 2015-12-01 18:52:13 UTC
> But then if sylpheed is not there, then it will be pulled in even
> if the user does not use the e-mailer,

It would be perfectly imaginable to have the "sylpheed" src.rpm put libsylph files into separate "libsylph" and "libsylph-devel" subpackages. It would not be the first application package to split off a library like that.

Just because that hasn't been done so far is no reason not to do it as soon as the first libsylph API users appear in the package collection.


> Btw, my communication with Hiroyuki led me to believe that if
> I had sylpheed installed as a binary, then I should use
> --with-libsylph=builtin.

That sounds unusual. If libsylph is available as a shared lib plus API headers, why not reuse it then?

Comment 14 Upstream Release Monitoring 2015-12-02 04:02:57 UTC
maitra's scratch build of sylfilter-0.8-2.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12027082

Comment 15 Ralf Corsepius 2015-12-02 16:48:37 UTC
Any update?

Open issues with *-2:
* [MUSTFIX] Not building against external sylpheed
- Missing "BuildRequires: sylpheed-devel"
- Pass --with-libsylph=sylpheed instead of --with-libsylph=builtin
  to %configure

* [MUSTFIX] no *-devel subpackage

* [MUSTFIX] *.la are not removed.
Add
rm ${RPM_BUILD_ROOT}%{_libdir}/*.la
to %install

* The DESTDIR=... is redundant in 
"%make_install DESTDIR=$RPM_BUILD_ROOT"
"%make_install" already adds DESTDIR

* Useless define: %define  ver 0.8

Comment 16 Ranjan Maitra 2015-12-02 17:26:32 UTC
(In reply to Ralf Corsepius from comment #15)
> Any update?
> 
> Open issues with *-2:
> * [MUSTFIX] Not building against external sylpheed
> - Missing "BuildRequires: sylpheed-devel"
> - Pass --with-libsylph=sylpheed instead of --with-libsylph=builtin
>   to %configure

Why is this a MUSTFIX? Building against sylpheed will mean requiring the package for installation. It reduces the package size from 91k to 46k which is not much, however, requiring sylpheed for someone who does not intend to use sylpheed for this purpose would increase his/her pulled-in RPM size by 7.8M (which is what sylpheed's RPM is).

As I said, it does not affect me personally, but is this something we want to do?

Nevertheless, I have done what you have suggested. Not sure if it is a good idea.


> 
> * [MUSTFIX] no *-devel subpackage

Will have to fix this: first have to find how to do this.

> * [MUSTFIX] *.la are not removed.
> Add
> rm ${RPM_BUILD_ROOT}%{_libdir}/*.la
> to %install

done (after %make_install)

> * The DESTDIR=... is redundant in 
> "%make_install DESTDIR=$RPM_BUILD_ROOT"
> "%make_install" already adds DESTDIR
> 
> * Useless define: %define  ver 0.8

I don't understand this one.

New files uploaded:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-3.fc23.src.rpm

Comment 17 Ralf Corsepius 2015-12-02 18:05:04 UTC
(In reply to Ranjan Maitra from comment #16)
> (In reply to Ralf Corsepius from comment #15)
> > Any update?
> > 
> > Open issues with *-2:
> > * [MUSTFIX] Not building against external sylpheed
> > - Missing "BuildRequires: sylpheed-devel"
> > - Pass --with-libsylph=sylpheed instead of --with-libsylph=builtin
> >   to %configure
> 
> Why is this a MUSTFIX?
To put it bluntly: because bundling is harmful, stupid and dumb.

Less bluntly: bundling renders packages unmaintainable, vulnerable and causes bloat.

> Building against sylpheed will mean requiring the
> package for installation. It reduces the package size from 91k to 46k which
> is not much, however, requiring sylpheed for someone who does not intend to
> use sylpheed for this purpose would increase his/her pulled-in RPM size by
> 7.8M (which is what sylpheed's RPM is).
Size-wise bundling causes bloat because each statically linked packages re-adds the same libraries over and over again.

What causes the bloat in case of sylpheed is the libs* not having been split out into a separate package and them being packaged together with docs and binaries. If this is of real concern to you, you can consider to file an RFE and request a split out of the libs.

> > * [MUSTFIX] no *-devel subpackage
> 
> Will have to fix this: first have to find how to do this.

Coarse (Incomplete) outline:

<snip>
% package devel
Summary: Development files for sylfilter
Requires: sylfilter%{?_isa} = %{version}-%{release}

%description devel
Development files for sylfilter
[...]
%files
%doc README
%license COPYING
%{_bindir}/sylfilter
%{_libdir}/libsylfilter.so.*

%files devel
%{_libdir}/libsylfilter.so
%{_includedir}/sylfilter
</snip>

I hope tot be able have a look into *-3, tomorrow.

Comment 18 Ranjan Maitra 2015-12-12 00:34:51 UTC
I have made the changes suggested by splitting sylfilter and making a sylfilter-devel file. Please review and comment:


New files uploaded:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-4.fc23.src.rpm

Comment 19 Ranjan Maitra 2016-01-07 05:04:57 UTC
Raif, Any luck finding time to look at this? Thanks!

Comment 20 Christopher Meng 2016-01-07 06:27:59 UTC
(In reply to Ranjan Maitra from comment #16)
> > * Useless define: %define  ver 0.8
> 
> I don't understand this one.

Please don't make use of any useless macros, you can also define a macro 'nam' and value it like Name: %{nam} and so on.

However, these are just useless, keeping your spec simple but powerful is the key that sponsor will basically notice your work. Keep it in a whirl will make people think if you really know how to package. There is no luck for people don't understand suggestions, especially useful ones.

Writing specs is not programming, you don't need to reinvent any wheels to demonstrate that you are skilled (And from comments I don't think you are skilled).

------------------------------- My review -------------------------------

1. Summary:  A generic message filter library and command-line tools

I guess you are not an native English speaker, please correct the sentence. Hence please revise %description.

2. Requires: sylpheed

This is often superfluous. Generally, RPM will pick up it automatically.

3. %__make %_smp_mflags

Please keep in one style: %{XYZ} instead of %XYZ

Also try using %{make_build} if you want.

4. %changelog
* Fri Dec 11 2015 Ranjan Maitra <X> 0.8-4
- initial rebuild for Fedor (to include the devel file)
* Wed Dec 02 2015 Ranjan Maitra <X> 0.8-3
- initial rebuild for Fedor (to make dependent on sylpheed, as per  comments from Ralf Corsepius <Y>)
* Tue Dec 01 2015 Ranjan Maitra <X> 0.8-2
- initial rebuild for Fedora (to take care of comments from Ralf Corsepius <Y>)
* Tue Sep 22 2015 Ranjan Maitra <X> 0.8-1
- initial build for Fedora

i. Please do not invoke any other people's email address unless there is a strong reason you must do. For such simple anti spam trick, email address collector will easily crack it.

ii. Please leave a blank line per entry.

iii. Please FIX the language, like 'Fedor'.

5. Check deps in -devel package to see if any explicit requires are needed. (Like, you build package with sqlite-devel and glib2-devel and while installing sylfilter-devel, RPM won't pick up sqlite-devel or glib2-devel)

Comment 21 Ranjan Maitra 2016-01-07 10:31:44 UTC
(In reply to Christopher Meng from comment #20)
> (In reply to Ranjan Maitra from comment #16)
> > > * Useless define: %define  ver 0.8
> > 
> > I don't understand this one.
> 
> Please don't make use of any useless macros, you can also define a macro
> 'nam' and value it like Name: %{nam} and so on.

Done.

> Writing specs is not programming, you don't need to reinvent any wheels to
> demonstrate that you are skilled (And from comments I don't think you are
> skilled).

Not much fear of that.
 

> ------------------------------- My review -------------------------------
> 
> 1. Summary:  A generic message filter library and command-line tools
> 
> I guess you are not an native English speaker, please correct the sentence.
> Hence please revise %description.

I was using the sylfilter site's description earlier. Rephrased.

> 2. Requires: sylpheed
> 
> This is often superfluous. Generally, RPM will pick up it automatically.

Why so? If sylpheed is not installed, then sylfilter has to be built differently.

> 
> 3. %__make %_smp_mflags
> 
> Please keep in one style: %{XYZ} instead of %XYZ
> 
> Also try using %{make_build} if you want.

Done.
 
> 4. %changelog
> * Fri Dec 11 2015 Ranjan Maitra <X> 0.8-4
> - initial rebuild for Fedor (to include the devel file)
> * Wed Dec 02 2015 Ranjan Maitra <X> 0.8-3
> - initial rebuild for Fedor (to make dependent on sylpheed, as per  comments
> from Ralf Corsepius <Y>)
> * Tue Dec 01 2015 Ranjan Maitra <X> 0.8-2
> - initial rebuild for Fedora (to take care of comments from Ralf Corsepius
> <Y>)
> * Tue Sep 22 2015 Ranjan Maitra <X> 0.8-1
> - initial build for Fedora
> 
> i. Please do not invoke any other people's email address unless there is a
> strong reason you must do. For such simple anti spam trick, email address
> collector will easily crack it.

Done. Removed all e-mail addresses.

> ii. Please leave a blank line per entry.

Done.
 
> iii. Please FIX the language, like 'Fedor'.

Done! And rpmlint did not picj this up?

> 5. Check deps in -devel package to see if any explicit requires are needed.
> (Like, you build package with sqlite-devel and glib2-devel and while
> installing sylfilter-devel, RPM won't pick up sqlite-devel or glib2-devel)

Yes, fixed.

New files posted at:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-5.fc23.src.rpm

Comment 22 Dan Horák 2016-01-07 19:43:31 UTC
> > 2. Requires: sylpheed
> > 
> > This is often superfluous. Generally, RPM will pick up it automatically.
> 
> Why so? If sylpheed is not installed, then sylfilter has to be built
> differently.

no, rpmbuild adds automatically dependencies on shared libraries used in the resulting rpm. So there will be always a dependency on libsylph no matter in which package or rpm libsylph will live.

If sylfilter is useful without the sylpheed GUI client, we can move the libsylph library into own subpackage.

Comment 23 Ranjan Maitra 2016-01-07 21:29:45 UTC
(In reply to Dan Horák from comment #22)
> > > 2. Requires: sylpheed
> > > 
> > > This is often superfluous. Generally, RPM will pick up it automatically.
> > 
> > Why so? If sylpheed is not installed, then sylfilter has to be built
> > differently.
> 
> no, rpmbuild adds automatically dependencies on shared libraries used in the
> resulting rpm. So there will be always a dependency on libsylph no matter in
> which package or rpm libsylph will live.
> 
> If sylfilter is useful without the sylpheed GUI client, we can move the
> libsylph library into own subpackage.

Sylfilter is useful without the sylpheed GUI client, but does this not mean that sylpheed will have to be rebuilt and be disruptive to an existing mature package (the mailer?).

Btw, someone is trying to package libsylph separately, FWIW:
https://bugzilla.redhat.com/show_bug.cgi?id=1288927


New files removing requirement of sylpheed posted at:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-6.fc23.src.rpm

Comment 24 Dan Horák 2016-01-07 21:59:57 UTC
(In reply to Ranjan Maitra from comment #23)
> (In reply to Dan Horák from comment #22)
> > > > 2. Requires: sylpheed
> > > > 
> > > > This is often superfluous. Generally, RPM will pick up it automatically.
> > > 
> > > Why so? If sylpheed is not installed, then sylfilter has to be built
> > > differently.
> > 
> > no, rpmbuild adds automatically dependencies on shared libraries used in the
> > resulting rpm. So there will be always a dependency on libsylph no matter in
> > which package or rpm libsylph will live.
> > 
> > If sylfilter is useful without the sylpheed GUI client, we can move the
> > libsylph library into own subpackage.
> 
> Sylfilter is useful without the sylpheed GUI client, but does this not mean
> that sylpheed will have to be rebuilt and be disruptive to an existing
> mature package (the mailer?).

There is nothing disruptive on introducing a new subpackage in an exiting package. The library file is already there, it will just move to new rpm.

> Btw, someone is trying to package libsylph separately, FWIW:
> https://bugzilla.redhat.com/show_bug.cgi?id=1288927

I don't understand why someone wants to introduce 8 years old code when an up-to-date version is already available in the distro ...
 
> 
> New files removing requirement of sylpheed posted at:
> 
> 
> SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
> SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-6.fc23.src.rpm

Comment 25 Ranjan Maitra 2016-01-07 22:19:51 UTC
(In reply to Dan Horák from comment #24)
> (In reply to Ranjan Maitra from comment #23)
> > (In reply to Dan Horák from comment #22)
> > > > > 2. Requires: sylpheed
> > > > > 
> > > > > This is often superfluous. Generally, RPM will pick up it automatically.
> > > > 
> > > > Why so? If sylpheed is not installed, then sylfilter has to be built
> > > > differently.
> > > 
> > > no, rpmbuild adds automatically dependencies on shared libraries used in the
> > > resulting rpm. So there will be always a dependency on libsylph no matter in
> > > which package or rpm libsylph will live.
> > > 
> > > If sylfilter is useful without the sylpheed GUI client, we can move the
> > > libsylph library into own subpackage.
> > 
> > Sylfilter is useful without the sylpheed GUI client, but does this not mean
> > that sylpheed will have to be rebuilt and be disruptive to an existing
> > mature package (the mailer?).
> 
> There is nothing disruptive on introducing a new subpackage in an exiting
> package. The library file is already there, it will just move to new rpm.

OK, but I guess then I am stuck till this happens. I will containing using my local rpm which has been serving me well for the past 5 years.

> 
> > Btw, someone is trying to package libsylph separately, FWIW:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1288927
> 
> I don't understand why someone wants to introduce 8 years old code when an
> up-to-date version is already available in the distro ...

I agree and have not quite understood the rationale.

>  
> > 
> > New files removing requirement of sylpheed posted at:
> > 
> > 
> > SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
> > SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-6.fc23.src.rpm

Comment 26 Zbigniew Jędrzejewski-Szmek 2016-01-08 20:42:10 UTC
(In reply to Ralf Corsepius from comment #17)
> (In reply to Ranjan Maitra from comment #16)
> > (In reply to Ralf Corsepius from comment #15)
> > > Open issues with *-2:
> > > * [MUSTFIX] Not building against external sylpheed
> > > - Missing "BuildRequires: sylpheed-devel"
> > > - Pass --with-libsylph=sylpheed instead of --with-libsylph=builtin
> > >   to %configure
> > 
> > Why is this a MUSTFIX?
> To put it bluntly: because bundling is harmful, stupid and dumb.
> 
> Less bluntly: bundling renders packages unmaintainable, vulnerable and
> causes bloat.

To expand a bit on this, because Ranjan is a new packager, and is most likely
not aware of the long history of this topic ;):

Bundling used to be totally forbidden, with exceptions reluctantly granted
by the Fedora Packaging Committee. This policy was recently relaxed [1,2],
but bundling is still best avoided [3]. When the bundled library is already
packaged, it is required to use the system-wide copy:
"All packages whose upstreams allow them to be built against system libraries
must be built against system libraries."

The old policy [4] explains why bundling is bad. Please note that those
considerations are still valid, and the policy was only relaxed because
people were tired of fighting with unreasonable upstreams and massive
amounts of bundled code in some projects.

[1] https://fedorahosted.org/fesco/ticket/1483
[2] https://fedorahosted.org/fesco/ticket/1491
[3] https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries
[4] https://fedoraproject.org/w/index.php?title=Packaging:No_Bundled_Libraries&oldid=406058

> > Building against sylpheed will mean requiring the
> > package for installation. It reduces the package size from 91k to 46k which
> > is not much, however, requiring sylpheed for someone who does not intend to
> > use sylpheed for this purpose would increase his/her pulled-in RPM size by
> > 7.8M (which is what sylpheed's RPM is).
> Size-wise bundling causes bloat because each statically linked packages
> re-adds the same libraries over and over again.

With today's disks, extra 8MB in files on disk is really unimportant.
Even if sylpheed is installed, nothing is run by default, so apart from
a bit of disk space this doesn't cause any problems. So even if the libs*
are *not* split out, it would be totally fine for sylfilter package to pull
in sylpheed.

Of course it is nice to split out libs* and avoid this bit
of bloat, but it's not that much of an issue, and no reason to hold up
the review. Currently sylpheed.rpm provides libsylph-0.so.1()(64bit),
and rpm will automatically generate a dependency on this in sylfilter.rpm.
If/when libs* are split out, this dependency will be satisfied by the
libs* subpackage. That's all.

Comment 27 Dan Horák 2016-01-10 11:10:47 UTC
The sylpheed-libs package has been introduced in sylpheed-3.4.3-3.fc24

Comment 28 Ranjan Maitra 2016-03-21 14:06:47 UTC
Any news on this review? Thanks!

Comment 29 Zbigniew Jędrzejewski-Szmek 2016-03-22 02:57:32 UTC
rpmlint is quite helpful:
sylfilter.i686: W: devel-file-in-non-devel-package /usr/lib/libsylfilter.so

.so is packaged in both subpackages. It should be present only in the -devel supbackage. Common trick is to %{_libdir}/libsylfilter.so.* in the main subpackage.

sylfilter-devel.i686: W: tag-in-description C Requires:
sylfilter-devel.i686: W: tag-in-description C Requires:
You should move the Requires tag above %description because they're being interpreted as the part of the text.

/usr/lib/libsylfilter.la should be removed after installation
[https://fedoraproject.org/wiki/Packaging:Guidelines#StaticLibraries].

I looked over all the other comments, and it seems everything that other reviews noted was fixed. It looks very clean now.

Comment 30 Ranjan Maitra 2016-03-22 10:03:09 UTC
Thank you for the detailed comments and suggested fixes - they have been incorporated. Here are the results from rpmlint:

$ rpmlint  sylfilter-0.8-7.fc23.src.rpm 
sylfilter.src: W: spelling-error %description -l en_US bogofilter -> bogometer
sylfilter.src: W: spelling-error %description -l en_US bsfilter -> filterer, filter
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


New files posted:

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-7.fc22.src.rpm

Please let me know if additional corrections and information are needed.

Comment 31 Ranjan Maitra 2016-03-22 10:04:30 UTC
Corrected and reposted links (sorry):

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-7.fc23.src.rpm

Comment 32 Zbigniew Jędrzejewski-Szmek 2016-03-22 13:00:10 UTC
? I don't see fixes for the things in comment #29 in the latest spec file.

Comment 33 Ranjan Maitra 2016-03-22 13:13:00 UTC
Perhaps I misunderstood your comments, but here are the changes that were made in response to #29.

> .so is packaged in both subpackages. It should be present only in the -devel supbackage. Common trick is to %{_libdir}/libsylfilter.so.* in the main subpackage.

In the spec file: I did (see in particular the fifth line from here):
....
%files
%doc README
%{_bindir}/sylfilter
%{_libdir}/libsylfilter.*
%{_libdir}/libsylfilter.so.*
%license COPYING

%files devel
%{_libdir}/libsylfilter.so
%{_includedir}/sylfilter

> You should move the Requires tag above %description because they're being interpreted as the part of the text.

My spec file now goes like this:

%package devel
Summary: Development files for sylfilter
Requires: sylfilter%{?_isa} = %{version}-%{release}

%description devel
Development files for sylfilter
Requires: sqlite-devel
Requires: glib2-devel

%description
This is SylFilter, a generic message filter library, and some command-line tools
that provide a Bayesian filter which is very popular as a spam filtering
algorithm.


Is this not what was suggested? Many thanks!

Comment 34 Ranjan Maitra 2016-03-22 13:19:54 UTC
Sorry,  I think I found my error: new files uploaded 

SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-8.fc23.src.rpm

Comment 35 Zbigniew Jędrzejewski-Szmek 2016-03-22 13:30:00 UTC
What about the .la file?

One more thing:
the %changelog is nearly useless.

*Tue Mar 22 2016 Ranjan Maitra 0.8-7
- initial rebuild for Fedora addresses comments in BZ 1265685
→ this tell me nothing. It should say "Fix %%description" or whatever.

* Thu Jan 7 2016 Ranjan Maitra 0.8-6
- initial rebuild for Fedora (removing requirement of sylpheed)
→ this should just say: "Remove requirement on sylpheed"

* Thu Jan 7 2016 Ranjan Maitra 0.8-5
- initial rebuild for Fedora (to fix stylistic suggestions per Christopher Meng
→ this should just say: "Fix style issues"
(or "Implement stylistic suggestions", if we want to be gramatically correct)

Comment 36 Ranjan Maitra 2016-03-22 16:08:15 UTC
Sorry, I will work on a more descriptive %changelog spec file.

How does one remove the .la file?

Comment 37 Ranjan Maitra 2016-03-22 16:09:01 UTC
Sorry, I will work on a more descriptive %changelog spec file.

How does one remove the .la file after installation? The webpage does not seem to me to be clear. It only says remove after installation.

Comment 38 Zbigniew Jędrzejewski-Szmek 2016-03-25 17:00:57 UTC
(In reply to Ranjan Maitra from comment #37)
> How does one remove the .la file after installation? The webpage does not
> seem to me to be clear. It only says remove after installation.

At the end of the %install section add:
rm %{buildroot}%{_libdir}/*.la
(adjust path as necessary)

Comment 39 Ranjan Maitra 2016-03-25 17:36:59 UTC
Thanks! It appears to work. New files posted:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-9.fc23.src.rpm

However, I seem to be getting a warning in my rpmlint now and I can't figure out where this third warning came from. Very perplexing!

$ rpmlint sylfilter-0.8-9.fc23.src.rpm 
sylfilter.src: W: spelling-error %description -l en_US bogofilter -> bogometer
sylfilter.src: W: spelling-error %description -l en_US bsfilter -> filterer, filter
sylfilter.src:70: W: macro-in-%changelog %description
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 40 Zbigniew Jędrzejewski-Szmek 2016-03-25 18:03:45 UTC
In comments and changelog percent signs must be escaped by doubling:
- removes %description to after Requires
→
- move %%description after Requires

%files
...
%{_libdir}/libsylfilter.*
%{_libdir}/libsylfilter.so.*

Everything which is matched by the second glob is also matched by the first glob. You only need the second line.

And as discussed before: there's no need to say "addresses comments of Zbigniew Jędrzejewski-Szmek". I understand that the intent is to give due credit, but we try to keep changelogs short. You can just remove those lines.

Package is APPROVED. Please fix up those minor issues in the initial upload.

Comment 41 Ranjan Maitra 2016-03-26 01:57:52 UTC
New files uploaded for the record:


SPEC: http://maitra.public.iastate.edu/Fedora/sylfilter.spec
SRPM: http://maitra.public.iastate.edu/Fedora/sylfilter-0.8-10.fc23.src.rpm


Will proceed with package approval.

Comment 42 Gwyn Ciesla 2016-03-27 04:28:42 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/sylfilter

Comment 43 Fedora Update System 2016-03-27 22:32:09 UTC
sylfilter-0.8-10.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-3e49eba720

Comment 44 Fedora Update System 2016-03-27 22:57:11 UTC
sylfilter-0.8-10.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-70cf974005

Comment 45 Fedora Update System 2016-03-27 23:00:43 UTC
sylfilter-0.8-10.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8c3ce917d1

Comment 46 Fedora Update System 2016-03-27 23:02:49 UTC
sylfilter-0.8-10.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-18c55e72fc

Comment 47 Fedora Update System 2016-03-28 20:19:20 UTC
sylfilter-0.8-10.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-70cf974005

Comment 48 Fedora Update System 2016-03-28 20:49:27 UTC
sylfilter-0.8-10.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-18c55e72fc

Comment 49 Fedora Update System 2016-03-28 20:49:31 UTC
sylfilter-0.8-10.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-3e49eba720

Comment 50 Fedora Update System 2016-03-28 20:54:58 UTC
sylfilter-0.8-10.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-8c3ce917d1

Comment 51 Fedora Update System 2016-04-02 15:55:48 UTC
sylfilter-0.8-10.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 52 Fedora Update System 2016-04-05 13:52:57 UTC
sylfilter-0.8-10.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 53 Fedora Update System 2016-04-05 14:17:24 UTC
sylfilter-0.8-10.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.

Comment 54 Fedora Update System 2016-04-05 16:20:19 UTC
sylfilter-0.8-10.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 55 Fedora Update System 2016-04-13 05:55:33 UTC
sylfilter-0.8-10.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.