Bug 1047647

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> - 1.0.2-2
- Added predefined macros to configure script in place of set paths

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>
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.

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>
>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

> (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.

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.

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*

> 2)

preferably use %find_lang --with-man and possibly --all-name

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 ?

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

Oh, and the manual pages belong into the -devel package.

Thanks Michael! New SRPM URL: http://breed808.com/rpmfusion-submission/libchardet-1.0.2-5.fc20.src.rpm

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

Any changes here?

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

Closing due long inactivity. Feel free to reopen if you want to continue.