Bug 1264713 - Review Request: uchardet - An encoding detector library ported from Mozilla
Review Request: uchardet - An encoding detector library ported from Mozilla
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 1249416 uchardet (view as bug list)
Depends On:
Blocks: 1264715
  Show dependency treegraph
 
Reported: 2015-09-20 20:27 EDT by Ilya Gradina
Modified: 2016-03-16 03:17 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-15 21:24:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ignatenko: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Ilya Gradina 2015-09-20 20:27:27 EDT
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-20 21:39:30 EDT
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-20 21:41:52 EDT
*** Bug 1249416 has been marked as a duplicate of this bug. ***
Comment 3 Ilya Gradina 2015-09-21 00:06:48 EDT
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 00:18:23 EDT
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 Ilya Gradina 2015-09-21 00:27:15 EDT
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 02:22:44 EDT
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 Ilya Gradina 2015-09-21 03:35:51 EDT
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 Ilya Gradina 2015-09-30 01:29:37 EDT
Hi Ralf,

what are the next steps?
Comment 10 Ralf Corsepius 2015-09-30 03:14:51 EDT
(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 Ilya Gradina 2015-09-30 03:17:23 EDT
ok, Thx.
Comment 12 Ralf Corsepius 2015-09-30 11:28:17 EDT
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 Ilya Gradina 2015-10-10 15:28:27 EDT
Hi Ralf,

what are the next steps?)
Comment 15 Jehan 2015-11-19 10:14:28 EST
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 10:37:22 EST
(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 13:24:00 EST
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 10:25:16 EST
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 12:52:13 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/uchardet
Comment 22 MartinKG 2016-02-10 03:30:47 EST
*** Bug 1305991 has been marked as a duplicate of this bug. ***
Comment 23 Fedora Update System 2016-02-12 06:35:06 EST
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 00:24:55 EST
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-15 21:24:29 EDT
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.