Bug 504261 (mailody)

yay, glad to see someone work on this... will help out review-wise, when I can (will be busy over the next week though...)

Some initial remarks:

o it don't work:
  Settings -> Configure Mailody -> Servers -> IMAP servers tab
   only gives: 

  The module could not be found

  The diagnostics is:
  The desktop file could not be found.

Missing buildreq on some imap libs or missing req on some kde imap libs?

o Missing desktop install etc, please read: 
  https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

o Why don't simplify

%dir %{_datadir}/kde4/apps/mailody
%{_datadir}/kde4/apps/mailody/*

to 

%{_datadir}/kde4/apps/mailody

o Should the top %define be switched to %global?

 -

koji build for rawhide is good:
 
 http://koji.fedoraproject.org/koji/taskinfo?taskID=1397272

Spec URL: http://red.fedorapeople.org/SRPMS/mailody.spec
SRPM URL:
http://red.fedorapeople.org/SRPMS/mailody-1.5.0-0.2.alfa1.fc11.src.rpm


Terje: Thanks for the comments!

Well, there's probably some Requires missing as of now since I really don't have an idea about them. But one should have been clear to me and I still missed it :/ kdepimlibs-akonadi now is on the Requires, I hope that fixes your problem.

Yes, I forgot about the desktop file install...or the validation of the installed file to be precise. I already fixed that here, but didn't upload that version until now :) Note: the desktop file is correctly installed automatically, I only validate it.

I also made the other changes you suggested.

[OK] MUST: rpmlint must be run on every package. The output should be posted in
the review.

% rpmlint mailody-*.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

[OK] MUST: The spec file name must match the base package %{name}, in the
format %{name}.spec unless your package has an exemption.
[OK] MUST: The License field in the package spec file must match the actual
license.
[OK] MUST: The package must meet the  Packaging Guidelines . 
[OK] MUST: The package must be licensed with a Fedora approved license and meet
the  Licensing Guidelines . 
[OK] MUST: The License field in the package spec file must match the actual
license.
[OK] MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
[OK] MUST: The spec file must be written in American English.
[OK] MUST: The spec file for the package MUST be legible.
[OK] MUST: The package <b>MUST</b> successfully compile and build into binary
rpms on at least one primary architecture.
[OK] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line.
[OK] MUST: The spec file MUST handle locales properly. This is done by using
the <code>%find_lang</code> macro. Using <code>%{_datadir}/locale/*</code> is
strictly forbidden.
[OK] MUST: Every binary RPM package (or subpackage) which stores shared library
files (not just symlinks) in any of the dynamic linker's default paths, must
call ldconfig in <code>%post</code> and <code>%postun</code>.
[OK] MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.
[OK] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[OK] MUST: A Fedora package must not list a file more than once in the spec
file's&nbsp;%files listings.
[OK] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every <code>%files</code> section
must include a <code>%defattr(...)</code> line.
[OK] MUST: Each package must have a&nbsp;%clean section, which contains
<code>rm -rf&nbsp;%{buildroot}</code> (<a
href="/wiki/Packaging/Guidelines#UsingBuildRootOptFlags"
title="Packaging/Guidelines" class="mw-redirect">or $RPM_BUILD_ROOT</a>).
[OK] MUST: Each package must consistently use macros.
[OK] MUST: The package must contain code, or permissable content.
[OK] MUST: Large documentation files must go in a -doc subpackage. (The
definition of large is left up to the packager's best judgement, but is not
restricted to size. Large can refer to either size or quantity).
[OK] MUST: If a package includes something as&nbsp;%doc, it must not affect the
runtime of the application. To summarize: If it is in&nbsp;%doc, the program
must run properly if it is not present.
[OK] MUST: Header files must be in a -devel package.
[OK] MUST: Static libraries must be in a -static package.
[OK] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
[OK] MUST: If a package contains library files with a suffix
(e.g.libfoo.so.1.1), then library files that end in .so (without suffix) must
go in a -devel package.
[OK] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: <code>Requires:&nbsp;%{name}
=&nbsp;%{version}-%{release} </code>
[OK] MUST: Packages must NOT contain any .la libtool archives, these must be
removed in the spec if they are built.
[OK] MUST: Packages containing GUI applications must include
a&nbsp;%{name}.desktop file, and that file must be properly installed with
desktop-file-install in the&nbsp;%install section. If you feel that your
packaged GUI application does not need a .desktop file, you must put a comment
in the spec file with your explanation.
[OK] MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed
should own the files or directories that other packages may rely upon. This
means, for example, that no package in Fedora should ever share ownership with
any of the files or directories owned by the <code>filesystem</code> or
<code>man</code> package. If you feel that you have a good reason to own a file
or directory that another package owns, then please present that at package
review time.
[OK] MUST: At the beginning of <code>%install</code>, each package MUST run
<code>rm -rf&nbsp;%{buildroot}</code> (<a
href="/wiki/Packaging/Guidelines#UsingBuildRootOptFlags"
title="Packaging/Guidelines" class="mw-redirect">or $RPM_BUILD_ROOT</a>).
[OK] MUST: All filenames in rpm packages must be valid UTF-8.
[OK] SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.
[OK] SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
[OK] SHOULD: The reviewer should test that the package builds in mock.
[OK] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[??] SHOULD: The reviewer should test that the package functions as described.
A package should not segfault instead of running, for example.

It didn't import my KMail settings for IMAP, though it did grab them for SMTP and the identities. I couldn't get it to load up one of my IMAP addresses, but gmail partially worked. It crashed when trying to view a mail (something with a slider).

I would recommend either downgrading to a stable version or keep in rawhide until a stable release.

[OK] SHOULD: If scriptlets are used, those scriptlets must be sane. This is
vague, and left up to the reviewers judgement to determine sanity.
[OK] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[OK] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase,
and this is usually for development purposes, so should be placed in a -devel
pkg.  A reasonable exception is that the main pkg itself is a devel tool not
installed in a user runtime, e.g. gcc or gdb.
[OK] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.

There's a few nitpicks about the spec file as it stands. Not review blockers, but I'd fix them before CVS import (some KDE4 packaging-isms):

- In %build please use:
mkdir %{_target_platform}
pushd %{_target_platform}
%{cmake_kde4} ..
popd
make %{?_smp_flags} VERBOSE=1 -C %{_target_platform}

and for %install:

make install DESTDIR=$RPM_BUILD_ROOT -C %{_target_platform}

- Personally, I'd break the BR's apart into one per line for readability, but that's up to you ultimately
- In files, please use:
-- %{_bindir} -> %{_kde4_bindir}
-- %doc /usr/share/doc/HTML/en/doc/ -> %{_kde4_docdir}/HTML/en/doc/
-- %{_datadir} -> %{_kde4_datadir}
-- %{_datadir}/icons -> %{_kde4_iconsdir}
-- Trail the "%{_datadir}/kde4/apps/mailody" with a slash so it's obvious that it's a directory

Other than that, looks good. I'll approve once you have a new spec file here (no need for a new SRPM/build) and some way to handle the issues raised above.

Spec URL: http://red.fedorapeople.org/SRPMS/mailody.spec
SRPM URL:
http://red.fedorapeople.org/SRPMS/mailody-1.5.0-0.3.alfa1.fc11.src.rpm


Thanks for the review, Ben!

I made the changes to the KDE4 packaging-isms :) Thanks for pointing them out so clearly. I also put one BR per line instead of all on the same line (I actually normally do this, not sure why I didn't do it this time). And changed the %files stuff.

I will not go back to the latest stable since that would mean KDE3 instead of KDE4 ;) But I'll keep it in rawhide until there's a version that works better.

Okay, looks good. I'd also recommend BR on kdelibs4-devel instead of kdelibs-devel (won't need changing with KDE5).

If you need testers for the next version, post something to the fedora-kde ML (<fedora-kde.org>) with "TESTING" in the subject and directions on what/how to test and the testing team should be able to help.

Great, thanks!

New Package CVS Request
=======================
Package Name: mailody
Short Description: Simple KDE-based IMAP mail client
Owners: red
Branches: F-10 F-11
InitialCC:

Sorry, for chiming in late, but my standard pet-peave applies... please remove "KDE-based" from pkg description.

Okay, spoke a bit about the summary with rdieter on IRC. The conclusion seems to be that 'simple kde-based imap mail client' is a terrible summary since it could become a 'complex whatever-based groupware suite' long term-ish ;) But I guess it needs some sort of summary (that says a bit more than 'piece of software) so I decided to go for 'simple mail client' as I believe it will always keep a simple application and mail will always be the most important feature.

New Package CVS Request
=======================
Package Name: mailody
Short Description: Simple mail client
Owners: red
Branches: F-10 F-11
InitialCC:

CVS done.

Juha, would you please stop doing that?

(In reply to comment #12)
> Juha, would you please stop doing that?  

I wouldn't be doing it if you would fill the entries in the first place so that people would not need to dig information from spec files.

I don't know who you're addressing as "you" (because it doesn't make sense to direct that comment at me), but as the person who does most of the triage for package review tickets, all I can say is that adding URLs and bug aliases is not really helping, especially for reviews that are already finished.  If you'd like to work on modifying the package submission guidelines to recommend setting the URL field and bug alias fields then feel free.

Sorry but i fail to see the damage... except lost energy on these comments?

Is this built for Rawhide? If so, this bug can be closed.

Yep, it's been in rawhide for a while.

Can this be built for F10 and F11?

Ben,

As you can read above, we decided not to push this to F10/F11 as the current version is not stable enough. As soon as there's a better version (i.e. a gold release), I'll push it to testing/stable repos.

Ah right. Was looking at it to try it out and saw it was Rawhide only. Forgot the pre-release status of it.