Bug 1047647 - Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API
Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API
Status: CLOSED DEFERRED
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
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2014-01-01 09:10 EST by Ben Reedy
Modified: 2015-12-09 03:46 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-07-21 10:57:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ben Reedy 2014-01-01 09:10:42 EST
Spec URL: http://breed808.com/rpmfusion-submission/libchardet.spec
SRPM URL: http://breed808.com/rpmfusion-submission/libchardet-1.0.2-1.fc20.src.rpm
Description: Mozilla's Universal Charset Detector C/C++ API
Fedora Account System Username: breed808

rpmlint -i libchardet-1.0.2-1.fc20.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint -i libchardet-1.0.2-1.fc20.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint -i libchardet-devel-1.0.2-1.fc20.x86_64.rpm
libchardet-devel.x86_64: W: no-manual-page-for-binary chardet-config
Each executable in standard binary directories should have a man page.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Hello! This is my first time submitting a package for review, so please let me know if I've made any mistakes.
Comment 1 MartinKG 2014-01-01 10:31:33 EST
Please insert each time you make a change an entry in the changelog and increase also the Release number. + post the %changelog.
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

necessary changes in the spec file:
Release:		2%{dist}

* Wed Jan 01 2014 Ben Reedy <thebenj808@gmail.com> - 1.0.2-2
- Added predefined macros to configure script in place of set paths
Comment 2 Christopher Meng 2014-01-01 13:55:03 EST
A dep of cmplayer, right?

I can't sponsor you, but some thoughts:

1. Group tag is not a MUST HAVE tag now, you can drop it on your own.

2. These 2 lines are not needed:

Packager:		Ben Reedy <thebenj88@gmail.com>
BuildRequires:	gcc-c++

3. Description and summary are the same, that's bad. Please improve.

4. Use macro:

%configure

instead of 

./configure --prefix=%{_prefix} --sysconfdir=%{_sysconfdir} --libdir=%{_libdir} \
	--mandir=%{_mandir} --disable-static

5. %files
# Libraries
%{_libdir}/%{name}.so.*
# Man pages
%{_mandir}/ko/man3/*
# We don't want the libtool archive
%exclude %{_libdir}/%{name}.la

%files devel
#Binary
%{_bindir}/chardet-config
# Header files
%{_includedir}/chardet/
# Development library
%{_libdir}/%{name}.so
# Documentation
%doc README LICENSE Changelog

Why did you add so many comments here? I don't think you need to write down "this part below is Binary" as you are the packager, you MUST know their usage, and we reviewer know them certainly as well.
Comment 3 Ben Reedy 2014-01-01 19:46:26 EST
spec file is in the same location
SRPM is located at http://breed808.com/rpmfusion-submission/libchardet-1.0.2-3.fc20.src.rpm

(In reply to Christopher Meng from comment #2)

Thank you for the help, Christopher.

>1. Group tag is not a MUST HAVE tag now, you can drop it on your own.
>
>2. These 2 lines are not needed:
>
>Packager:		Ben Reedy <thebenj88@gmail.com>
>BuildRequires:	gcc-c++

Removed.

> 3. Description and summary are the same, that's bad. Please improve.

I've updated the description, though it is fairly short. I've had difficulty finding a valuable description for libchardet.
 
> 4. Use macro:
> 
> %configure
> 
> instead of 
> 
> ./configure --prefix=%{_prefix} --sysconfdir=%{_sysconfdir}
> --libdir=%{_libdir} \
> 	--mandir=%{_mandir} --disable-static

Is it ok that I've added the '--disable-static' flag after the %configure macro?

> Why did you add so many comments here? I don't think you need to write down
> "this part below is Binary" as you are the packager, you MUST know their
> usage, and we reviewer know them certainly as well.

Agreed, I should have removed these some time ago.


(In reply to MartinKG from comment #1)
> Please insert each time you make a change an entry in the changelog and
> increase also the Release number. + post the %changelog.
> https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

Hi Martin, I've updated the changelog and release number.
One question: the changelog guidelines mention that if a package has been updated but not built, the package's Release does not need to be incremented and a new entry can be added to the changelog. Does that apply for packages under review?
https://fedoraproject.org/wiki/Packaging:Guidelines#Repeat_the_old_version_release_with_a_new_entry
Comment 4 MartinKG 2014-01-02 04:06:19 EST
> (In reply to MartinKG from comment #1)
> > Please insert each time you make a change an entry in the changelog and
> > increase also the Release number. + post the %changelog.
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
> 
> Hi Martin, I've updated the changelog and release number.
> One question: the changelog guidelines mention that if a package has been
> updated but not built, the package's Release does not need to be incremented
> and a new entry can be added to the changelog. Does that apply for packages
> under review?
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Repeat_the_old_version_release_with_a_new_entry

I think this applies also to a review.
Comment 5 Michael Schwendt 2014-01-02 09:17:01 EST
It's a matter of taste.

In general, increasing %release with every change that results in a new src.rpm build is good practice also during review:

  https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes

The new src.rpm filename will make it more convenient to run rpmdiff and rpmdev-diff.
Comment 6 Denis Fateyev 2014-02-08 11:11:54 EST
Planned to package "libchardet" myself, but incidentally found that the review request already exists ;-) Small fixes here:

1) Use "%{?_smp_mflags}" with 'make': https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make

2) Use "%lang" for localized manpages in "%files" section.
 "%lang(ko) %{_mandir}/ko/man3/*.3*
Comment 7 Michael Schwendt 2014-02-08 11:58:20 EST
> 2)

preferably use %find_lang --with-man and possibly --all-name
Comment 8 Ben Reedy 2014-02-09 21:34:29 EST
Thanks for the help Denis.

New URL for the SRPM: http://breed808.com/rpmfusion-submission/libchardet-1.0.2-4.fc20.src.rpm

Michael, I'm not sure how to get %find_lang to work in this situation. As far as I know, %find_lang uses a string (usually %{name}), but the manpages for libchardet have various names (detect.3.gz, detect_destroy.3.gz, detect_init.3.gz, etc). Is it possible to pass a wildcard to %find_lang ?
Comment 9 Michael Schwendt 2014-02-10 04:32:08 EST
Have you tried adding the suggested options?

--- libchardet.spec.orig	2014-02-09 05:47:28.000000000 +0100
+++ libchardet.spec	2014-02-10 10:30:49.024562542 +0100
@@ -34,14 +34,14 @@
 
 %install
 make DESTDIR=%{buildroot} install
+%find_lang %{name} --with-man --all-name
 
 %post -p /sbin/ldconfig
 
 %postun -p /sbin/ldconfig
 
-%files
+%files -f %{name}.lang
 %{_libdir}/%{name}.so.*
-%lang(ko) %{_mandir}/ko/man3/*.3*
 %exclude %{_libdir}/%{name}.la
 
 %files devel
Comment 10 Michael Schwendt 2014-02-10 04:54:58 EST
Oh, and the manual pages belong into the -devel package.
Comment 11 Ben Reedy 2014-02-10 07:22:09 EST
Thanks Michael! New SRPM URL: http://breed808.com/rpmfusion-submission/libchardet-1.0.2-5.fc20.src.rpm
Comment 12 Michael Schwendt 2014-02-11 07:04:58 EST
Good. If there will ever be translation files for the library, you may need to "grep" from the %name.lang file to extract what to include in the individual subpackages.

[...]

Issues with /usr/bin/chardet-config as packaged:

* It sources the libtool archive (.la) file to define some variables, such as "libdir", and fails:

  /usr/bin/chardet-config: line 20: /usr/lib64/libchardet.la: No such file or directory

* For its --libs option, it relies on libtool archive inter-dependencies instead of only returning -lchardet. Inter-deps in .la files are poisonous and a primary reason why those files are not included in Fedora packages.

* For its --cflags option it prints the compiler flags that have been used to build libchardet. That is a mistake. Or to put it differently, since there's also a --defs option for printing preprocessor options, what --cflags prints is questionable. It should only print any _added_ flags that would be mandatory when compiling _with_ the libchardet API.

* Option --defs returning -I/usr/include/chardet bears a risk, because it alters search path for headers and unhides generic header names from the chardet directory, such as "version.h". On the long term, it would be better, if the API were to be used via "#include <chardet/chardet.h>" in _standard_ search path instead of "#include <chardet.h>" in customized search path.

* /usr/bin/chardet-config will conflict on multi-arch installations (e.g. x86_64) due to its hardcoded /usr/lib64 path components. While the guidelines are not too specific on multiarch conflicts, Fedora has been trying to resolve also such conflicts for several years.
https://fedoraproject.org/wiki/Packaging:Guidelines#Conflicts

[...]

* The license text must be included in the base package (and the -devel package depends on that one).
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> Release:		5%{dist}

More accurately, it's %{?dist} which allows for the %dist macro being undefined.

https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Using_the_.25.7B.3Fdist.7D_Tag
Comment 13 Denis Fateyev 2014-07-06 17:59:47 EDT
Any changes here?
Comment 14 Sergio Monteiro Basto 2015-06-23 23:38:36 EDT
Just FYI I found : jchardet, juniversalchardet in python-chardet in fedora repos, 2 java and 1 python implementation of the encoding detector library of Mozilla
Comment 15 Miroslav Suchý 2015-07-21 10:57:18 EDT
Closing due long inactivity. Feel free to reopen if you want to continue.

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