Bug 1193878 - Review Request: qmasterpassword - Stateless Master Password Manager
Summary: Review Request: qmasterpassword - Stateless Master Password Manager
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-02-18 13:38 UTC by Beat Küng
Modified: 2015-07-21 13:21 UTC (History)
3 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-07-21 13:21:50 UTC
bugs.michael: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Beat Küng 2015-02-18 13:38:33 UTC
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/

Comment 1 Beat Küng 2015-02-18 13:44:58 UTC
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

Comment 2 Sinny Kumari 2015-04-12 17:56:37 UTC
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.

Comment 3 Michael Schwendt 2015-04-12 21:13:30 UTC
> 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

Comment 4 Beat Küng 2015-04-13 13:10:18 UTC
(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

Comment 5 Beat Küng 2015-04-13 13:12:41 UTC
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

Comment 6 Sinny Kumari 2015-04-13 20:26:20 UTC
(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.

Comment 7 Beat Küng 2015-04-14 07:16:13 UTC
(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

Comment 8 Sinny Kumari 2015-04-14 11:53:17 UTC
(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

Comment 9 Sinny Kumari 2015-04-14 12:16:39 UTC
(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

Comment 10 Beat Küng 2015-04-14 12:51:41 UTC
> 
> 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.

Comment 11 Beat Küng 2015-07-06 05:02:03 UTC
libscrypt has been updated to 1.20-1 in F22, so it compiles fine now:
http://koji.fedoraproject.org/koji/taskinfo?taskID=10287067

Comment 12 Michael Schwendt 2015-07-14 10:18:57 UTC
> 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



* 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.

Comment 13 Beat Küng 2015-07-15 11:16:35 UTC
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.

Comment 14 Michael Schwendt 2015-07-15 18:48:56 UTC
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.

Comment 15 Beat Küng 2015-07-16 07:58:14 UTC
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:

Comment 16 Kevin Fenzi 2015-07-20 16:45:00 UTC
Git done (by process-git-requests).


Note You need to log in before you can comment on or make changes to this bug.