Bug 468633 - (wput) Review Request: wput - A utility for uploading files or whole directories to remote ftp-servers
Review Request: wput - A utility for uploading files or whole directories to ...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Fabian Affolter
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2008-10-26 20:06 EDT by Itamar Reis Peixoto
Modified: 2009-10-03 17:32 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-11-19 09:46:39 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mail: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Itamar Reis Peixoto 2008-10-26 20:06:58 EDT
Spec URL: http://ispbrasil.com.br/wput/wput.spec
SRPM URL: http://ispbrasil.com.br/wput/wput-0.6.1-1.0.fc9.src.rpm


wput is a command-line ftp-client that looks like wget but instead of
downloading, uploads files or whole directories to remote ftp-servers.
Comment 1 Fabian Affolter 2008-10-27 10:07:32 EDT
Just some small comments on your spec file

- Release: 1.0%{?dist}
 - Just '1' is enough. Next release will be '2'

- You can use the macro %{name} everywhere where you used 'wput' 

- %files section
 - '%defattr(-,root,root)', the usual one is '%defattr(-,root,root,-)'
 - Is it not enough just to use '%{_bindir}/%{name}' insteed of '%attr(0755, root, root) %{_bindir}/wput' ?
Comment 3 Fabian Affolter 2008-10-27 17:17:02 EDT
If this is your first package you need to seek a sponsor. I can't sponsor you but I can do the review.

Please visit the following pages:

https://fedoraproject.org/wiki/PackageMaintainers/Join#Create_Your_Review_Request (-> FE-NEEDSPONSOR ;-) )
Comment 4 Fabian Affolter 2008-10-27 17:30:11 EDT
Again some comments on your spec file

- Source:
 - Please use 'Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz' instead of a link to a mirror

- Patch1: wput-destdir.patch
 - The first patch is 'Patch0'

- %changelog
 - To every release bump belongs an changelog entry.  
Comment 5 Fabian Affolter 2008-10-27 17:32:42 EDT
(In reply to comment #3)
> (-> FE-NEEDSPONSOR ;-) )

Forget this...you added the blocker already.
Comment 6 Itamar Reis Peixoto 2008-10-27 17:49:09 EDT

Can I start bumping versions later ?

Comment 7 manuel wolfshant 2008-10-27 17:57:58 EDT
> The first patch is 'Patch0'
Just for the record: that's more a cosmetic / pedantic problem. rpm is very
happy to use anything, as long as the definition of the patch (the PatchN line)
and it's usage (the %PatchN line) correspond.
But yes, for a first submission looking nice (from a cosmetic point of view) is
important and could be a decision factor.

More important: please preserve the older changelog entries when you/add make
changes and create new releases of the spec. I am looking at releases 2 and 3 right now and I would have liked to know what was changed compared to the first ones. Especially as the _current_ changelog (the one from release 3) still says "Initial package."

A correct changelog would have probably had something along:

* Thu Oct 27 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-3
- start counting patches from 0

* Thu Oct 27 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-2
- fixes %%files, consistent use of macros<, other relevant changes, if any>

* Thu Oct 26 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-1.0
- Initial package.
Comment 8 Fabian Affolter 2008-10-27 18:13:36 EDT
(In reply to comment #6)
> Can I start bumping versions later ?

I'm not talking about the version of the source.  The 'Release' is for the spec file. 

Again changelog...in your lasted spec file is only *ONE* changelog entry.  If your Release is '3' there have to be '3' entries. Your spec file should contain.  For the reviewers it's much easier to keep track of the changes. 

* Mon Oct 27 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-3
- Fix Source0
- Rename Patch1 to Patch0

* Mon Oct 27 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-2
- Changes acc. #468633 Comment #1

* Thu Oct 26 2008 Itamar Reis Peixoto <itamar@ispbrasil.com.br> - 0.6.1-1
- Initial package.
Comment 9 Fabian Affolter 2008-10-27 18:43:06 EDT
Koji scratch build:

Comment 10 manuel wolfshant 2008-10-27 19:09:45 EDT
My nose smells missing BR: gnutls-devel. And I have a feeling that gettext is needed, too , despite the fact that the locale files get created (the configure script complains about missing gettext)
Comment 11 Fabian Affolter 2008-10-27 20:05:09 EDT
gettext is definitely missing and adding gnutls will be a good idea ;-)

BuildRequires: gettext
BuildRequires: gnutls-devel

Itamar, take a look into the log at http://koji.fedoraproject.org/koji/getfile?taskID=908022&name=build.log
Comment 12 Itamar Reis Peixoto 2008-10-27 23:19:27 EDT
(In reply to comment #11)
> gettext is definitely missing and adding gnutls will be a good idea ;-)
> BuildRequires: gettext
> BuildRequires: gnutls-devel


Comment 13 Fabian Affolter 2008-10-28 04:47:34 EDT
Thanks for the changes. I will make a full review today.
Comment 14 Mamoru TASAKA 2008-10-28 14:01:27 EDT
Comment 15 Fabian Affolter 2008-10-28 18:55:21 EDT
Package Review

 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary RPMs on at least one supported architecture.
     Tested on: F9/i386
 [x] Rpmlint output:
     Source RPM:
     [fab@laptop024 SRPMS]$ rpmlint wput*
     1 packages and 1 specfiles checked; 0 errors, 0 warnings.
     Binary RPMs:
     [fab@laptop024 i386]$ rpmlint -i wput*
     2 packages and 0 specfiles checked; 0 errors, 0 warnings.
 [x] Package is not relocatable.
 [x] Buildroot is correct
     master   : %{_tmppath}/%{name}-%{version}-%{release}-root
     spec file: %{_tmppath}/%{name}-%{version}-%{release}-root
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: GPLv2+
 [x] If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     SHA1SUM upstream: 67125acab1d520e5d2a0429cd9cf7fc379987f30d5bbed0b0e97b92b554fcc13  wput-0.6.1.tgz
     SHA1SUM of Source RPM: 67125acab1d520e5d2a0429cd9cf7fc379987f30d5bbed0b0e97b92b554fcc13  wput-0.6.1.tgz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [x] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [-] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: F9/i386
 [x] Package should compile and build into binary RPMs on all supported architectures.
     Tested F9:  http://koji.fedoraproject.org/koji/taskinfo?taskID=910045
     Tested F10: http://koji.fedoraproject.org/koji/taskinfo?taskID=910050
 [?] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.

I see no further blocker, package APPROVED
Comment 16 Itamar Reis Peixoto 2008-10-29 17:01:48 EDT
New Package CVS Request
Package Name: wput
Short Description:  A utility for uploading files or whole directories to remote ftp-servers
Owners: itamarjp
Branches: F-9  F-10
InitialCC: fabian
Comment 17 Kevin Fenzi 2008-10-29 17:25:10 EDT
cvs done.
Comment 18 Fedora Update System 2008-10-30 13:40:47 EDT
wput-0.6.1-4.fc9 has been submitted as an update for Fedora 9.
Comment 19 Fedora Update System 2008-10-31 06:26:49 EDT
wput-0.6.1-4.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update wput'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-9330
Comment 20 Fedora Update System 2008-11-19 09:46:36 EST
wput-0.6.1-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 21 Itamar Reis Peixoto 2009-10-01 18:40:26 EDT
Package Change Request
Package Name: wput
New Branches: EL-4 EL-5
Owners: itamarjp
Comment 22 Kevin Fenzi 2009-10-03 17:32:09 EDT
cvs done.

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