Bug 617592

Hi Chen Lei,

here are some quick comments:

- since the name of the upstream project and the tarball is 'accounts-qt', the package should get this name too

- concerning the patch, I suggest to replace  
  value = qlonglong(g_value_get_int64(&val)); with
  value = qint64(g_value_get_int64(&val));
  
  and
  
  value = qlonglong(g_value_get_uint64(&val)); with
  value = quint64(g_value_get_uint64(&val));

- add switch -f to all rm statements to ensure the files/dirs are actually removed

- the directory %{_libdir}/qt4/mkspecs/ is owned by qt-devel, so you should add Requires: qt-devel to the devel package

- in %description devel, replace "for the accounts-qt" with "for %{name}"

- the doxygen docs should go to %{_defaultdocdir}/%{name}-%{version}/

- please be more specific in %files devel, e.g.:
%{_libdir}/lib*.so
%{_includedir}/accounts-qt/
%{_libdir}/pkgconfig/accounts-qt.pc
%{_libdir}/qt4/mkspecs/*
...

(In reply to comment #1)
> Hi Chen Lei,
> 
> here are some quick comments:
> 
> - since the name of the upstream project and the tarball is 'accounts-qt', the
> package should get this name too
> 
I suggest to call it accounts-qt, because it's a meego-specfic package and also don't release any tarballs publicly. It'll be better to keep what upstream calls, also we also have libaccounts-glib in fedora.

Keeping the same name with upstream is helpful, I use it to compare the version of all meego related packages between Fedora Rawhide and Meego Trunk. 

> 
> - the doxygen docs should go to %{_defaultdocdir}/%{name}-%{version}/
> 

libaccounts-qt also provides .qch docs which is used by assitant. Install .qch files to version doc directories is unacceptable which will break bookmarks when updating libaccounts-qt. It may be better to simply delete html docs which provides the same contents with .qch files.

What's your opinion?

(In reply to comment #2)
> I suggest to call it accounts-qt
I think you meant libaccounts-qt here. 
In this special case and because of its companion package libaccounts-glib, naming the package libaccounts-qt is probably OK.


> libaccounts-qt also provides .qch docs which is used by assitant. Install .qch
> files to version doc directories is unacceptable which will break bookmarks
> when updating libaccounts-qt. It may be better to simply delete html docs which
> provides the same contents with .qch files.
> 
> What's your opinion?

I would tend to remove the qch docs and go with html only, as it's the more common format especially for API docs. I don't think most developers would prefer qch over html but might miss the latter. Splitting qch and html docs to different directories is also a bad idea. Thus, I would simply drop the qch file.

(In reply to comment #3)
> 
> > libaccounts-qt also provides .qch docs which is used by assitant. Install .qch
> > files to version doc directories is unacceptable which will break bookmarks
> > when updating libaccounts-qt. It may be better to simply delete html docs which
> > provides the same contents with .qch files.
> > 
> > What's your opinion?
> 
> I would tend to remove the qch docs and go with html only, as it's the more
> common format especially for API docs. I don't think most developers would
> prefer qch over html but might miss the latter. Splitting qch and html docs to
> different directories is also a bad idea. Thus, I would simply drop the qch
> file. 

Obviously, html docs is a much more common API docs format. But since qt 4.7, assistant_adp is dropped by Nokia. assitant-qt4 in qt-devel can only be used to open qch files. assistant/assistant_adp are far better tools than normal browser(e.g. firefox) to browse qt related api docs.

Also it seems qt-doc installs both qch and html api docs to un-versioned directory. There's some discussion on this topic several months ago on KDE-SIG meetings.

See http://www.mail-archive.com/devel@lists.fedoraproject.org/msg07695.html

OK, thanks for the info. Since the KDE-SIG approved to place KDE-/Qt-related .qch and html files in unversioned doc directories, it's probably OK to do the same with the API docs of libaccounts-qt. However, I suggest to move the API docs from %{_docdir}/accounts-qt to %{_docdir}/%{name}.

I have 0.31 ready to submit, but depends on new libaccounts-glib i submitted yesterday.
Chen, do you minf if i take the chain depends like libaccounts-glib, libaccounts-qt, qmf and qt-mobility ?

[]'s

(In reply to comment #6)
> I have 0.31 ready to submit, but depends on new libaccounts-glib i submitted
> yesterday.
> Chen, do you minf if i take the chain depends like libaccounts-glib,
> libaccounts-qt, qmf and qt-mobility ?
> []'s    

Feel to add yourself as comaintainer to all my packages listed in pkgdb[1]. For qmf, we should keep the name messagingframework. Fedora already have a package named qmf, try "yum info qmf", and meego 1.1 still uses messagingframework as rpm name.

If you can help to package meego packages, it'll be great.

It seems meego use a forked messagingframework, I wonder if we should use this forked version instead of the original version maintained by Nokia, what's your opinion?


[1]https://admin.fedoraproject.org/pkgdb/users/packages/supercyper

(In reply to comment #6)
> I have 0.31 ready to submit, but depends on new libaccounts-glib i submitted
> yesterday.
> Chen, do you minf if i take the chain depends like libaccounts-glib,
> libaccounts-qt, qmf and qt-mobility ?
> []'s    

Would mind to only upgrade the latest version to rawhide(Fedora 15)? I intend to package old version from meego 1.1 for F14. Also, I suggest you only update qt-mobility 1.1 tp for F15 temporarily.

From the meego release plan, it seems we should focus on packaging meego 1.1 packages for F14, and meego 1.2 ones for F15 in order to provide a complete meego MTF UX in Fedora.

Regarding the Harmattan/Meego x Upstream qmf, i already had some patches to upstream and exchanged a few mails with Nokia guys about this.
The position is not change current meego version and in near future pass to upstream, so we should go with upstream version.
Minor patches, like enable convenient headers, etc.

Regarding the F14, would not be wise having everyone using current Meego and get F14 launched with old version, so people start to bug everyone about backport from day 1 of F14 release.
So, i'm fine if you really want to keep old version, i just think that will be a little upsetting for users interested in use F14.

Btw, i work for Collabora in a Harmattan project for Nokia, so i have direct contact with all teams there and even sometimes go to Helsinki offices, so this helps when some info is needed

[]'s

(In reply to comment #9)
> Regarding the Harmattan/Meego x Upstream qmf, i already had some patches to
> upstream and exchanged a few mails with Nokia guys about this.
> The position is not change current meego version and in near future pass to
> upstream, so we should go with upstream version.
> Minor patches, like enable convenient headers, etc.

Currently, I packaged the meego-messagingframework[1], it seems nokia messagingframework[2] hasn't integrated with libaccounts yet, is it right? What's the meaning of "we should go with upstream version"?

[1]http://meego.gitorious.org/meego-middleware/messagingframework
[2]http://qt.gitorious.org/qt-labs/messagingframework



> Regarding the F14, would not be wise having everyone using current Meego and
> get F14 launched with old version, so people start to bug everyone about
> backport from day 1 of F14 release.
> So, i'm fine if you really want to keep old version, i just think that will be
> a little upsetting for users interested in use F14.

I don't oppose you to update particular meego packages to the lastest git tag if you are really familiar with the code of those packages(e.g. libaccounts), it'll greater if users can experience more new features in F14. But for the major packages in F14, we should track meego 1.1 src.rpm tightly, meego 1.1 isn't an old release, the release date of meego 1.1[1] is almost the same as F14's[2].

Old versions don't mean they have more bugs than newer release, especially for enhancement update. Meego has its own bug report system, if we use the same version as meego 1.1, actually we can forward most bugs to upstream instead of treating it by ourselves, I wonder it's impossible to fix bugs for dozens(>50) of packages without upstream support.  Also, it's very easier to track the daily change of meego 1.1's rpms, we can update those changed rpms weekly, many of those rpms already backport some patches from git repos and fix known bugs. 

[1]http://wiki.meego.com/Release_Engineering/Plans/1.1
[2]http://fedoraproject.org/wiki/Releases/14/Schedule

New upstream release
Several changes against comment #1

Links:
http://dl.dropbox.com/u/1338197/1/libaccounts-qt-0.31-1.fc13.src.rpm
http://dl.dropbox.com/u/1338197/1/libaccounts-qt.spec

The package is almost ready. There are just a few minor things left, you should address:

- Add a short comment above Patch0 telling what it does. Did you send the patch to the developer(s)? I think the changes should be applied upstream.

- Drop the word "the" in the description of the devel package (otherwise the sentence sounds strange to me), and preferably replace "accounts-qt" with "libaccounts-qt" (or %{name}).

- As suggested in comment #5, I recommend to move the API docs to %{_docdir}/libaccounts-qt. Using different "namespaces", i.e libaccounts-qt and accounts-qt, for the license file and the API docs is confusing. I'd expect the latter to be located in a folder starting with %{name}.

(In reply to comment #12)
> The package is almost ready. There are just a few minor things left, you should
> address:
> 
> - Add a short comment above Patch0 telling what it does. Did you send the patch
> to the developer(s)? I think the changes should be applied upstream.
> 
This patch is already sent to upstream, the function for patch is fix-64bit-compilation.patch(original libaccounts-qt can't build on 64bit arch)

> - Drop the word "the" in the description of the devel package (otherwise the
> sentence sounds strange to me), and preferably replace "accounts-qt" with
> "libaccounts-qt" (or %{name}).
> 
Fixed, http://dl.dropbox.com/u/1338197/1/libaccounts-qt-0.31-2.fc13.src.rpm

> - As suggested in comment #5, I recommend to move the API docs to
> %{_docdir}/libaccounts-qt. Using different "namespaces", i.e libaccounts-qt and
> accounts-qt, for the license file and the API docs is confusing. I'd expect the
> latter to be located in a folder starting with %{name}.    

I suggest to keep the doc directory name, because it's the default installation place for this package and this package also use accounts-qt namespace for header files and pkgconfig file. 

The directories for qt/kde documentation looks a bit strange for me also, maybe we should move those docs to another directory other than %{_docdir}.
e.g. gtk related packages install their apidocs to %{_datadir}/gtk-doc/html

(In reply to comment #13)
> This patch is already sent to upstream
OK, fine.

> the function for patch is fix-64bit-compilation.patch(original libaccounts-qt can't build on 64bit arch)

Yes, I know. :) But you should also add this information to the spec file as a short comment, e.g. something like this:
"This patch fixes a compilation error related to type ambiguities, and increases Accounts::AccountId to a 64bit integer to make the package build on 64bit archs." 


> I suggest to keep the doc directory name, because it's the default installation
> place for this package and this package also use accounts-qt namespace for
> header files and pkgconfig file. 

I tend to disagree here. Many library packages called libFOO place their include files in %{_includedir}/FOO and keep the docs in %{_docdir}/libFOO* (because they are added with %doc), even if "make install" put them somewhere else by default. Since nearly all docs go to a folder called like the package, I would keep this concept for libaccounts-qt as well. Otherwise, this inconsistency might confuse the user who expects the docs in %{_docdir}/%{name}*, especially as there's already such a folder containing the license.

 
> The directories for qt/kde documentation looks a bit strange for me also, maybe
> we should move those docs to another directory other than %{_docdir}.
> e.g. gtk related packages install their apidocs to %{_datadir}/gtk-doc/html    

That might be a good idea. But this should probably be discussed on the devel/packaging list(s) to get feedback from all involved packagers.

Fix doc path for now, though I think it's not necessary. libaccounts-qt actually use accounts-qt as a namespace for all files, but libaccounts-glib use libaccounts-glib as a namespace. Since those two libraries are maintained by the same upstream author, I don't know why.

New links:
http://dl.dropbox.com/u/1338197/1/libaccounts-qt-0.31-3.fc13.src.rpm

Sorry for having been a bit nit-picking. However, I think the package is more consistent now. Since docs are usually added with %doc, the Fedora guidelines define the namespace of the doc files to be %{name}. It doesn't matter where upstream wants to put them. If the package provided only html docs and no qch file, you would have to add them with %doc and they would therefore be placed in %{_docdir}/%{name}-%{version}. A single additional doc variant (qch) should not lead to a change of the namespace. But maybe you can ask the upstream developer whether he might adapt the naming schemes of libaccounts-glib and libaccounts-qt.

Here comes the formal review. The package looks fine and is ready now. 
You should update the referenced location of the upstream SRPM, though. Release 4 of the package is no longer available, but the tarball of release 5 equals that of your package too.

$ rpmlint libaccounts-qt-*.rpm
libaccounts-qt.x86_64: W: no-manual-page-for-binary account-tool
libaccounts-qt-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/accounts-qt-0.31/Accounts/.moc
libaccounts-qt-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/accounts-qt-0.31/Accounts/.moc
libaccounts-qt.src: W: no-cleaning-of-buildroot %clean
libaccounts-qt.src: W: no-buildroot-tag
libaccounts-qt.src: W: no-%clean-section
libaccounts-qt.src: W: invalid-url Source0: accounts-qt-0.31.tar.gz
4 packages and 0 specfiles checked; 0 errors, 7 warnings.

- the hidden dir is created by moc and is expected
- the missing %clean section and buildroot is OK as the package is 
  intended for F14+ only
All warnings can safely be ignored.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
[+] MUST: The License field in the package spec file must match the actual license.
    - LGPLv2 according to source file headers 

[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum accounts-qt-0.31.tar.gz /home/martin/rpmbuild/SOURCES/accounts-qt-0.31.tar.gz 
    d6429682ff3623fcf0ddd2023603b491  accounts-qt-0.31.tar.gz
    d6429682ff3623fcf0ddd2023603b491  accounts-qt-0.31.tar.gz.upstream

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[+] MUST: Packages storing shared library files must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[+] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[+] MUST: Library files that end in .so must go in a -devel package.
[+] MUST: devel packages must require the base package using a fully versioned dependency
[+] MUST: Packages must NOT contain any .la libtool archives
[.] MUST: Packages containing GUI applications must include a %{name}.desktop file
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: subpackages other than devel should require the base package using a fully versioned dependency.
[+] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[+] 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.

(In reply to comment #16)
> Sorry for having been a bit nit-picking. However, I think the package is more
> consistent now. Since docs are usually added with %doc, the Fedora guidelines
> define the namespace of the doc files to be %{name}. It doesn't matter where
> upstream wants to put them. If the package provided only html docs and no qch
> file, you would have to add them with %doc and they would therefore be placed
> in %{_docdir}/%{name}-%{version}. A single additional doc variant (qch) should
> not lead to a change of the namespace. But maybe you can ask the upstream
> developer whether he might adapt the naming schemes of libaccounts-glib and
> libaccounts-qt.
I never mind of changing the place of docs, I just consider the locatation of docs is a very trivial issue before Fedora package guideline has new changes :)

KDE-SIG seems want to talk docs issues again.

> Here comes the formal review. The package looks fine and is ready now. 
> You should update the referenced location of the upstream SRPM, though. Release
> 4 of the package is no longer available, but the tarball of release 5 equals
> that of your package too.

I'll add a permanent link to upstream SRPM before importing to git, the permanent link don't exist when I try to update this package to the latest upstream SRPM.

New Package SCM Request
=======================
Package Name: libaccounts-qt
Short Description: Accounts framework
Owners: supercyper heliocastro 
Branches: f14 
InitialCC:

Git done (by process-git-requests).