[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647

Petr Pisar  changed:

   What|Removed |Added

   See Also||https://bugzilla.redhat.com
   ||/show_bug.cgi?id=1289634



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647

Miroslav Suchý msu...@redhat.com changed:

   What|Removed |Added

 Status|NEW |CLOSED
 CC||msu...@redhat.com
 Blocks|177841 (FE-NEEDSPONSOR) |201449 (FE-DEADREVIEW)
 Resolution|--- |DEFERRED
Last Closed||2015-07-21 10:57:18



--- Comment #15 from Miroslav Suchý msu...@redhat.com ---
Closing due long inactivity. Feel free to reopen if you want to continue.


Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=201449
[Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter
response should be blocking this bug.
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #14 from Sergio Monteiro Basto ser...@serjux.com ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #13 from Denis Fateyev de...@fateyev.com ---
Any changes here?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #12 from Michael Schwendt bugs.mich...@gmx.net ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #9 from Michael Schwendt bugs.mich...@gmx.net ---
Have you tried adding the suggested options?

--- libchardet.spec.orig2014-02-09 05:47:28.0 +0100
+++ libchardet.spec2014-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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #10 from Michael Schwendt bugs.mich...@gmx.net ---
Oh, and the manual pages belong into the -devel package.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #11 from Ben Reedy theben...@gmail.com ---
Thanks Michael! New SRPM URL:
http://breed808.com/rpmfusion-submission/libchardet-1.0.2-5.fc20.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #8 from Ben Reedy theben...@gmail.com ---
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 ?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647

Denis Fateyev de...@fateyev.com changed:

   What|Removed |Added

 CC||de...@fateyev.com



--- Comment #6 from Denis Fateyev de...@fateyev.com ---
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*

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #7 from Michael Schwendt bugs.mich...@gmx.net ---
 2)

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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #4 from MartinKG mgans...@alice.de ---
 (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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #5 from Michael Schwendt bugs.mich...@gmx.net ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647

Ben Reedy theben...@gmail.com changed:

   What|Removed |Added

 CC||theben...@gmail.com
 Blocks||177841 (FE-NEEDSPONSOR)




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #1 from MartinKG mgans...@alice.de ---
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 thebenj...@gmail.com - 1.0.2-2
- Added predefined macros to configure script in place of set paths

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647

Christopher Meng cicku...@gmail.com changed:

   What|Removed |Added

 CC||cicku...@gmail.com



--- Comment #2 from Christopher Meng cicku...@gmail.com ---
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 theben...@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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review

[Bug 1047647] Review Request: libchardet - Mozilla's Universal Charset Detector C/C++ API

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1047647



--- Comment #3 from Ben Reedy theben...@gmail.com ---
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 theben...@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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review