Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 229154 - Review Request: konwert - Converter of character encodings
Summary: Review Request: konwert - Converter of character encodings
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2007-02-18 23:00 UTC by Daniil Ivanov
Modified: 2018-08-28 16:36 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-07-10 15:41:53 UTC


Attachments (Terms of Use)
mock build log of konwert-1.8-7.fc7 (deleted)
2007-02-19 17:40 UTC, Mamoru TASAKA
no flags Details
mock build log of konwert-1.8-7.fc7 (deleted)
2007-02-21 04:09 UTC, Daniil Ivanov
no flags Details
-11 spec file (some fixes added) (deleted)
2007-04-08 05:35 UTC, Mamoru TASAKA
no flags Details

Description Daniil Ivanov 2007-02-18 23:00:16 UTC
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 23:04:57 UTC
(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-19 01:38:06 UTC
i'll make a full review within a day in waitting you find out a sponsor.

Comment 3 Mamoru TASAKA 2007-02-19 13:35:26 UTC
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 16:02:09 UTC
Hi mamoru,

i'll within an hours (mock's working for)

Comment 5 Xavier Lamien 2007-02-19 16:12:07 UTC
Hi mamoru,

i'll within an hours (mock's working for)

Comment 6 Mamoru TASAKA 2007-02-19 17:40:54 UTC
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 23:37:28 UTC
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-21 04:09:13 UTC
Created attachment 148464 [details]
mock build log of konwert-1.8-7.fc7

Comment 9 Daniil Ivanov 2007-02-21 04:17:13 UTC
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-21 04:44:02 UTC
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 05:23:42 UTC
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 19:01:38 UTC
> %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 23:12:17 UTC
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-23 03:53:01 UTC
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-02 04:27:06 UTC
ping?

Comment 16 Daniil Ivanov 2007-03-02 09:29:48 UTC
Can't reproduce the problem on my machine.

~Daniil.

Comment 17 Mamoru TASAKA 2007-03-02 10:53:15 UTC
Xavier, would you review this review again?

Note: I cannot still approve this request.

Comment 18 Xavier Lamien 2007-03-02 17:25:41 UTC
Ok, i will

Comment 19 Xavier Lamien 2007-03-03 02:03:00 UTC
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 19:37:07 UTC
Mamoru: ping?

Can you approve this request now ?

Comment 21 Mamoru TASAKA 2007-03-14 19:56:34 UTC
(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 23:29:33 UTC
(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-15 03:54:07 UTC
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 18:31:50 UTC
Daniil: ping ?


Comment 25 Mamoru TASAKA 2007-03-28 11:37:05 UTC
Again, ping, Daniil?

Comment 26 Daniil Ivanov 2007-04-04 01:23:41 UTC
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 05:35:50 UTC
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 15:04:33 UTC
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 14:54:21 UTC
(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 16:36:52 UTC
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 08:03:01 UTC
ping?

Comment 34 Mamoru TASAKA 2007-05-18 18:44:54 UTC
Again ping?

Comment 35 Mamoru TASAKA 2007-05-28 13:45:32 UTC
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 11:41:47 UTC
Again? I will close this bug tomorrow.

Comment 37 Daniil Ivanov 2007-06-04 13:20:16 UTC
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 13:21:06 UTC
We can wait for the weekend after next weekend.

Comment 39 Mamoru TASAKA 2007-06-21 14:20:44 UTC
Again ping?

Comment 40 Mamoru TASAKA 2007-06-28 17:44:11 UTC
Again ping?

Comment 41 Mamoru TASAKA 2007-07-03 17:38:10 UTC
I will close this bug if no response from the reporter is
received within one week,

Comment 42 Mamoru TASAKA 2007-07-10 15:41:53 UTC
CLOSING.

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


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