Bug 226200

preliminary review ->
rpmlint complained
nkf.src:29: W: rpm-buildroot-usage %prep %{__rm} -rf    $RPM_BUILD_ROOT
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it
will break short circuiting.

nkf.src:30: W: rpm-buildroot-usage %prep %{__mkdir_p}   $RPM_BUILD_ROOT/%{_bindir}
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it
will break short circuiting.

nkf.src:31: W: rpm-buildroot-usage %prep %{__mkdir_p}  
$RPM_BUILD_ROOT%{_mandir}/{man1,ja/man1}
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it
will break short circuiting.

nkf.src:39: W: rpm-buildroot-usage %build CFLAGS="$RPM_OPT_FLAGS" perl
Makefile.PL PREFIX=$RPM_BUILD_ROOT%{_prefix} INSTALLDIRS=vendor
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it
will break short circuiting.

===> Remove %prep section lines add them to %build

nkf.src: W: summary-ended-with-dot A Kanji code conversion filter.
Summary ends with a dot.

perl-NKF.i386: W: hidden-file-or-dir
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/NKF/.packlist
The file or directory is hidden. You should see if this is normal,
and delete it from the package if not.

perl-NKF.i386: W: perl-temp-file
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/NKF/.packlist
You have a perl temporary file in your package. Usually, this
file is beginning with a dot (.) and contain "perl" in its name.

perl-NKF.i386: E: non-standard-executable-perm
/usr/lib/perl5/vendor_perl/5.8.8/i386-linux-thread-multi/auto/NKF/NKF.so 0555
A standard executable should have permission set to 0755. If you get this
message, it means that you have a wrong executable permissions in some files
included in your package.

nkf.src: W: no-url-tag
The URL tag is missing.


SHOULD:
  Add missing BR perl-ExtUtils-MakeMaker
  Preserve timestamps
  clean buildroot in install stage

Thanks for reviewing. the below should works:

Spec file: http://tagoh.fedorapeople.org/nkf/nkf.spec
SRPM file: http://tagoh.fedorapeople.org/nkf/nkf-2.0.8b-1.fc8.src.rpm

thanks.
source URL though not worked with wget but worked in browser. I guess it should
be problem.

In reply to comment #3)
> thanks.
> source URL though not worked with wget but worked in browser. I guess it should
> be problem.
> 
$ wget -N http://downloads.sourceforge.jp/nkf/26243/nkf-2.0.8b.tar.gz

should work.

yes source URL working fine. looks I got some problem. its working fine.
Review:
+ package builds in mock (development i386).
+ rpmlint is silent for SRPM and RPM.
+ source files match upstream.
1851260a2719629294740783c14ca3d5  nkf-2.0.8b.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.
+ License text is included in package.
+ %doc files present.
+ BuildRequires are proper.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code.
+ no static libraries.
+ no .pc file present.
+ no -devel subpackage exists.
+ no .la files.
+ no translations are available.
+ Does owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.
+ no scriptlets are used.
+ Pacakge nkf-2.0.8b-1.fc8 ->
  Requires: libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1) rtld(GNU_HASH)
  Package perl-NKF-2.0.8b-1.fc8 ->
  Provides: NKF.so perl(NKF) = 2.08
  Requires: libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1.3)
libc.so.6(GLIBC_2.3.4) libc.so.6(GLIBC_2.4) perl(DynaLoader) perl(Exporter)
perl(strict) perl(vars) rtld(GNU_HASH)
+ Not a GUI app.
APPROVED.

Sorry, but please wait.

* Compilation does not honor %optflags
  http://koji.fedoraproject.org/koji/getfile?taskID=168881&name=build.log
------------------------------------------------------
+ make nkf
cc -O -c utf8tbl.c
cc -O -o nkf nkf.c utf8tbl.o
------------------------------------------------------

* perl macro
  - Please use %perl_vendorarch or %perl_vendorlib

* Timestamp
  - For SOURCE3, please use "cp -p" to keep timestamp.

* defattr
  - Now we recommend %defattr(-,root,root,-)

* Using direct command vs using macro command
  - Please choose if you use write commands directly, or
    use macros as much as possible.

    For example, if you want to use %__install_p, you also
    want to use %__rm, %__cp, %__make, %__chmod.

(In reply to comment #6)
> Sorry, but please wait.
> 
> * Compilation does not honor %optflags
>   http://koji.fedoraproject.org/koji/getfile?taskID=168881&name=build.log
> ------------------------------------------------------
> + make nkf
> cc -O -c utf8tbl.c
> cc -O -o nkf nkf.c utf8tbl.o
> ------------------------------------------------------

  use make CFLAGS="$RPM_OPT_FLAGS"  nkf

> 
> * perl macro
>   - Please use %perl_vendorarch or %perl_vendorlib
  ohh I missed to check perl macors.
> 
> * Timestamp
>   - For SOURCE3, please use "cp -p" to keep timestamp.
   yes. this is still missing in SPEC.
> 
> * defattr
>   - Now we recommend %defattr(-,root,root,-)
   I am still unclear on usage of this as I guess some packagers still prefer
%defattr(-,root,root) bu tI have seen you always asking packagers to use
%defattr(-,root,root,-) but can't find this thing written in packaging guidelines.
> 
> * Using direct command vs using macro command
>   - Please choose if you use write commands directly, or
>     use macros as much as possible.
> 
>     For example, if you want to use %__install_p, you also
>     want to use %__rm, %__cp, %__make, %__chmod.

 I better should look carefully on macros from next time while doing reviews.

Mamoru,
  Thanks for commenting missing things here.

Thanks for catching them up. should be ok now with the same URL above.

(In reply to comment #7)

> > * defattr
> >   - Now we recommend %defattr(-,root,root,-)
>    I am still unclear on usage of this as I guess some packagers still prefer
> %defattr(-,root,root) bu tI have seen you always asking packagers to use
> %defattr(-,root,root,-) but can't find this thing written in 
> packaging guidelines.
  - Actually also I cannot see the explicit mention in wiki,
    however
    * I was told by some other reviewers to do so
    * and all the examples under http://fedoraproject.org/wiki/Packaging/
     (for example, http://fedoraproject.org/wiki/Packaging/Guidelines )
     uses %defattr(-,root,root,-)

(In reply to comment #8)
> Thanks for catching them up. should be ok now with the same URL above.
- Sorry, however more issues:

* Perl module BuildRequires
  - For Perl module BuildRequires, please do not use the name of
    rpms but write the module name of it like
    http://fedoraproject.org/wiki/Packaging/Perl
    i.e. "BuildRequires: perl(ExtUtils::MakeMaker)" is better.

* Directory ownership issue
   - %{perl_vendorarch}/auto itself is owned by perl and
     this package should not own the directory.

* And please change the SourceURL.

(In reply to comment #9)
> (In reply to comment #7)
> 
> > > * defattr
> > >   - Now we recommend %defattr(-,root,root,-)
> >    I am still unclear on usage of this as I guess some packagers still prefer
> > %defattr(-,root,root) bu tI have seen you always asking packagers to use
> > %defattr(-,root,root,-) but can't find this thing written in 
> > packaging guidelines.
>   - Actually also I cannot see the explicit mention in wiki,
>     however
>     * I was told by some other reviewers to do so
>     * and all the examples under http://fedoraproject.org/wiki/Packaging/
>      (for example, http://fedoraproject.org/wiki/Packaging/Guidelines )
>      uses %defattr(-,root,root,-)
> 
    Yes. I also checked same when I saw your comments asking to use that
%defattr(-,root,root,-). But I guess its better to have it mentioned in Guidelines.
> (In reply to comment #8)
> > Thanks for catching them up. should be ok now with the same URL above.
> - Sorry, however more issues:
> 
> * Perl module BuildRequires
>   - For Perl module BuildRequires, please do not use the name of
>     rpms but write the module name of it like
>     http://fedoraproject.org/wiki/Packaging/Perl
>     i.e. "BuildRequires: perl(ExtUtils::MakeMaker)" is better.
> 
> * Directory ownership issue
>    - %{perl_vendorarch}/auto itself is owned by perl and
>      this package should not own the directory.

(In reply to comment #9)
> * Perl module BuildRequires
>   - For Perl module BuildRequires, please do not use the name of
>     rpms but write the module name of it like
>     http://fedoraproject.org/wiki/Packaging/Perl
>     i.e. "BuildRequires: perl(ExtUtils::MakeMaker)" is better.
> 
> * Directory ownership issue
>    - %{perl_vendorarch}/auto itself is owned by perl and
>      this package should not own the directory.

Doh. thanks again. hopefully this should be ok then.

(In reply to comment #10)
> * And please change the SourceURL.

have to bump a release?

(In reply to comment #12)
> Doh. thanks again. hopefully this should be ok then.
  Seems okay for me.

> (In reply to comment #10)
> > * And please change the SourceURL.
> 
> have to bump a release?
  This nkf is not built yet so bumping release number is not
  needed.

Ok, the updated nkf has been built on koji.

Package Change Request
======================
Package Name: nkf
New Branches: epel7
Owners: tagoh
InitialCC: i18n-team

Git done (by process-git-requests).