Bug 229154

Summary: Review Request: konwert - Converter of character encodings
Product: [Fedora] Fedora Reporter: Daniil Ivanov <daniil.ivanov>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NOTABUG QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: lxtnow, mtasaka
Target Milestone: ---Keywords: Reopened
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-07-10 11:41:53 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    
Attachments:
Description Flags
mock build log of konwert-1.8-7.fc7
none
mock build log of konwert-1.8-7.fc7
none
-11 spec file (some fixes added) none

Description Daniil Ivanov 2007-02-18 18:00:16 EST
Spec URL: http://users.jyu.fi/~divanov/devel/
SRPM URL: http://users.jyu.fi/~divanov/devel/konwert-1.8-7.src.rpm
Description: Konwert is a charset converter similar to iconv. Available features
include one-to-many conversions, context-dependent conversions,
approximations of some unavailable characters. 'filterm' applies
filter conversion to a terminal's I/O, to get on-the-fly charset
conversion, and customized input methods.

rpmlint -i -v konwert-1.8-7.src.rpm 
I: konwert checking

This is my first package, and I am seeking a sponsor.
Comment 1 Daniil Ivanov 2007-02-18 18:04:57 EST
(In reply to comment #0)
> Spec URL: http://users.jyu.fi/~divanov/devel/
> SRPM URL: http://users.jyu.fi/~divanov/devel/konwert-1.8-7.src.rpm
> Description: Konwert is a charset converter similar to iconv. Available features
> include one-to-many conversions, context-dependent conversions,
> approximations of some unavailable characters. 'filterm' applies
> filter conversion to a terminal's I/O, to get on-the-fly charset
> conversion, and customized input methods.
> 
> rpmlint -i -v konwert-1.8-7.src.rpm 
> I: konwert checking
> 
> This is my first package, and I am seeking a sponsor.

Sorry,
Spec URL: http://users.jyu.fi/~divanov/devel/konwert.spec
Comment 2 Xavier Lamien 2007-02-18 20:38:06 EST
i'll make a full review within a day in waitting you find out a sponsor.
Comment 3 Mamoru TASAKA 2007-02-19 08:35:26 EST
Xavier, please review this request if you like (and
mark this as FE-REVIEW)

As this request is sponsor needed issue and you are not
a sponsor, a sponsor must review this finally. If the time
comes, I will do a final check and do some needed procedure
for sponsorship issue as one of the sponsors.
Comment 4 Xavier Lamien 2007-02-19 11:02:09 EST
Hi mamoru,

i'll within an hours (mock's working for)
Comment 5 Xavier Lamien 2007-02-19 11:12:07 EST
Hi mamoru,

i'll within an hours (mock's working for)
Comment 6 Mamoru TASAKA 2007-02-19 12:40:54 EST
Created attachment 148349 [details]
mock build log of konwert-1.8-7.fc7

Xavier, if you want to check mock build log of me,
please check the attached (on FC-devel i386)

Well, this package seems to have some big and minor
issues, however I leave this to you for now.
Comment 7 Xavier Lamien 2007-02-19 18:37:28 EST
Indeed,

There's some things which should be fix.
-------------------------------
spec file:
-------------------------------
 ** BuildRequires

 - BR perl is useless as it requires by default installed package (such as 
xorg-x11-server-Xorg)

 ** %package Devel

 - Requires: konwert SHOULD be : requires:  %{name} = %{version}-%{release}
 - Group should be Development/Tools

 ** %description devel

 -  SHOULD only describe contains about -devel package, not   
  main package.

 ** %build

 - OPTFLAGS="$RPM_OPT_FLAGS %{!?debug:-fomit-frame-pointer}" can be remove.
   Build and work well without this.
 - in this case, use OPTFLAGS="$RPM_OPT_FLAGS -fno-rtti -fno-exceptions \   
   -fno-implicit-templates" instead of
 - use make instead %{_make}.

 ** %install 

 - use make install instead of %{_make} install.
 - The use of : prefix=$RPM_BUILD_ROOT%{_prefix} \
	        mandir=$RPM_BUILD_ROOT%{_mandir} \
	        mydocdir=$RPM_BUILD_ROOT%{_docdir}/konwert-%{version} \
	        perl=%{_bindir}/perl \
	        libdir=$RPM_BUILD_ROOT%{_libdir} \
   sound good and work except mydocdir= (see below for more explaination).
   after have a look on build.log (and Makefile), i can see the use of "sed"
   to change default location (which match with fedora install location) 
   such as /usr/share/ by /usr/local/share).
   Can be fix by patching Makefile but require more working time.
 - the use of "dontfixmanconfig=1", check if it's necessary and comment why.
 - keep timestamps on make install by addind "INSTALL=install -p"

** %post

 - Doesn't look good.

** %files and %files devel

 - doesn't sound good at all.
 - The use of "%{_docdir}/konwert-%{version}/en/*" is deprecated and must be drop.
   All doc MUST be set in %doc just after %defattr macros.
 - the use of %attr(755,root,root):
   Personnaly, I prefere use chmod command in %install section and comment the
reason of.
 - You package doesn't own the %{_datadir}/%{name}.
 - The use of %lang(p1) is useless
 - some files mentioned twice.
 - %{_mandir}/man1 instead of %{_mandir}/man*/*
 - You don't need to mentione explicitly all files, just the use of main
location is enough.
   such as %{_datadir}/%{name} (and own in the same time)

Spec file doesn't meet the packeging Guidlines, see
http://fedoraproject.org/wiki/Packaging/Guidelines.

--------------------------
 Rpmlint output
--------------------------

 ** From srpm file: clean.

 ** From main rpm package:

   W: konwert file-not-utf8 /usr/share/man/man1/filterm.1.gz
   W: konwert file-not-utf8 /usr/share/man/pl/man1/trs.1.gz
   W: konwert file-not-utf8 /usr/share/man/pl/man1/filterm.1.gz
   W: konwert file-not-utf8 /usr/share/man/pl/man1/konwert.1.gz
   W: konwert file-not-utf8 /usr/share/man/man1/konwert.1.gz

 - Can be fix by the use of iconv command in %prep or %install.

   W: konwert one-line-command-in-%post /usr/share/konwert/aux/fixmanconfig
 
 - As i mentioned above, this is not quite good.


** From -devel rpm package: clean.

** From -debuginfos rpm package:

  E: konwert-debuginfo empty-debuginfo-package

 - this package contains no files.
   rpmbuild not being able to strip the binaries.


If i forgot to mentione something, i will post

FE-REVIEW
Comment 8 Daniil Ivanov 2007-02-20 23:09:13 EST
Created attachment 148464 [details]
mock build log of konwert-1.8-7.fc7
Comment 9 Daniil Ivanov 2007-02-20 23:17:13 EST
Hi,

 attempt #2
Spec URL: http://users.jyu.fi/~divanov/devel/konwert.spec
SRPM URL: http://users.jyu.fi/~divanov/devel/konwert-1.8-7.fc7.src.rpm

rpmlint -iv konwert-1.8-7.fc7.x86_64.rpm 
I: konwert checking

rpmlint -i konwert-1.8-7.fc7.src.rpm 
I: konwert checking

rpmlint -i konwert-debuginfo-1.8-7.fc7.x86_64.rpm 
I: konwert-debuginfo checking
E: konwert-debuginfo empty-debuginfo-package

rpmlint -i konwert-devel-1.8-7.fc7.x86_64.rpm 
I: konwert-devel checking
Comment 10 Mamoru TASAKA 2007-02-20 23:44:02 EST
Please increment release number every time you change/fix
your spec/srpm. Changing spec/srpm without changing release
number causes confusion on the people who are watching your
spec/srpm.
Comment 11 Daniil Ivanov 2007-02-21 00:23:42 EST
Sorry, I increased the relesed number

Spec URL: http://users.jyu.fi/~divanov/devel/konwert.spec
SRPM URL: http://users.jyu.fi/~divanov/devel/konwert-1.8-8.fc7.src.rpm
Comment 12 Xavier Lamien 2007-02-21 14:01:38 EST
> %exclude %{_datadir}/konwert/devel
> %exclude %{_libdir}/konwert/devel

Should be put in "%files devel" without "%exclude" macros.

rpmlint output:

form srpm:

W: konwert mixed-use-of-spaces-and-tabs (spaces: line 68, tab: line 1)

easy fix ;-)
Comment 13 Daniil Ivanov 2007-02-21 18:12:17 EST
Hi,

 attempt #3
 http://users.jyu.fi/~divanov/devel/konwert.spec
 http://users.jyu.fi/~divanov/devel/konwert-1.8-9.fc7.src.rpm

 Xavier, how did you get warning about the spaces/tabs?
 rpmlint didn't give it for me. should I use some policy?

Thanks, Daniil.
Comment 14 Xavier Lamien 2007-02-22 22:53:01 EST
you should use only tabs in your spec file.
Move your forward-key just after your "sharedir=$RPM_BUILD_ROOT%{_datadir} \"
you will see.
Vim is your friend... ;-)

Spec file and srpm don't look to be fix for last comment #12.
Comment 15 Mamoru TASAKA 2007-03-01 23:27:06 EST
ping?
Comment 16 Daniil Ivanov 2007-03-02 04:29:48 EST
Can't reproduce the problem on my machine.

~Daniil.
Comment 17 Mamoru TASAKA 2007-03-02 05:53:15 EST
Xavier, would you review this review again?

Note: I cannot still approve this request.
Comment 18 Xavier Lamien 2007-03-02 12:25:41 EST
Ok, i will
Comment 19 Xavier Lamien 2007-03-02 21:03:00 EST
OK - Mock Build on FC-6 FC-Devel (i386,x86_64)
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License is GPL
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum for both:
0a1dcb0fa7a1990980aba8ab9a4c3184  konwert-1.8.tar.gz
962c3d339b99656bb37a25c39d7d3dca  konwert-forbids_data_member.patch
OK - Package has correct buildroot.
OK - BuildRequires isn't required.
OK - Subpackage -devel is present.
OK - %prep stage is correct and work.
OK - The use of iconv is proper.
OK - %build and %install stages is correct and work.
OK - No *.la files is present in -devel subpackage.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - rpmlint output are clean.

OK - Should function as described.
OK - Should package latest version
OK - Should have %{?dist} tag but, should be remove to avoid trouble in CVS
import/Update/Build procedure.



Comment 20 Xavier Lamien 2007-03-14 15:37:07 EDT
Mamoru: ping?

Can you approve this request now ?
Comment 21 Mamoru TASAKA 2007-03-14 15:56:34 EDT
(In reply to comment #20)
> Can you approve this request now ?

Well, are you referring to 1.8-9?
If so, take my comment 17 as "there is a issue to be fixed
on this package before I can approve".

I have not checked this package precisely, however at least
------------------------------------------------
E: konwert-debuginfo empty-debuginfo-package
------------------------------------------------
cannot be ignored. Mock build log shows:
------------------------------------------------
install -p -m755 -s install/bin/trs    
/var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
install -p -m755    install/bin/konwert
/var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
install -p -m755 -s install/bin/filterm
/var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
------------------------------------------------
and this should be fixed (at least).
Comment 22 Xavier Lamien 2007-03-14 19:29:33 EDT
(In reply to comment #21)
> (In reply to comment #20)
> > Can you approve this request now ?
> 
> Well, are you referring to 1.8-9?
> If so, take my comment 17 as "there is a issue to be fixed
> on this package before I can approve".
> 
> I have not checked this package precisely, however at least
> ------------------------------------------------
> E: konwert-debuginfo empty-debuginfo-package
> ------------------------------------------------

 Can be fix by disabling the debuginfo

> cannot be ignored. Mock build log shows:
> ------------------------------------------------
> install -p -m755 -s install/bin/trs    
> /var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
> install -p -m755    install/bin/konwert
> /var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
> install -p -m755 -s install/bin/filterm
> /var/tmp/konwert-1.8-9.fc7-root-mockbuild/usr/bin
> ------------------------------------------------
> and this should be fixed (at least).

You right.
These files should be installed in 0644 mode.
Comment 23 Mamoru TASAKA 2007-03-14 23:54:07 EDT
Well, no..

These are executable binaries or scripts, so the permission
should be 0755.
What is wrong here is that 
--------------------------------------------
install -p -m755 "-s"
--------------------------------------------
means that this binary is stripped on this procedure, which
make it impossible to create debuginfo rpm.
Comment 24 Xavier Lamien 2007-03-19 14:31:50 EDT
Daniil: ping ?
Comment 25 Mamoru TASAKA 2007-03-28 07:37:05 EDT
Again, ping, Daniil?
Comment 26 Daniil Ivanov 2007-04-03 21:23:41 EDT
Hi,

something like this?
http://users.jyu.fi/~divanov/devel/konwert-1.8-10.src.rpm
http://users.jyu.fi/~divanov/devel/konwert.spec

Thanks, Daniil.
Comment 28 Mamoru TASAKA 2007-04-08 01:35:50 EDT
Created attachment 151926 [details]
-11 spec file (some fixes added)

Well, as this is a NEEDSPONSOR ticket, I do a full review
for this package.

For -10:
* Timestamps
  - Well, actually 'INSTALL="install -p"' in %install stage does nothing, as
    all files installed in "INSTALL" macro are modified beforhand
    during the build stage.

    However, there are many files under %{_datadir}/%{name},
    of which the timestamps should
    be kept as they are not modified during build stage. This should
    be done by modifying Makefile directly.

* Lang file
  - All _documentation_ other than in English should be marked as
    %%lang(...).


The spec file attached should fix all the issues I mentioned above.
Please this the attached. If you agree with my spec file, I can
say okay for this package.

Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora Extras package review requests which are waiting for someone to
review can be checked on:
https://bugzilla.redhat.com/bugzilla/buglist.cgi?cmdtype=runnamed&namedcmd=mtasaka-review-noone

NOTE: FE-NEW blockers are now not complete.

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 29 Xavier Lamien 2007-04-17 11:04:33 EDT
this bug should be assigned and approuved by a sponsor, removed myself from
assignee block.

Daniil, do you have a updated srpm fix from Mamoru review ?
Comment 30 Daniil Ivanov 2007-04-22 10:54:21 EDT
(In reply to comment #28)
I agree with your changes. Also I agree with you that I don't have an
understanding of the process and of the packaging guidelines.
Comment 32 Mamoru TASAKA 2007-04-22 12:36:52 EDT
Well, spec/srpm is okay (actually it is what I uploaded).
Then:
* would you do a "pre-review" of a review request you choose
  from the following?
https://bugzilla.redhat.com/bugzilla/buglist.cgi?cmdtype=runnamed&namedcmd=mtasaka-review-noone
* Or do you have a plan to submit another review request
  or do you already have another review request?
Comment 33 Mamoru TASAKA 2007-05-04 04:03:01 EDT
ping?
Comment 34 Mamoru TASAKA 2007-05-18 14:44:54 EDT
Again ping?
Comment 35 Mamoru TASAKA 2007-05-28 09:45:32 EDT
I will close this bug as NOTABUG if no response is
received on this bug within ONE WEEK
Comment 36 Mamoru TASAKA 2007-06-04 07:41:47 EDT
Again? I will close this bug tomorrow.
Comment 37 Daniil Ivanov 2007-06-04 09:20:16 EDT
I'm really short on time on working days and the next weekend I again must
travel, so I won't have access to pc/net. I have nothing to do with that.
Comment 38 Daniil Ivanov 2007-06-04 09:21:06 EDT
We can wait for the weekend after next weekend.
Comment 39 Mamoru TASAKA 2007-06-21 10:20:44 EDT
Again ping?
Comment 40 Mamoru TASAKA 2007-06-28 13:44:11 EDT
Again ping?
Comment 41 Mamoru TASAKA 2007-07-03 13:38:10 EDT
I will close this bug if no response from the reporter is
received within one week,
Comment 42 Mamoru TASAKA 2007-07-10 11:41:53 EDT
CLOSING.

If someone wants to import this into Fedora, please file
a new bug report, thank you!