Bug 560179

Summary: accountsdialog - An application to view and modify user accounts information
Product: [Fedora] Fedora Reporter: Matthias Clasen <mclasen>
Component: Package ReviewAssignee: Christoph Wickert <cwickert>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: cwickert, fedora-package-review, notting, pahan
Target Milestone: ---Flags: cwickert: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-02-03 11:51:36 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 560178    
Bug Blocks:    

Description Matthias Clasen 2010-01-29 23:11:51 EST
spec: http://mclasen.fedorapeople.org/accounts/accountsdialog.spec
srpm: http://mclasen.fedorapeople.org/accounts/accountsdialog-0.4-1.fc12.src.rpm

notes: 

1) the dialog depends on a dbus service under review here: bug 560178

2) the dialog depends on the git version of cheese for the webcam functionality. 

3) a gdm patch to get the user information from the service is currently under development
Comment 1 Christoph Wickert 2010-01-30 19:14:31 EST
OK - MUST: rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/accountsdialog-*
accountsdialog.src: W: no-buildroot-tag
3 packages and 0 specfiles checked; 0 errors, 1 warnings.
OK - MUST: named according to the Package Naming Guidelines
OK - MUST: spec file name matches the base package %{name}
OK - MUST: package meets the Packaging Guidelines
OK - MUST: Fedora approved license and meets the Licensing Guidelines: GPLv3+
OK - MUST: License field in spec file matches the actual license
OK - MUST: license file included in %doc
OK - MUST: spec is in American English
OK - MUST: spec is legible
OK - MUST: sources match the upstream source by MD5 c3f1931123adcf0f0dc749edb60a7d60
OK - MUST: successfully compiles and builds into binary rpms on x86_64
OK - MUST: no ExcludeArch.
OK - MUST: all build dependencies are listed in BuildRequires.
OK - MUST: handles locales properly with %find_lang
N/A - 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 %post and %postun.
?? - MUST: Package does not bundle copies of system libraries (see below).
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review.
OK - MUST: owns all directories that it creates
OK - MUST: no duplicate files in the %files listing
OK - MUST: Permissions on files are set properly, includes %defattr(...)
OK - MUST: package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
OK - MUST: consistently uses macros
OK - MUST: package contains code, or permissable content
N/A - MUST: Large documentation files should go in a -doc subpackage
OK - MUST: Files included as %doc do not affect the runtime of the application
N/A - MUST: Header files must be in a -devel package
N/A - MUST: Static libraries must be in a -static package
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix, then library files that end in .so must go in a -devel package.
N/A - MUST: devel packages must require the base package using a fully versioned dependency
OK - MUST: The package does not contain any .la libtool archives.
FIX - MUST: The package contains a GUI application and includes a %{name}.desktop file, but that file is properly not installed with desktop-file-install in the %install section.
OK - MUST: package does not own files or directories already owned by other packages.
OK - MUST: at the beginning of %install, the package runs rm -rf RPM_BUILD_ROOT.
OK - MUST: all filenames valid UTF-8


SHOULD Items:
OK - SHOULD: Source package includes license text(s) as a separate file.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: builds in mock.
OK - SHOULD: compiles and builds into binary rpms on all supported architectures.
OK - SHOULD: functions as described.
N/A - SHOULD: Scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg
N/A - 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.


Other items:
OK - latest stable version
OK - SourceURL valid
OK - Compiler flags ok
OK - Debuginfo complete


Issues:
- The package doesn't build against the Polkit in F12, so please add the minimim required version to the polkit-devel BuildRequires.
- You are not using desktop-file-install or desktop-file-validate for accountsdialog.desktop
- The Exec= line in the desktop file should not have an absolute path.
- Add INSTALL='install -p' to make install to preserve the timestamps of the data files (pixmaps etc).


Notes:
- The source code is a mixture of GPLv2+ and GPLv3, src/locarchive.h is LGPLv2+. Can this be cleaned up?
- There are files from fprint and GDM included in the source, which violates the "No duplication of system libraries" policy. I assume this code will be moved into it's proper upstream packages once it's mature and you will build against these packages then, so for now this is not a problem.


Please fix the items marked as issues and consider the package APPROVED.
Comment 2 Matthias Clasen 2010-01-31 13:42:49 EST
> - The package doesn't build against the Polkit in F12, so please add the
> minimim required version to the polkit-devel BuildRequires.

Hmm, I just tried, and it build fine in mock here. What failure are you seeing ?

> - You are not using desktop-file-install or desktop-file-validate for
> accountsdialog.desktop

I'll add that.

> - The Exec= line in the desktop file should not have an absolute path.

Not sure there is any rule about this, and I don't think it makes a difference either way. But I've remove the path in git, will be in the next release.

> - Add INSTALL='install -p' to make install to preserve the timestamps of the
> data files (pixmaps etc).

I don't think it makes any difference whatsoever, but I'll add it to make you happy.


> - The source code is a mixture of GPLv2+ and GPLv3, src/locarchive.h is
> LGPLv2+. Can this be cleaned up?

No. These are files that are copied from elsewhere.

> - There are files from fprint and GDM included in the source, which violates
> the "No duplication of system libraries" policy. I assume this code will be
> moved into it's proper upstream packages once it's mature and you will build
> against these packages then, so for now this is not a problem.

Not sure where you are going with this, but copying and adapting sources from elsewhere is quite normal, and the system library rule doesn't even come into play since we are not building any libraries here at all.
Comment 3 Christoph Wickert 2010-01-31 17:26:34 EST
(In reply to comment #2)
> > - The package doesn't build against the Polkit in F12, so please add the
> > minimim required version to the polkit-devel BuildRequires.
> 
> Hmm, I just tried, and it build fine in mock here. What failure are you seeing
> ?

http://koji.fedoraproject.org/koji/taskinfo?taskID=1955235

> > - The Exec= line in the desktop file should not have an absolute path.
> 
> Not sure there is any rule about this, and I don't think it makes a difference
> either way. But I've remove the path in git, will be in the next release.

Fine with me.

> > - Add INSTALL='install -p' to make install to preserve the timestamps of the
> > data files (pixmaps etc).
> 
> I don't think it makes any difference whatsoever, but I'll add it to make you
> happy.

It's not to make me happy but to follow the guidelines:
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

> > - There are files from fprint and GDM included in the source, which violates
> > the "No duplication of system libraries" policy. I assume this code will be
> > moved into it's proper upstream packages once it's mature and you will build
> > against these packages then, so for now this is not a problem.
> 
> Not sure where you are going with this, but copying and adapting sources from
> elsewhere is quite normal, and the system library rule doesn't even come into
> play since we are not building any libraries here at all.    

Please take a look at the bugs blocking the duplication of system libraries tracker bug 504493, for example at but 495310. Duplication of system libraries not only means that you must not build new, modified libs but also that you should not contain forked copies of other code in your tarballs to build against them. The focus should be on upstreaming the changes and I'm sure you will be going the right thing here.
Comment 4 Matthias Clasen 2010-02-02 23:09:17 EST
New Package CVS Request
=======================
Package Name: accountsdialog
Short Description: An application to view and modify user accounts information
Owners: mclasen
Branches: 
InitialCC:
Comment 5 Kevin Fenzi 2010-02-02 23:15:59 EST
CVS done (by process-cvs-requests.py).
Comment 6 Matthias Clasen 2010-02-03 11:51:36 EST
build done