Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1428202 - Review Request: sirikali - GUI front end to encfs,cryfs,gocryptfs and securefs
Summary: Review Request: sirikali - GUI front end to encfs,cryfs,gocryptfs and securefs
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Damian Wrobel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1384023 1429090
Blocks: qt-reviews
TreeView+ depends on / blocked
 
Reported: 2017-03-02 00:30 UTC by Raphael Groner
Modified: 2018-02-27 17:19 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-02-27 17:19:25 UTC
dwrobel: fedora-review+


Attachments (Terms of Use)

Description Raphael Groner 2017-03-02 00:30:31 UTC
Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.2.5-1.fc25.src.rpm
Description: SiriKali is a Qt/C++ GUI front end to encfs,cryfs,gocryptfs and securefs.
Fedora Account System Username: raphgro

Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=18130114

Comment 1 Raphael Groner 2017-03-04 09:03:21 UTC
For a good package, bug #1429090 needs to get fixed.

Comment 2 Damian Wrobel 2017-03-08 17:28:25 UTC
Please find some initial comments:

 %doc COPYING
COPYING should go to %license, please also remove the redundant space character in front of %doc 


%doc README*
It's minor, but it causes that both README and README.md are packaged, although both are identical:
$ sha256sum README README.md 
6b6f723b4db0fe3468e32a6cb488c42f2cc3c499538367bb89bebd3b432fe6c6  README
6b6f723b4db0fe3468e32a6cb488c42f2cc3c499538367bb89bebd3b432fe6c6  README.md
Maybe upstream can simply remove one of it?


%dir %{_datadir}/%{name}/translations
%dir %{_datadir}/%{name}
Please consider reverting the order of them, will look more natural.


-rwxr-xr-x    1 root    root                      332 Mar  8 16:24 /usr/share/applications/sirikali.desktop
Destkop file seems to have superfluous executable attribute.


Based on the project's homepage[1]:

cryfs binary application is required to be installed for SiriKali to gain support for cryfs volumes.

gocryptfs binary application is required to be installed for SiriKali to gain support for gocryptfs volumes.

encfs binary application is required to be installed for SiriKali to gain support for encfs volumes.

securefs binary application is required to be installed for SiriKali to gain support for securefs volumes.

But the package itself doesn't list any of them as a run-time dependency.

BTW, we don't seem to have cryfs:

$ dnf provides '*cryfs'
Last metadata expiration check: 2:21:08 ago on Wed Mar  8 15:59:56 2017.
Error: No Matches found

[1] https://mhogomchungu.github.io/sirikali/

Comment 3 Raphael Groner 2017-04-01 06:46:54 UTC
This request is stalled for now. ecryptfs-utils (dependency of ecryptfs-simple) is currently FTBFS.

Comment 4 Raphael Groner 2017-06-04 20:19:39 UTC
ecryptfs-utils is fixed. We can continue with this review.

Comment 5 Raphael Groner 2017-06-04 23:27:03 UTC
Daniel, thanks for your comments.

Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.2.5-2.fc26.src.rpm

Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=19859311

(In reply to Damian Wrobel from comment #2)
> Please find some initial comments:
> 
>  %doc COPYING
> COPYING should go to %license, please also remove the redundant space
> character in front of %doc 
fixed

> %doc README*
> It's minor, but it causes that both README and README.md are packaged,
> although both are identical:
> $ sha256sum README README.md 
> 6b6f723b4db0fe3468e32a6cb488c42f2cc3c499538367bb89bebd3b432fe6c6  README
> 6b6f723b4db0fe3468e32a6cb488c42f2cc3c499538367bb89bebd3b432fe6c6  README.md
> Maybe upstream can simply remove one of it?
fixed, no README found in current trunk

> %dir %{_datadir}/%{name}/translations
> %dir %{_datadir}/%{name}
> Please consider reverting the order of them, will look more natural.
fixed

> -rwxr-xr-x    1 root    root                      332 Mar  8 16:24
> /usr/share/applications/sirikali.desktop
> Destkop file seems to have superfluous executable attribute.
fixed

> Based on the project's homepage[1]: > gocryptfs binary application is required to be installed for SiriKali to
> gain support for gocryptfs volumes.
no package found for gocryptfs, it needs golang [1] etc. to build properly, maybe come back to gocryptfs later
https://nuetzlich.net/gocryptfs/compile/

> encfs binary application is required to be installed for SiriKali to gain
> support for encfs volumes.
fixed with weak dependency to fuse-encfs

> securefs binary application is required to be installed for SiriKali to gain
> support for securefs volumes.
no idea about this, please ignore

…
> $ dnf provides '*cryfs'
> Last metadata expiration check: 2:21:08 ago on Wed Mar  8 15:59:56 2017.
> Error: No Matches found
> 
> [1] https://mhogomchungu.github.io/sirikali/
there's an upstream issue [2] about docker and Fedora. PCLinuxOS has some effort [3] to ship a package of cryfs, maybe worth to look into their srpm, though: "This package is obsolete. Try find newer cryfs-gui [4]" owned by same upstream as of sirikali
[2] https://github.com/cryfs/cryfs/issues/89
[3] http://rpm.pbone.net/index.php3/stat/4/idpl/34219112/dir/pclinuxos/com/cryfs-gui-1.3.1-2pclos2016.x86_64.rpm.html
[4] https://github.com/mhogomchungu/cryfs-gui/blob/master/rpm/cryfs-gui.spec

Comment 6 Raphael Groner 2017-06-10 17:57:28 UTC
Upstream writes:
You will need this[1] patch to make released version of SiriKali(1.2.7) work with suid-root-less ecryptfs-simple in fedora. This is because Fedora does not have "su" binary where SiriKali expects it to be.

In SiriKali 1.2.7 going forward, polkit support can be set from GUI by selecting menu->enable/disable polkit support.

[1] https://github.com/mhogomchungu/sirikali/commit/4ebe69f0dd4ddc3f57764aa697f39a9ae305cae6

Comment 7 Damian Wrobel 2017-06-12 13:38:37 UTC
Do you have plans to update the spec file to v1.2.7, so I could continue the review?

Comment 8 Raphael Groner 2017-06-12 17:29:50 UTC
Thanks for the hint about a new version.

Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.2.7-1.20170611git.fc26.src.rpm

%changelog
* Mon Jun 12 2017 Builder <projects.rg@smart.ms> - 1.2.7.1.20170611git
- new version
- use git snapshot to include latest upstream patches
- include upstream patch to get full path of su binary
- distribute additonal files
- drop workaround for duplicated readme files
- fix length of line in description

Comment 9 Raphael Groner 2017-06-12 17:38:49 UTC
Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20000007

Comment 10 Raphael Groner 2017-06-12 17:52:49 UTC
Noticed that we've a package for bundled json.
https://admin.fedoraproject.org/pkgdb/package/rpms/json/
https://github.com/mhogomchungu/sirikali/blob/master/src/3rdParty/json/json.hpp

I'll update this review ASAP to unbundle json.

Comment 11 Raphael Groner 2017-07-11 20:43:33 UTC
Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.2.9-1.fc26.src.rpm

* Tue Jul 11 2017 Raphael Groner <projects.rg@smart.ms> - 1.2.9-1
- new version
- unbundle json

Comment 12 Raphael Groner 2017-07-14 14:58:35 UTC
Damiam, are you still interested in continueing with this review? If yes, please set fedora-review flag.

Comment 13 Damian Wrobel 2017-07-15 19:18:17 UTC
(In reply to Raphael Groner from comment #12)
Yes, I'm interested - (just after my holiday).

Comment 14 Raphael Groner 2017-07-16 06:02:10 UTC
(In reply to Damian Wrobel from comment #13)
> (In reply to Raphael Groner from comment #12)
> Yes, I'm interested - (just after my holiday).

Please follow the policy:
https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

Comment 15 Raphael Groner 2017-08-13 21:38:34 UTC
Friendly reminder.

Comment 16 Damian Wrobel 2017-08-21 16:14:51 UTC
Few comments:

# general license is GPLv2+ but BSD for 3rdParty/json.hpp
License:        GPLv2+ and BSD

Now when json 3rdparty had been unbundled the BSD part seems to not be applicable any longer.


Based on the build-log and debug-info I see that lxqt-wallet seems to be used (the bundled) version. It looks that we have in fedora[1]. Please consider to unbundle.


As per [2] it would be nice to include appdata (for convenience Debian seems to have it, see: [3]).


[1]. https://admin.fedoraproject.org/pkgdb/package/rpms/lxqt-wallet/
[2]. https://fedoraproject.org/wiki/Packaging:AppData 
[3]. https://packages.debian.org/sid/amd64/sirikali/filelist

Comment 17 Raphael Groner 2017-09-21 23:06:22 UTC
Thanks for your comments. Will update soonish, please be still patient.

Comment 18 Raphael Groner 2017-09-22 09:37:04 UTC
(In reply to Damian Wrobel from comment #16)
> Few comments:> Now when json 3rdparty had been unbundled the BSD part seems to not be
> applicable any longer.

OK

> Based on the build-log and debug-info I see that lxqt-wallet seems to be
> used (the bundled) version. It looks that we have in fedora[1]. Please
> consider to unbundle.

Found some (old & closed) issues in upstream tracker. Therefore not sure if unbundling is fully supported but CMakeLists.txt has an option, we'll see.
https://github.com/mhogomchungu/sirikali/issues/9
https://github.com/mhogomchungu/sirikali/issues/34
https://github.com/mhogomchungu/sirikali/blob/master/CMakeLists.txt#L75

> As per [2] it would be nice to include appdata (for convenience Debian seems
> to have it, see: [3]).

OK

> [1]. https://admin.fedoraproject.org/pkgdb/package/rpms/lxqt-wallet/
> [2]. https://fedoraproject.org/wiki/Packaging:AppData 
> [3]. https://packages.debian.org/sid/amd64/sirikali/filelist

Comment 19 Raphael Groner 2017-10-30 08:21:19 UTC
Lost this, sorry.

Comment 20 Raphael Groner 2018-01-13 10:35:01 UTC
(In reply to Raphael Groner from comment #18)

> > As per [2] it would be nice to include appdata (for convenience Debian seems
> > to have it, see: [3]).
> 
> OK

https://github.com/mhogomchungu/sirikali/pull/56

> 
> > [1]. https://admin.fedoraproject.org/pkgdb/package/rpms/lxqt-wallet/
> > [2]. https://fedoraproject.org/wiki/Packaging:AppData 
> > [3]. https://packages.debian.org/sid/amd64/sirikali/filelist

Comment 21 Raphael Groner 2018-01-13 10:57:46 UTC
Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.3.2-1.fc27.src.rpm

%changelog
* Sat Jan 13 2018 Raphael Groner <projects.rg@smart.ms> - 1.3.2-1
- new version
- drop BSD because unbundled json
- unbundle lxqt_wallet

Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=24169375

Comment 23 Damian Wrobel 2018-01-17 14:12:49 UTC
Raphael,

Two minor issues:

Please remove %post, %postun and %posttrans sections as per [4] - now it's obsolete. There was a change recently in that area (see History section of [4]).

Please remove %attr(644,-,-) in %files section as it shouldn't be used unless you need to set non-default value. BTW 0644 is already set in %install section and is the default one as per [5].

Rest looks ok.

[4] https://fedoraproject.org/wiki/Packaging:Scriptlets#Icon_Cache
[5] https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

Comment 24 Raphael Groner 2018-02-07 21:23:15 UTC
Sorry for the delay. I was busy with other work.

Spec URL: https://raphgro.fedorapeople.org/review/util/sirikali.spec
SRPM URL: https://raphgro.fedorapeople.org/review/util/sirikali-1.3.2-1.fc27.src.rpm

%changelog
* Wed Feb 07 2018 Raphael Groner <projects.rg@smart.ms> - 1.3.2-2
- drop obsolete scriptlets
- drop explicit file permission
- include upstreamed patches

Test builds: https://koji.fedoraproject.org/koji/taskinfo?taskID=24811226

Comment 26 Damian Wrobel 2018-02-12 13:15:56 UTC
Raphael,

Please consider adding upstream link to patches as per [6].

Package approved.

[6] https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Comment 27 Raphael Groner 2018-02-12 19:20:40 UTC
Thanks for the review!

What's wrong with the comments above the patches? I forgot how upstream gets notified and let know about both commits. Maybe upstream wrote me by e-mail.

Comment 29 Gwyn Ciesla 2018-02-12 20:00:49 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/sirikali

Comment 30 Fedora Update System 2018-02-14 22:06:21 UTC
sirikali-1.3.3-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4e325158b8

Comment 31 Fedora Update System 2018-02-15 15:28:39 UTC
sirikali-1.3.3-1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-4e325158b8

Comment 32 Fedora Update System 2018-02-27 17:19:25 UTC
sirikali-1.3.3-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.


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