For a good package, bug #1429090 needs to get fixed.

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/

This request is stalled for now. ecryptfs-utils (dependency of ecryptfs-simple) is currently FTBFS.

ecryptfs-utils is fixed. We can continue with this review.

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

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

Do you have plans to update the spec file to v1.2.7, so I could continue the review?

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

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

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.

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> - 1.2.9-1
- new version
- unbundle json

Damiam, are you still interested in continueing with this review? If yes, please set fedora-review flag.

(In reply to Raphael Groner from comment #12)
Yes, I'm interested - (just after my holiday).

(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

Friendly reminder.

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

Thanks for your comments. Will update soonish, please be still patient.

(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

Lost this, sorry.

(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

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> - 1.3.2-1
- new version
- drop BSD because unbundled json
- unbundle lxqt_wallet

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

Upstream added appdata:
https://github.com/mhogomchungu/sirikali/commit/ef54268108f0f04e5e1c8aaffd2bf4bdcca4d087

Possible FTBFS due to missing task.h is also fixed:
https://github.com/mhogomchungu/sirikali/commit/6dad4d1feb4f4f2f98ab4c02114cafd560cb61c0

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

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> - 1.3.2-2
- drop obsolete scriptlets
- drop explicit file permission
- include upstreamed patches

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

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

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

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.

https://pagure.io/releng/fedora-scm-requests/issue/4485
https://pagure.io/releng/fedora-scm-requests/issue/4486

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/sirikali

sirikali-1.3.3-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-4e325158b8

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

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.