Bug 1310294

Offhand, in -devel subpkg I see:

1.  
%{_includedir}/%{name}/sqlite3ext.h
%{_includedir}/%{name}/sqlite3.h

which implies that
%{_includedir}/%{name}
dir is not owned, imo, easier to replace those 2 lines with just:

%{_includedir}/sqlcipher/

(personal preference of mine to not use %name macro here, so that it'll continue to work regardless of the actual package name)

2. 
%{_libdir}/libsqlcipher-3.8.10.2.so.0
%{_libdir}/libsqlcipher-3.8.10.2.so.0.8.6

these libraries should be in the main pkg (not -devel), and include ldconfig scriptlets

3.
%{_libdir}/libsqlcipher.la

libtool archives should not be included in packaging, this file should be deleted or otherwise excluded.

4.  %package -devel MUST have a tighter dependency on the main pkg, instead of
Requires: %{name} = %{version}
use
Requires: %{name}%{?_isa} = %{version}-%{release}

5. SHOULD document why these compiler flags are needed in a .spec comment
CFLAGS="-DSQLITE_HAS_CODEC -DSQLITE_TEMP_STORE=2"
LDFLAGS="-lcrypto"

6. While you're at it, MUST respect distro compiler flags, using (something like)
CFLAGS="%{optflags} -DSQLITE_HAS_CODEC -DSQLITE_TEMP_STORE=2"
LDFLAGS="%{?__global_ldflags} -lcrypto"

Hi,
is there any progress with this package? I could do the official review.

(In reply to Raphael Groner from comment #2)
> Hi,
> is there any progress with this package? I could do the official review.

Thanks I will fix it today.

(In reply to Raphael Groner from comment #2)
> Hi,
> is there any progress with this package? I could do the official review.

new SPEC and SRPM

SPEC: https://siddharths.fedorapeople.org/SPECS/sqlcipher.spec
SRPM: https://siddharths.fedorapeople.org/SRPMS/sqlcipher-3.3.1-2.fc23.src.rpm

It would also be good to look at this bug https://bugzilla.redhat.com/show_bug.cgi?id=996813 while reviewing new specs in
https://bugzilla.redhat.com/show_bug.cgi?id=1310294#c4

*** Bug 996813 has been marked as a duplicate of this bug. ***

Why do you set the fedora-review flag for yourself? That's not in conjunction with the common process, instead a reviewer have to do that (besides also assigning the bug).
https://fedoraproject.org/wiki/Package_Review_Process#Reviewer

I anyone looking to do fedora-review

Is anyone looking to do fedora-review

What's your FAS account? Please use the same e-mail address and name for your Bugzilla account. Otherwise, people have trouble to identify you.

(In reply to Siddharth Sharma from comment #4)
> (In reply to Raphael Groner from comment #2)
> > Hi,
> > is there any progress with this package? I could do the official review.
> 
> new SPEC and SRPM
> 
> SPEC: https://siddharths.fedorapeople.org/SPECS/sqlcipher.spec
> SRPM:
> https://siddharths.fedorapeople.org/SRPMS/sqlcipher-3.3.1-2.fc23.src.rpm

Review swap with bug #1343733?

(In reply to Raphael Groner from comment #10)
> What's your FAS account? Please use the same e-mail address and name for
> your Bugzilla account. Otherwise, people have trouble to identify you.

FAS: siddharths

Are you interested in a review swap? See comment #11.

Are you interested in some review for mono packages to do a review swap?

I can review today

Looks good, all the blocker items mentioned previously were addressed.  Remaining items to consider:

5. SHOULD document why these compiler flags are needed in a .spec comment
CFLAGS="-DSQLITE_HAS_CODEC -DSQLITE_TEMP_STORE=2"
LDFLAGS="-lcrypto"

...

I'd suggest a comment:
# recommended in README.md ## Compiling section

7a.  MUST use %%license tag, instead of:
%doc LICENSE README.md
use
%doc README.md
%license LICENSE

7b.  SHOULD, since main pkg already includes README.md and LICENSE, no need to include in -devel and -tcl subpkg's too, please remove them.

8. (since you're already using %{make_install}), SHOULD use convenience macro %{make_build}
instead of
make %{?_smp_mflags}
use
%{make_build}

naming: ok

sources: ok
26be3c23220192fb42e1d60f8c90ac69  v3.3.1.tar.gz

macros: ok

scriptlets: ok

licensing: NOT ok

9. MUST use
License: BSD
to match recommendation on 
https://fedoraproject.org/wiki/Licensing

10a. SHOULD consider using build option:
--disable-tcl
and avoid the hassle of the -tcl subpkg, unless you specifically are willing to support this.

10b.  MUST: If you choose to keep -tcl subpkg, then must add a versioned dependency to main pkg:
Requires: %{name}%{?_isa} = %{version}-%{release}

11. MUST remove
%define _unpackaged_files_terminate_build 0
there are better ways of fixing unpackaged files than this hack, for example, adding this after %{make_install}:
rm -fv %{buildroot}%{_libdir}/lib*.la

12. MUST fix rpath issues, doing a local build, I get:
ERROR   0001: file '/usr/share/tcl8.6/sqlite3/libtclsqlite3.so' contains a standard rpath '/usr/lib64' in [/usr/lib64]
ERROR   0001: file '/usr/bin/sqlcipher' contains a standard rpath '/usr/lib64' in [/usr/lib64]
per https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Removing_Rpath
adding after %configure, this helps:

13. SHOULD remove deprecated items/tags from .spec, including:
(from %install section):
rm -rf $RPM_BUILD_ROOT


Please fix all MUST (blocker) items, and I will approve this package review.  Consider addressing SHOULD (optional) items too, and you'll be extra awesome.

If it helps, here's a copy of 
https://rdieter.fedorapeople.org/rpms/sqlcipher.spec
that implements all the suggested fixes (I think).

re-reading, looks like my item 12 was missing the actual spec suggestion:
... this helps:
# fix/workaround hard-coded rpaths
sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

(In reply to Rex Dieter from comment #17)
> re-reading, looks like my item 12 was missing the actual spec suggestion:
> ... this helps:
> # fix/workaround hard-coded rpaths
> sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g'
> libtool
> sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

Thanks a lot Rex :) Will fix it asap.

(In reply to Rex Dieter from comment #17)
> re-reading, looks like my item 12 was missing the actual spec suggestion:
> ... this helps:
> # fix/workaround hard-coded rpaths
> sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g'
> libtool
> sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool

Updated for Review

SPECS: https://siddharths.fedorapeople.org/SPECS/sqlcipher.spec
SRPMS: https://siddharths.fedorapeople.org/SRPMS/sqlcipher-3.3.1-4.fc24.src.rpm

I removed tcl from packaging and openssl-devel was missing from build requires.

Looks good, APPROVED

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/sqlcipher

builds submitted

http://koji.fedoraproject.org/koji/taskinfo?taskID=15039542
http://koji.fedoraproject.org/koji/taskinfo?taskID=15039547
http://koji.fedoraproject.org/koji/taskinfo?taskID=15039559
http://koji.fedoraproject.org/koji/taskinfo?taskID=15039582

Are you planning on submitting updates for f24/f23 to bodhi?  (I don't see any yet)

sqlcipher-3.3.1-4.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-068d457ba0

sqlcipher-3.3.1-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-5ff49b651b

sqlcipher-3.3.1-4.fc23 has been pushed to the Fedora 23 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-2016-5ff49b651b

sqlcipher-3.3.1-4.fc24 has been pushed to the Fedora 24 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-2016-068d457ba0

Can this be closed as sqlcipher is already available now.

Yes, closing