Bug 196434 - Review Request: ren
Review Request: ren
Status: CLOSED DUPLICATE of bug 211711
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-23 05:45 EDT by Alan Iwi
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-21 03:42:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Alan Iwi 2006-06-23 05:45:39 EDT
Spec URL: http://home.badc.rl.ac.uk/iwi/ren.spec
SRPM URL: http://home.badc.rl.ac.uk/iwi/ren-1.0-5.src.rpm
Description: Rename multiple files

This small but very useful utility (not written by me) is in RedHat contrib at version 1.0-4, but doesn't seem to be in Fedora Extras.  I have taken it and made some tweaks to clean up the build and packaging and called it 1.0-5, but the functionality is still as it ever was.
Comment 1 Parag AN(पराग) 2006-06-23 07:38:07 EDT
Not an official review as I'm not yet sponsored
Mock build fro i386 development is sucessfull

MUST Items:
     - MUST: rpmlint shows error 
       W: ren summary-not-capitalized rename multiple files
       Summary doesn't begin with a capital letter.
            Change Summary to Summary: Rename multiple files

      W: ren non-standard-group Utilities/File
      The group specified in your spec file is not valid. To find a valid group,
please refer to the  RPM documentation.
        No such groups exists. 
       
      W: ren invalid-license public domain
      The license you specified is invalid. The valid licenses are:

      -GPL                                    -LGPL
      -Artistic                               -BSD
      -MIT                                    -QPL
       -MPL                                    -IBM Public License
       -Apache License                         -PHP License
      -Public Domain                          -Modified CNRI Open Source License
      -zlib License                           -CVW License
      -Ricoh Source Code Public License       -Python license
      -Vovida Software License                -Sun Internet Standards Source License
       -Intel Open Source License              -Jabber Open Source License

      if the license is close to an existing one, you can use '<license> style'.
 
     Add License tag and any of above suitable open source license.

      W: ren no-url-tag
       Add URL: package hosting URL

      W: ren hardcoded-prefix-tag /usr
      The Prefix tag is hardcoded in your spec file. It should be removed, so as
to allow package relocation.
        Use macros instead hardcoding directories. check
http://www.fedoraproject.org/wiki/Extras/RPMMacros

       E: ren no-%clean-section
       The spec file doesn't contain a %clean section to remove the files
installed by the %install section.
        Add to SPEC file
       %clean
        rm -rf $RPM_BUILD_ROOT

       W: ren patch-not-applied Patch0: ren-1.0.Wall.patch
       A patch is included in your package but was not applied. Refer to the
patches documentation to see what's wrong.

       Move changelog at end.
         You need to read http://fedoraproject.org/wiki/Packaging/Guidelines
Comment 2 Alan Iwi 2006-06-23 08:07:09 EDT
Please can you download the file again, as I have now corrected as many of the
above errors as possible.

However, there are two errors that I can't correct because I can't see what's wrong.

1) "no-%clean-section"

   There is already a %clean section. It says:

   %clean
    rm -rf %{buildroot}

2) "patch-not-applied"

   The patch is applied when I do rpmbuild.

   The purpose of the patch is to fix a load of things that cause warnings
   when compiled with "gcc -Wall".  When I build it, it tells me:

   + echo 'Patch #0 (ren-1.0.Wall.patch):'
   Patch #0 (ren-1.0.Wall.patch):
   + patch -p1 -s

   and later:

   + make CC=gcc CFLAGS=-Wall
   gcc -o ren -Wall ren.c
   + exit 0

   It could not possibly be completing "gcc -Wall" without errors if the
   patch was not being applied.

Regards,
Alan
Comment 3 Parag AN(पराग) 2006-06-23 08:13:23 EDT
will check it again now
Comment 4 Parag AN(पराग) 2006-06-23 08:16:12 EDT
hey you have not changed version number and not given updates SPEC as well as
SRPM links
Remember to update release versions whenever you change SPEC file and give
upadtes links  
Comment 5 Parag AN(पराग) 2006-06-23 08:23:31 EDT
ok rpmlint on package return no error
Mock built for i386 development is also successfull

but still i found that 
   * no dist tag 
       Add to release as Release: 5%{?dist}
   * Change Buildroot to 
     %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
   * Add version info to each Chnagelog entry.  Check other peoples SPEC files here.
   * You need to provide your License file in tarball.
Comment 6 Alan Iwi 2006-06-23 08:41:20 EDT
Sorry, I am going to have to cancel this submission as the License file is not
in the tarball, and in accordance with the "pristine sources" principle I will
not change the tarball in any way from what is on ftp://sunsite.unc.edu/

(The package originates from a posting to comp.sources.unix, which I assume
means it is public domain, but there is no License file.)
Comment 7 Parag AN(पराग) 2006-06-23 08:46:17 EDT
hey dont do that now. wait for official review of this package from any of
official reviewers.
Comment 8 Paul Howarth 2006-06-23 08:48:32 EDT
(In reply to comment #6)
> Sorry, I am going to have to cancel this submission as the License file is not
> in the tarball, and in accordance with the "pristine sources" principle I will
> not change the tarball in any way from what is on ftp://sunsite.unc.edu/
> 
> (The package originates from a posting to comp.sources.unix, which I assume
> means it is public domain, but there is no License file.)

Parag is confusing the roles of "upstream" and "packager". Sometimes the two are
the same, in which case upstream should ensure that licensing information is
included properly in the distribution. In this case, "upstream" and "packager"
are different, and the packager should *not* be fiddling with the tarball,
Indeed, the package review guidelines do not require the inclusion of license
text if upstream does not provide it.

There are very few circumstances in which a packager should alter an upstream
tarball (e.g. it contains patented content that is not used in the package), and
adding a license file is certainly not one of them.

Please continue with your submission.
Comment 9 Alan Iwi 2006-06-23 09:08:15 EDT
Okay, fine, I won't withdraw it now.  However unfortunately I do not understand
what is meant regarding dist tags, and have run out of time, so if the review
indicates that further changes are required then I will let it drop at that
stage.  But if someone who knows more about Fedora packaging requirements than I
do agrees with my view that this is a very useful little utility to include, and
is willing to take on the packaging and iron out any further little problems,
then that would be much appreciated, thanks.

Bye for now,
Alan
Comment 10 Paul Howarth 2006-06-23 09:15:26 EDT
(In reply to comment #9)
> Okay, fine, I won't withdraw it now.  However unfortunately I do not understand
> what is meant regarding dist tags, and have run out of time, so if the review
> indicates that further changes are required then I will let it drop at that
> stage.  But if someone who knows more about Fedora packaging requirements than I
> do agrees with my view that this is a very useful little utility to include, and
> is willing to take on the packaging and iron out any further little problems,
> then that would be much appreciated, thanks.

The dist tag is described on the wiki:
http://fedoraproject.org/wiki/DistTag

Use of it is not mandatory, but most people do use it as it's a conveient way of
distinguishing packages built for different distribution releases.

If you've run out of time then it's probably better to close the review. Adding
packages to Fedora Extras is not a "fire and forget" process; packages need to
be _maintained_, so that bug reports get answered, security issues get fixed
etc. If you don't have the time to finish the submission process then it's
unlikely you'll have the time to maintain the package in the longer term, so
indeed it would be better off for someone else to maintain it.

Comment 11 Parag AN(पराग) 2006-06-23 09:35:55 EDT
Paul,
  Thanks for clearing licensing issue. 
Comment 12 manuel wolfshant 2006-06-23 12:03:25 EDT
I have adjusted the spec per comment #5. Spec file and src.rpm are available at
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-6.src.rpm and
http://wdl.lug.ro/linux/rpms/ren/ren.spec

Alan, per comment #9, if you have nothing against, I'll try to co-maintain
together with you this package.

Manuel
Comment 13 Michael Schwendt 2006-06-26 08:07:45 EDT
What's the reason for using

  CFLAGS=-Wall

instead of

  CFLAGS="-Wall %{optflags}"

?
Comment 14 manuel wolfshant 2006-06-27 19:36:02 EDT
I've uploaded to http://wdl.lug.ro/linux/rpms/ren/ren.spec and
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-6.src.rpm new versions of the spec and
src.rpm files.
Changes: cleanup of the %build and %files (permisssions)sections to closer
resemble the skeleton generated by fedora-newrpmspec.

Obs: the original package does not really include an install section in the
Makefile. So we could either keep the 3 lines  from the current form (they
simply copy the files to their places) or add yet another patch which would
modify the Makefile so that make install would copy the files itself.
Comment 15 manuel wolfshant 2006-06-27 19:38:12 EDT
I am sorry, I meant http://wdl.lug.ro/linux/rpms/ren/ren-1.0-7.src.rpm not 1.0-6
Comment 16 Michael J Knox 2006-07-25 22:56:04 EDT
I can review this one. 

----------------------------------------

Review for release 7:
* RPM name is OK
* This is the latest version
* Builds fine in mock
* rpmlint looks OK
* File list looks OK

Needs work:

debuginfo package is not built correctly. I added CFLAGS="$RPM_OPT_FLAGS" to the
make line:

make CFLAGS="$RPM_OPT_FLAGS" %{?_smp_mflags}

This builds a correct debuginfo package. 

Fix that up and I can approve this submission. 
Comment 17 manuel wolfshant 2006-07-26 18:40:22 EDT
Thank you for the review, Michael.

Spec file modified accordingly. The new versions are available at
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-8.src.rpm and
http://wdl.lug.ro/linux/rpms/ren/ren.spec
Comment 18 Michael J Knox 2006-07-26 22:27:22 EDT
Looks good. APPROVED. Please remember to close this review submission once
imported into CVS and built. 
Comment 19 manuel wolfshant 2006-07-27 04:15:25 EDT
I am sorry, but as this is my first approved package, I am not sure about the
next step... Does your approval mean that I am sponsored?
Comment 20 Michael J Knox 2006-07-27 04:19:37 EDT
Nope.. someone needs to sponsor you. 

You needed to have FE-NEEDSPONSOR blocking your review request. I can't sponsor
anyone just yet. 
Comment 21 Michael J Knox 2006-08-01 17:50:43 EDT
OK, I can sponsor you.( just been made a sponsor)

However, this is a fairly small and simple package. Do you plan to submit more
packages to FE ?
Comment 22 manuel wolfshant 2006-08-01 19:47:05 EDT
Actually my first submission was ssmtp. Please be as kind as to check 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=188400 
 
I have adopted ren because the original submitter preferred to abandon rather 
then make a few [simple] modifications and it was a pity to loose his work. 
And since we use it anyway at my workplace... 
Comment 23 Michael J Knox 2006-09-16 16:29:42 EDT
Sorry. Due to my stepping out for a while, I am unable to complete this review. 
Comment 24 Stewart Adam 2006-09-21 19:45:18 EDT
I'm not a reviewer, as I'm waiting to be sponsored, but I can help out:

rpmlint still shows warning on the RPM, I'm not sure if you're absolutely
required to fix these as they're minor, but they're there nonetheless:

W: ren setup-not-quiet
You should use -q to have a quiet extraction of the source tarball, as this
generate useless lines of log ( for buildbot, for example )

W: ren macro-in-%changelog defattr
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

W: ren macro-in-%changelog _mandir
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.
Comment 25 manuel wolfshant 2006-09-21 21:43:15 EDT
Thank you for your review.
New versions are available at http://wdl.lug.ro/linux/rpms/ren/ren.spec and
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-9.src.rpm
Comment 26 Stewart Adam 2006-09-21 22:15:12 EDT
Excellent - rpmlint's clean on both SRPM and binary RPM.
Comment 27 Kevin Fenzi 2006-10-20 12:25:33 EDT
It looks to me like Michael was going to sponsor you, but due to his time 
constraints he was unable to do so. 

However, this bug is in a weird state where potential sponsors can't see it. 
I am changing it back to block FE-NEW so another sponsor can see it. 
Comment 28 Patrice Dumas 2006-10-20 14:14:49 EDT
Ok, I'll have a look at it soon.
Comment 29 Patrice Dumas 2006-10-20 15:27:46 EDT
The source don't match. From spectool -g:
245453453a8bd1c0bf7d03880b97d632  ren-1.0.tar.gz

versus yours
2fa23fa3ebf7e6d68147f5f91725dd29  ../SOURCES/ren-1.0.tar.gz

Otherwise the man page is better globbed like, in my opinion
%{_mandir}/man1/ren.1*

I would have choosed
ftp://sunsite.unc.edu/pub/Linux/utils/file/ren-1.0.tar.gz.lsm
as url. Not a big deal.
Comment 30 manuel wolfshant 2006-10-20 17:28:22 EDT
Thanks for the review, Patrice. As usual, your eye view is sharper then mine..
at least for the time being :)

Indeed, the tar file included by Alan seemed to be a repacked version of the
files included in the upstream version. diff -r did not show any differences
between the actual content of the two but I have included the original, as it
should have been in the first place.
AFAIK in all RH/FC distributions, man pages are gzip-ed, hence the .ren.1.gz
from the spec. I have decided to follow your advice and play safe, just in case.
Please find the updated spec and src.rpm at
http://wdl.lug.ro/linux/rpms/ren/ren.spec and
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-10.src.rpm respectively.
Comment 31 Patrice Dumas 2006-10-20 17:58:11 EDT
* rmlint is silent
* licence is not explicitely set, however since it comes from
  a post on usenet with explicit permission to reuse and redistribute
  this may be considered public domain.
* follow guidelines
* match upstream
245453453a8bd1c0bf7d03880b97d632  ren-1.0.tar.gz
* %files right

The timestamp of the file is wrong. To get the right timestamp, 
you can use 
wget -N ftp://sunsite.unc.edu/pub/Linux/utils/file/ren-1.0.tar.gz
or 
spectool -g ren.spec

This is not a blocker so it is APPROVED.

It would be better, however to open a new bug such that you appear 
as the reporter. So, please next open a new bug for this review, 
with the latest srpm posted. I'll close that one as a duplicate and 
set the blockers right.

Also go on with the sponsorship process, I'll sponsor you.
Comment 32 manuel wolfshant 2006-10-20 20:00:45 EDT
Thank you a lot, Patrice.

Updated spec and src.rpm which include the correct timestamp are available at
http://wdl.lug.ro/linux/rpms/ren/ren.spec and
http://wdl.lug.ro/linux/rpms/ren/ren-1.0-11.src.rpm respectively.

This is the original review, resubmitted as
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211711
Comment 33 Patrice Dumas 2006-10-21 03:42:59 EDT
Closing this as a duplicate of #Bug 211711.


Alan, if you are still interested in submitting other packages
to fedora extras and become a fedora contributor, remember to
point your potential sponsor at this bug to show you allready
did something usefull.

*** This bug has been marked as a duplicate of 211711 ***

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