https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #13 from Beat Küng beat-kueng@gmx.net --- Thanks for the review!
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #12)
Spec URL: http://n.ethz.ch/~bkueng/qmasterpassword/qmasterpassword.spec SRPM URL: http://n.ethz.ch/~bkueng/qmasterpassword/qmasterpassword-1.1-1.fc21.src.rpm
fedora-review notices that the spec file at "Spec URL:" is different from the spec file in the src.rpm
The changes in the spec file would have been a great opportunity to practise bumping the Release tag and maintaining the %changelog comments at the bottom: https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes
# QLineEdit::clearButtonEnabled requires Qt 5.2 or higher Requires: qt5-qtbase >= 5.2.0
First of all, adding %{?_isa} would be safer and would prevent package resolver tools from running into multiarch problems easily (that can happen when they run into unresolvable dependencies and pull in old multilib packages to satisfy dependencies).
Secondly, your added comment indicates you fear that somebody might not have Qt >= 5.2 when installing qMasterPassword. When would that be the case?
Qt older than 5.2.0 is very old: http://koji.fedoraproject.org/koji/packageinfo?packageID=15742
Fedora 21 has started with 5.3.2 already: http://dl.fedoraproject.org/pub/fedora/linux/releases/21/Everything/x86_64/ os/Packages/q/qt5-qtbase-5.3.2-4.fc21.x86_64.rpm
Removed the Qt requirement
- fedora-review licensecheck.txt confirms that all source files are GPLv3.
Okay.
fedora-review doesn't report any basic issues. Okay.
rpmlint output:
| qmasterpassword.x86_64: W: no-manual-page-for-binary qMasterPassword
Acceptable, as it's a GUI.
| qmasterpassword.x86_64: E: invalid-appdata-file /usr/share/appdata/qMasterPassword.appdata.xml
Caution! This is because rpmlint runs "appdata-validate" on the file and therefore finds more issues that what is required by the guidelines. Let's take a look with appstream-util non-relaxed validate:
$ appstream-util validate qMasterPassword.appdata.xml qMasterPassword.appdata.xml: FAILED: • tag-missing : <update_contact> is not present • tag-missing : <name> is not present • tag-missing : <url> is not present Validation of files failed
These may be worth fixing upstream. Even when "validate-relax" says "OK", there are corner-cases when GNOME Software may not display the program. For example, Scribus is affected: https://bugzilla.redhat.com/1231445
Fixed upstream
- Package listing:
$ rpmls qmasterpassword -rwxr-xr-x /usr/bin/qMasterPassword -rw-r--r-- /usr/share/appdata/qMasterPassword.appdata.xml -rw-r--r-- /usr/share/applications/qMasterPassword.desktop drwxr-xr-x /usr/share/doc/qmasterpassword -rw-r--r-- /usr/share/doc/qmasterpassword/HISTORY -rw-r--r-- /usr/share/doc/qmasterpassword/README.md drwxr-xr-x /usr/share/licenses/qmasterpassword -rw-r--r-- /usr/share/licenses/qmasterpassword/LICENSE -rw-r--r-- /usr/share/pixmaps/qmasterpassword.png
Looks okay.
- Some compiler warnings in the build.log:
main_window.cpp warning: enumeration value 'Type_random_failed' not handled in switch
Worth looking into. And switch-without-default is a matter of taste. ;-)
Fixed upstream
- unit tests:
If you like building unit tests to check against the build target environment early, the %check section would be the right place where to run them.
Added unit tests in %check
- Else package looks good.
I updated the spec file & uploaded the new src rpm: http://n.ethz.ch/~bkueng/qmasterpassword/qmasterpassword-1.1-2.fc21.src.rpm Upstream changes are not yet included in this.