Bug 1310294 - Review Request: sqlcipher - An SQLite extension that provides 256 bit AES encryption of database files
Summary: Review Request: sqlcipher - An SQLite extension that provides 256 bit AES enc...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 996813 (view as bug list)
Depends On:
Blocks: 1261614
TreeView+ depends on / blocked
 
Reported: 2016-02-20 04:28 UTC by Siddharth Sharma
Modified: 2017-01-07 15:18 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2017-01-07 15:18:02 UTC
Type: ---
Embargoed:
rdieter: fedora-review+


Attachments (Terms of Use)

Description Siddharth Sharma 2016-02-20 04:28:52 UTC
Spec URL: https://siddharths.fedorapeople.org/SPECS/sqlcipher.spec
SRPM URL: https://siddharths.fedorapeople.org/SRPMS/sqlcipher-3.3.1-1.fc23.src.rpm

Description:
SQLCipher is an SQLite extension that provides transparent 256-bit
AES encryption of database files. Pages are encrypted before being
written to disk and are decrypted when read back. Due to the small
footprint and great performance it’s ideal for protecting embedded
application databases and is well suited for mobile development.

Fedora Account System Username: siddharths

Comment 1 Rex Dieter 2016-02-20 04:42:38 UTC
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"

Comment 2 Raphael Groner 2016-03-06 17:24:39 UTC
Hi,
is there any progress with this package? I could do the official review.

Comment 3 Siddharth Sharma 2016-03-09 15:34:26 UTC
(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.

Comment 4 Siddharth Sharma 2016-03-09 16:04:55 UTC
(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

Comment 5 Siddharth Sharma 2016-03-09 16:07:37 UTC
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

Comment 6 Raphael Groner 2016-03-10 10:02:58 UTC
*** Bug 996813 has been marked as a duplicate of this bug. ***

Comment 7 Raphael Groner 2016-04-07 09:25:37 UTC
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

Comment 8 Siddharth Sharma 2016-06-08 04:52:48 UTC
I anyone looking to do fedora-review

Comment 9 Siddharth Sharma 2016-06-08 04:53:04 UTC
Is anyone looking to do fedora-review

Comment 10 Raphael Groner 2016-06-08 08:58:34 UTC
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.

Comment 11 Raphael Groner 2016-06-08 09:02:41 UTC
(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?

Comment 12 Siddharth Sharma 2016-06-10 08:14:49 UTC
(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

Comment 13 Raphael Groner 2016-06-10 14:24:08 UTC
Are you interested in a review swap? See comment #11.

Comment 14 Raphael Groner 2016-07-17 15:15:21 UTC
Are you interested in some review for mono packages to do a review swap?

Comment 15 Rex Dieter 2016-07-26 15:44:47 UTC
I can review today

Comment 16 Rex Dieter 2016-07-26 16:29:44 UTC
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).

Comment 17 Rex Dieter 2016-07-26 16:30:52 UTC
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

Comment 18 Siddharth Sharma 2016-07-26 17:51:31 UTC
(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.

Comment 19 Siddharth Sharma 2016-07-27 06:30:58 UTC
(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.

Comment 20 Rex Dieter 2016-07-27 18:02:17 UTC
Looks good, APPROVED

Comment 21 Gwyn Ciesla 2016-07-27 19:43:51 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/sqlcipher

Comment 23 Rex Dieter 2016-07-29 14:18:19 UTC
Are you planning on submitting updates for f24/f23 to bodhi?  (I don't see any yet)

Comment 24 Fedora Update System 2016-07-31 06:00:52 UTC
sqlcipher-3.3.1-4.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-068d457ba0

Comment 25 Fedora Update System 2016-07-31 06:01:31 UTC
sqlcipher-3.3.1-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-5ff49b651b

Comment 26 Fedora Update System 2016-08-01 20:54:25 UTC
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

Comment 27 Fedora Update System 2016-08-01 20:57:41 UTC
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

Comment 28 Sudhir Khanger 2017-01-07 14:53:21 UTC
Can this be closed as sqlcipher is already available now.

Comment 29 Rex Dieter 2017-01-07 15:18:02 UTC
Yes, closing


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