https://bugzilla.redhat.com/show_bug.cgi?id=1193878 --- Comment #13 from Beat Küng <beat-kueng@xxxxxxx> --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review