Bug 1264713 - Review Request: uchardet - An encoding detector library ported from Mozilla
Summary: Review Request: uchardet - An encoding detector library ported from Mozilla
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1249416 uchardet (view as bug list)
Depends On:
Blocks: 1264715
TreeView+ depends on / blocked
 
Reported: 2015-09-21 00:27 UTC by Ilia Gradina
Modified: 2016-03-16 07:17 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-03-16 01:24:33 UTC
Type: ---
Embargoed:
ignatenko: fedora-review+


Attachments (Terms of Use)

Description Ilia Gradina 2015-09-21 00:27:27 UTC
Spec URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet.spec
SRPM URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet-0.0.1-1.fc24.src.rpm
Description: is an encoding detector library, which takes a sequence of bytes
in an unknown character encoding without any additional information,
and attempts to determine the encoding of the text.
Fedora Account System Username: ilgrad

Comment 1 Christopher Meng 2015-09-21 01:39:30 UTC
1. IMO 3 packages needed: uchardet(bin only), libuchardet(or uchardet-libs), libuchardet-devel(or uchardet-libs-devel).

2. %doc COPYING -> %license COPYING

3. No one from upstream allows you to set version as 0.0.1, use 0.0.0 instead.

4. %description
is an encoding detector library, which takes a sequence of bytes
in an unknown character encoding without any additional information,
and attempts to determine the encoding of the text.

Sorry, I can't parse the subject.

5. No static lib please.

Comment 2 Christopher Meng 2015-09-21 01:41:52 UTC
*** Bug 1249416 has been marked as a duplicate of this bug. ***

Comment 3 Ilia Gradina 2015-09-21 04:06:48 UTC
SPEC URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet.spec
SRPM URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet-0.0.0-2.fc24.src.rpm

1. fix number packages
2. fix license path
3. fix version
4. fix description
5. exclude static lib

Comment 4 Christopher Meng 2015-09-21 04:18:23 UTC
Oh, but libs %description is not good, you should check and see how other $NAME-libs packages write.

Also, reason of %global debug_package %{nil} is unclear to me, or just a mistake?

Comment 5 Ilia Gradina 2015-09-21 04:27:15 UTC
This command %global debug_package %{nil} I disable debug packages, you wrote the uchardet(bin only). (or I misunderstood?)

Comment 7 Ralf Corsepius 2015-09-21 06:22:44 UTC
This package has quite a few issues.

- Enable building debug packages.
i.e. remove %global debug_package %{nil}

- Making is non-verbose.
Please append V=1 to the make-call, i.e. 
make %{?_smp_mflags} V=1

- libdir is not being honored:
One way to achieve this, is this
%cmake -DCMAKE_INSTALL_LIBDIR=%{_libdir}
and to remove
mv %{buildroot}/usr/lib %{buildroot}%{_libdir}

- As you have split the package into a main package (uchardet) and a *-libs package, the main should not contain the libs (they already are in *-libs).
Please remove
%{_libdir}/lib%{name}.so.*
from the main-package's %files

- The libs are contained in *-libs. 
=> The %post/%postun scriptlets need to be added to the *-libs package and not to the main package.

Please change %post/%postun into
%post libs -p /sbin/ldconfig
%postun libs -p /sbin/ldconfig

- The package internally uses 0.0.1 as version number and not 0.0.0:
cf. CMakeLists.txt:
...
set (UCHARDET_VERSION_MAJOR 0)
set (UCHARDET_VERSION_MINOR 0)
set (UCHARDET_VERSION_REVISION 1)
...

Change your package version accordingly.

- There is a bug in upstream's PACKAGE_NAME.
cf. CMakeLists.txt:
set (PACKAGE_NAME opencc)
This likely should be "uchardet"

This doesn't seem to have an impact on the package, but please consider contacting upstream about it.

Comment 8 Ilia Gradina 2015-09-21 07:35:51 UTC
SPEC URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet.spec
SRPM URL: https://github.com/ilgrad/fedora-packages/raw/master/flacon/uchardet-0.0.1-4.git84e292d.fc24.src.rpm

- fix enable debug packages
- fix add flag verbose for make
- fix change in build
- fix remove in libs from files
- fix add change for libs in post/postun
- fix version on 0.0.1 from git
- added macros

Package opencc and uchardet are different.

Comment 9 Ilia Gradina 2015-09-30 05:29:37 UTC
Hi Ralf,

what are the next steps?

Comment 10 Ralf Corsepius 2015-09-30 07:14:51 UTC
(In reply to Ilya Gradina from comment #9)
> what are the next steps?
Me to continue ;) 

Sorry, this review had dropped off my radar - I'll try to have a look into it today or tomorrow.

Comment 11 Ilia Gradina 2015-09-30 07:17:23 UTC
ok, Thx.

Comment 12 Ralf Corsepius 2015-09-30 15:28:17 UTC
I am basically OK with this package, with 2 exceptions:

- %{sum}
I don't see any need for this define. IMO, all it does is to decrease readability. Please remove it.

-  %{srcname}
Simiarly to %{sum} I don't see any need for this define.

Worse, it is being used at places it doesn't make sense.

E.g. programs or libraries won't change their names just because the tarball changed its name.

Comment 14 Ilia Gradina 2015-10-10 19:28:27 UTC
Hi Ralf,

what are the next steps?)

Comment 15 Jehan 2015-11-19 15:14:28 UTC
Hello!

I'm the new co-maintainer of uchardet. I have fixed a few things, and improved the build (with tests, and such). And in particular I have made sure that all the returned charsets are iconv-compatible (they already mostly were but 2), which is probably one of the most interesting change for using software.

The last release is version 0.0.3: https://github.com/BYVoid/uchardet/releases/tag/v0.0.3

Would it be possible to use this version for the package rather than the random git commit from your spec?
More software are using it now (mpv for instance, but also gtksourceview is going to add it as a dependency, hence gedit, etc.) and I would welcome having a stable version in the repository.
Thanks!

Comment 16 Ralf Corsepius 2015-11-20 15:37:22 UTC
(In reply to Jehan from comment #15)
> Would it be possible to use this version for the package rather than the
> random git commit from your spec?

Ilya, any update?

Comment 18 Upstream Release Monitoring 2015-11-20 18:24:00 UTC
ilgrad's scratch build of uchardet-0.0.3-1.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11923000

Comment 20 Igor Gnatenko 2016-02-06 15:25:16 UTC
LGTM.

Rpmlint shows only-binary-in-usr-lib, but looks like it is a bug in rpmlint.

APPROVED.

Comment 21 Gwyn Ciesla 2016-02-06 17:52:13 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/uchardet

Comment 22 MartinKG 2016-02-10 08:30:47 UTC
*** Bug 1305991 has been marked as a duplicate of this bug. ***

Comment 23 Fedora Update System 2016-02-12 11:35:06 UTC
uchardet-0.0.5-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8a7ae72e2d

Comment 24 Fedora Update System 2016-02-15 05:24:55 UTC
uchardet-0.0.5-3.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-8a7ae72e2d

Comment 25 Fedora Update System 2016-03-16 01:24:29 UTC
uchardet-0.0.5-3.fc23 has been pushed to the Fedora 23 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.