https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Bug ID: 1193878 Summary: Review Request: qmasterpassword - Stateless Master Password Manager Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: beat-kueng@gmx.net QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
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 Description: qMasterPassword is a graphical master password manager using Qt. How is it different from existing managers? It generates unique passwords from a master password and a site name. This means no (encrypted) secrets need to be stored in a safe file and thus nothing can get lost or stolen. There is compatible software for other platforms, ie Android or iOS on [1]. This page also describes the algorithm in detail.
Fedora Account System Username: bkueng
koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=8981274
[1] http://masterpasswordapp.com/
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Beat Küng beat-kueng@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #1 from Beat Küng beat-kueng@gmx.net --- Hi Since I use Fedora, I thought it would be a useful addition to include qMasterPassword in the package repository. I would like to maintain the package and thus learn the full process, but I need a sponsor. However if someone wants to take over and maintain it, that would be fine with me too.
The source is on [1], which is also my personal github page (I'm the author). Among others I contributed to upstream yumex and OpenCV, but no experience with packaging yet.
There is one warning from rpmlint, that there is no man page, but I don't think it is needed since it is a GUI application.
fedora-review complains: - 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 is included in %doc. Note: Cannot find LICENSE in rpm(s) But I use %license which as far as I know is the correct thing to do.
[1] https://github.com/bkueng/qMasterPassword
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Sinny Kumari ksinny@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ksinny@gmail.com
--- Comment #2 from Sinny Kumari ksinny@gmail.com --- This is unofficial review of the package-
Packaging looks good to me.
Building package for Fedora rawhide is failing http://koji.fedoraproject.org/koji/taskinfo?taskID=9464365 . I think this should be fixed first.
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #3 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net ---
Requires: qt5-qtbase >= 5.2.0
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
%{_datadir}/appdata/%{project_name}.appdata.xml
https://fedoraproject.org/wiki/Packaging:Guidelines#AppData_files
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #4 from Beat Küng beat-kueng@gmx.net --- (In reply to Sinny Kumari from comment #2)
This is unofficial review of the package-
Packaging looks good to me.
Building package for Fedora rawhide is failing http://koji.fedoraproject.org/koji/taskinfo?taskID=9464365 . I think this should be fixed first.
The koji error is: undefined reference to `libscrypt_scrypt(unsigned char const*, unsigned int, unsigned char const*, unsigned int, unsigned long long, unsigned int, unsigned int, unsigned char*, unsigned int)'
I've seen this before, it was because of a missing 'extern "C"{' in the header of the libscrypt library. Just to test it, I added this in my code, and then koji builds fine [1]. But this must be fixed in the library.
However it is weird because according to [2] all fedora versions since f19 use the same version of the library. I don't know koji well enough to get to the bottom of this...
[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=9469356 [2] http://pkgs.fedoraproject.org/cgit/libscrypt.git
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #5 from Beat Küng beat-kueng@gmx.net --- Updated the spec file.
Requires: qt5-qtbase >= 5.2.0
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
I added a comment about the Qt version requirement.
%{_datadir}/appdata/%{project_name}.appdata.xml
https://fedoraproject.org/wiki/Packaging:Guidelines#AppData_files
added %check
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #6 from Sinny Kumari ksinny@gmail.com --- (In reply to Beat Küng from comment #5)
Updated the spec file.
Requires: qt5-qtbase >= 5.2.0
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
I added a comment about the Qt version requirement.
Explicit require is not needed to specify in a spec file. It is left to be handled by rpmbuild to automatically add dependencies on library SONAMEs. Try to remove Requires: qt5-qtbase >= 5.2.0
and then rebuild package and then install it on system/test box and see if right dependency of qt5-base is getting pulled or not.
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #7 from Beat Küng beat-kueng@gmx.net --- (In reply to Sinny Kumari from comment #6)
(In reply to Beat Küng from comment #5)
Updated the spec file.
Requires: qt5-qtbase >= 5.2.0
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
I added a comment about the Qt version requirement.
Explicit require is not needed to specify in a spec file. It is left to be handled by rpmbuild to automatically add dependencies on library SONAMEs. Try to remove Requires: qt5-qtbase >= 5.2.0
and then rebuild package and then install it on system/test box and see if right dependency of qt5-base is getting pulled or not.
I tried to remove the line and rpmbuild adds the following dependencies: libQt5Core.so.5()(64bit) libQt5DBus.so.5()(64bit) libQt5Gui.so.5()(64bit) libQt5Widgets.so.5()(64bit) [...]
which resolves to qt5-qtbase. The thing is if someone has qt5-base 5.0 installed, these deps are fulfilled, but the program actually uses functionality that was only introduced in Qt 5.2. So I wanted to make sure this is satisfied as well with Requires: qt5-qtbase >= 5.2.0
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #8 from Sinny Kumari ksinny@gmail.com --- (In reply to Beat Küng from comment #7)
I tried to remove the line and rpmbuild adds the following dependencies: libQt5Core.so.5()(64bit) libQt5DBus.so.5()(64bit) libQt5Gui.so.5()(64bit) libQt5Widgets.so.5()(64bit) [...]
which resolves to qt5-qtbase. The thing is if someone has qt5-base 5.0 installed, these deps are fulfilled, but the program actually uses functionality that was only introduced in Qt 5.2. So I wanted to make sure this is satisfied as well with Requires: qt5-qtbase >= 5.2.0
Okay, then it make sense to keep explicit requires in spec file.
Also whenever you modify spec file. New changelog entry should be added as per Fedora guideline http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #9 from Sinny Kumari ksinny@gmail.com --- (In reply to Beat Küng from comment #4)
(In reply to Sinny Kumari from comment #2)
This is unofficial review of the package-
Packaging looks good to me.
Building package for Fedora rawhide is failing http://koji.fedoraproject.org/koji/taskinfo?taskID=9464365 . I think this should be fixed first.
The koji error is: undefined reference to `libscrypt_scrypt(unsigned char const*, unsigned int, unsigned char const*, unsigned int, unsigned long long, unsigned int, unsigned int, unsigned char*, unsigned int)'
I've seen this before, it was because of a missing 'extern "C"{' in the header of the libscrypt library. Just to test it, I added this in my code, and then koji builds fine [1]. But this must be fixed in the library.
However it is weird because according to [2] all fedora versions since f19 use the same version of the library. I don't know koji well enough to get to the bottom of this...
No, rawhide and F21 uses different version of libscrypt as you can from root.log file of respective koji build for rawhide and F21.
F21 - libscrypt version - 1.20-1.fc21 Scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=9476344 root.log - https://kojipkgs.fedoraproject.org//work/tasks/6347/9476347/root.log
rawhide - libscrypt version - 1.19-3.fc22 Scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=9476350 root.log - https://kojipkgs.fedoraproject.org//work/tasks/6353/9476353/root.log
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #10 from Beat Küng beat-kueng@gmx.net ---
No, rawhide and F21 uses different version of libscrypt as you can from root.log file of respective koji build for rawhide and F21.
F21 - libscrypt version - 1.20-1.fc21 Scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=9476344 root.log - https://kojipkgs.fedoraproject.org//work/tasks/6347/9476347/root.log
rawhide - libscrypt version - 1.19-3.fc22 Scratch build - http://koji.fedoraproject.org/koji/taskinfo?taskID=9476350 root.log - https://kojipkgs.fedoraproject.org//work/tasks/6353/9476353/root.log
Ok then it makes sense. But rawhide should use the newer fixed version then.
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #11 from Beat Küng beat-kueng@gmx.net --- libscrypt has been updated to 1.20-1 in F22, so it compiles fine now: http://koji.fedoraproject.org/koji/taskinfo?taskID=10287067
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Michael Schwendt (Fedora Packager Sponsors Group) bugs.micheal@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |bugs.micheal@gmx.net Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #12 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.micheal@gmx.net ---
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/o...
* 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
* 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. ;-)
* 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.
* Else package looks good.
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Michael Schwendt (Fedora Packager Sponsors Group) bugs.micheal@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) | Flags|fedora-review? |fedora-review+
--- Comment #14 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.micheal@gmx.net --- Okay.
Just one minor nitpick:
$ rpmlint qmasterpassword-1.1-2.fc21.src.rpm qmasterpassword.src:73: W: macro-in-%changelog %check 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
In the %changelog or in other comments anywhere in the spec file, some macros expand and cause side-effects, which may even influence the build in bad ways. Imagine you added a changelog comment mentioning the "%configure" macro. In the built packages you would get to see the exanded macro, i.e. a big blog as can be seen in "rpm -E %configure".
Hence in comments it is better to escape macros with a double '%%'.
-BuildRequires: qt5-qtbase-devel >= 5.2.0 +BuildRequires: qt5-qtbase-devel
Note that my earlier comment referred to the "Requires" tag only, not the BuildRequires.
It's okay to drop the version here, too, but minimum versions on BuildRequires can be helpful actually when trying to build a src.rpm for a completely unexpected build target. For example, if Fedora EPEL 6 did not contain Qt 5.4.x already, the src.rpm would fail to build.
Though, in many packages, minimum version requirements are out-of-date, and configure scripts or other version checks are applied by the used build framework.
%changelog
There is nothing about this in the guidelines,
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
and it is no blocker criterion, but "rpm --query --changelog …" output adds an empty line between individual changelog entries for increase readability. So, it's widely accepted practice to do that also in the spec file and increase readability. Decide yourself whether you like that, too.
* Package builds fine on Rawhide and F21. * Basic installation and runtime testing: F21 with GNOME Shell
Package APPROVED.
Please fix the unescaped macro in %changelog when/after importing into dist git.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Beat Küng beat-kueng@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #15 from Beat Küng beat-kueng@gmx.net --- New Package SCM Request ======================= Package Name: qmasterpassword Short Description: Stateless Master Password Manager Upstream URL: https://github.com/bkueng/qMasterPassword Owners: bkueng Branches: f21 f22 f23 InitialCC:
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Kevin Fenzi kevin@scrye.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
--- Comment #16 from Kevin Fenzi kevin@scrye.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=1193878
Michael Schwendt (Fedora Packager Sponsors Group) bugs.micheal@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2015-07-21 09:21:50
package-review@lists.fedoraproject.org