Bug 570424 - Review Request: transmission-remote-cli - A console client for the Transmission BitTorrent client
Summary: Review Request: transmission-remote-cli - A console client for the Transmissi...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dominic Hopf
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-03-04 10:05 UTC by Satya Komaragiri
Modified: 2010-06-23 16:47 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-27 15:45:49 UTC
dmaphy: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Satya Komaragiri 2010-03-04 10:05:18 UTC
Spec URL: http://sundaram.fedorapeople.org/packages/transmission-remote-cli.spec
SRPM URL: http://sundaram.fedorapeople.org/packages/transmission-remote-cli-0.5.5-1.20100303git.fc12.src.rpm
Description: A simple command line interface using ncurses for the Transmission BitTorrent client. It is an alternative to transmissioncli and can be used to control local and remote instances of the Transmission daemon.

Comment 1 Dominic Hopf 2010-03-05 22:50:21 UTC
A few notes:
 * Line 30 is not necessary. You're installing /usr/bin/ there, I guess that directory should already exist on any system. ;) To solve the issue you may wanted to solve with that line I suggest to write the line following to that like this:

   install -Dpm 755  %{SOURCE0} %{buildroot}/%{_bindir}/transmission-remote-cli
(the -D is the important switch here, see 'man install')

 * The full URL to the README.md file is missing, when trying to build the package, that file can not be found
 * You should preserve timestamps within %install section (-p for install)
 * You don't need the copy commands in %prep section, since you do any copying with the install commands in %install section
 * Suggestion how to install the README.md in %install section:

   install -pm 755 %{SOURCE1} %{buildroot}README.md

 * This CLI obviously is not developed by the transmission team itself, so the URL http://www.transmissionbt.com/ may not be the correct one. Since I couldn't find a serious looking web page of fagga or this CLI client, I'd suggest to use the "project page" (http://github.com/fagga/transmission-remote-cli) instead.

Comment 2 Satya Komaragiri 2010-03-07 09:20:09 UTC
Hi Dominic,

Thanks for the suggestions.

The updated spec file may be found at: http://sundaram.fedorapeople.org/packages/transmission-remote-cli.spec

And the updated SRPM is at: http://sundaram.fedorapeople.org/packages/transmission-remote-cli-0.5.5-2.20100303git.fc12.src.rpm

Comment 3 Dominic Hopf 2010-03-07 23:00:31 UTC
$ rpmlint transmission-remote-cli.spec
transmission-remote-cli.spec: W: no-buildroot-tag
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

The no-buildroot-tag warning can be ignored since you actually use it to install
the files.

$ rpmlint transmission-remote-cli.spec
transmission-remote-cli.spec: W: no-cleaning-of-buildroot %install
transmission-remote-cli.spec: W: no-buildroot-tag

The first warning can and should be solved by adding 'rm -rf %{buildroot}' at
the beginning of the %install section. For second warning, see above.

$ rpmlint transmission-remote-cli-0.5.5-2.20100303git.fc12.noarch.rpm
transmission-remote-cli.noarch: W: spelling-error %description -l en_US ncurses -> nurses, curses, n curses
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Guess ncurses actually is spelled correctly. ;)


Package Review
==============

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

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines
 [x] Specfile name matches %{name}.spec
 [x] Package seems to meet Packaging Guidelines
 [x] Package successfully compiles and builds into binary RPMs on at least one
     supported architecture.
     Tested on: Fedora 12/x86_64
 [x] Rpmlint output:
     source RPM: see above
     binary RPM: see above
 [x] Package is not relocatable.
 [x] License in specfile matches actual License and meets Licensing Guidelines
     License: GPLv3+
 [-] License file is included in %doc.
 [x] Specfile is legible and written in AE
 [x] Sourcefile in the Package is the same as provided in the mentioned Source
     SHA1SUM of Source: 52873f09c773101e3ef982d5406205ae878b3c33
 [x] Package compiles successfully
 [x] All build dependencies are listed in BuildRequires
 [-] Specfile handles locales properly
 [-] ldconfig called in %post and %postun if required
 [-] Package owns directorys it creates
 [-] Package requires other packages for directories it uses.
 [x] Package does not list a file more than once in the %files listing
 [x] %files section includes %defattr and permissions are set properly
 [x] %clean section is there and contains rm -rf %{buildroot}
 [x] Macros are consistently used
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage
 [x] Program runs properly without files listed in %doc
 [-] Header files are in a -devel package
 [-] Static libraries are in a -static package
 [-] Package requires pkgconfig if .pc files are present
 [-] .so-files are put into a -devel subpackage
 [-] Subpackages include fully versioned dependency for the base package
 [-] Any libtool archives (*.la) are removed
 [-] contains desktop file (%{name}.desktop) if it is a GUI application
 [x] Package does not own files or directories owned by other packages.
 [!] %{buildroot} is removed at beginning of %install
 [-] Filenames are encoded in UTF-8

=== SUGGESTED ITEMS ===
 [!] Package contains latest upstream version
 [x] Package does not include license text files separate from upstream.
 [-] non-English translations for description and summary
 [!] Package builds in mock
     Tested on: F12/x86_64; see my note concerning README.md below
 [x] Package should compile and build into binary RPMs on all supported architectures.
     tested build with koji
 [x] Program runs
 [-] Scriptlets must be sane, if used.
 [-] pkgconfig (*.pc) files are placed in a -devel package
 [-] require package providing a file instead of the file itself
     no files outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin are required

Issues to point out:
* You should write down in your ChangeLog what you actually changed in the
  package or rather in the specfile
* The installation of the README.md is not okay yet. The file actually would get
  installed in /, which obviuously is not the right place for documentation files.
  Seems I forgot the important thing in my suggestion before, sorry for that.
  (It really was a 'quick note' it seems).

  This one will be even better:

  install -Dpm 644 %{SOURCE1} %{buildroot}/%{_docdir}/%{name}-%{version}/README.md

  Note you will also have to change the line in %files section accordingly:

  %doc %{_docdir}/%{name}-%{version}/README.md

* While checking the sha1sum I noted there were some changes on the code just
  today, maybe you want to update the file and the release tag accordingly then?

* The release number should always begin with 0. For example a full version string
  should be something like 0.5.5-0.20100303git. See [1] for the guidelines about
  that. The principle you are following at preset is the one for post-releases,
  but there weren't any releases before. Basically, the guideline which concerns
  to pre-release packages should be applied here. But, I guess there never will
  be a release after this "pre-release", since the sources just can be obtained
  from the git repository. I won't be that strict and would approve your package
  also when you're staying at increasing the release number.

Please fix at least the first two issues and I will approve your package then.


[1] http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

Comment 4 Satya Komaragiri 2010-03-10 08:48:41 UTC
> [!] %{buildroot} is removed at beginning of %install
According to http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag, cleaning up of buildroot isn't required from Fedora 10 onwards.

> [!] Package contains latest upstream version
> * While checking the sha1sum I noted there were some changes on the code just
>   today, maybe you want to update the file and the release tag accordingly
>   then?
Corrected.

> [!] Package builds in mock
>      Tested on: F12/x86_64; see my note concerning README.md below
> * The installation of the README.md is not okay yet. The file actually would
> get installed in /, which obviuously is not the right place for documentation
> files.
 --snip--
Corrected

> * You should write down in your ChangeLog what you actually changed in the
>   package or rather in the specfile
Corrected

> * The release number should always begin with 0. 
--snip--
Corrected.


The updated spec file may be found at:
http://sundaram.fedorapeople.org/packages/transmission-remote-cli.spec

And the updated SRPM is at:
http://sundaram.fedorapeople.org/packages/transmission-remote-cli-0.5.5-0.3.20100310git.fc12.src.rpm

Comment 5 Rahul Sundaram 2010-04-03 01:08:52 UTC
Dominic Hopf,

Please complete this review.  I would like to see this as part of the Fedora 13 release and we need some time for testing.  We are nearing the beta release.

Comment 6 Dominic Hopf 2010-04-05 20:43:17 UTC
(In reply to comment #4)
> > [!] %{buildroot} is removed at beginning of %install
> According to http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag,
> cleaning up of buildroot isn't required from Fedora 10 onwards.
Satya, the mentioned guideline just allows you to omit the BuildRoot-tag at the beginning of the file. This is not the same as the %{buildroot} macro or the $RPM_BUILD_ROOT variable. rpmlint still claims, that the 'rm -rf %{buildroot}' command at the beginning of the %install section is missing. It wouldn't if it really was obsolete. Please add it before requesting CVS access. :)

> > [!] Package contains latest upstream version
> > * While checking the sha1sum I noted there were some changes on the code just
> >   today, maybe you want to update the file and the release tag accordingly
> >   then?
> Corrected.
> 
> > [!] Package builds in mock
> >      Tested on: F12/x86_64; see my note concerning README.md below
> > * The installation of the README.md is not okay yet. The file actually would
> > get installed in /, which obviuously is not the right place for documentation
> > files.
>  --snip--
> Corrected
> 
> > * You should write down in your ChangeLog what you actually changed in the
> >   package or rather in the specfile
> Corrected
> 
> > * The release number should always begin with 0. 
> --snip--
> Corrected.
> 
> 
> The updated spec file may be found at:
> http://sundaram.fedorapeople.org/packages/transmission-remote-cli.spec
> 
> And the updated SRPM is at:
> http://sundaram.fedorapeople.org/packages/transmission-remote-cli-0.5.5-0.3.20100310git.fc12.src.rpm

Thanks very much for correcting the mentioned isssues and for your patience. I'm sorry I was a bit stressed the last weeks and had lot of things to do at work, which caused my delayed answer to this issue. Except the mentioned issue with the %install section, your package looks good now and is APPROVED. Remember to add the 'rm -rf %{buildroot}' to the %install section before requesting the CVS access.

Comment 7 Rahul Sundaram 2010-04-05 21:06:38 UTC
Actually, rpmlint needs to be updated and a bug report filed against it already.  Cleaning up the buuldroot is completely unnecessary and is done automatically by newer versions of RPM a clearly indicated in the guidelines.  To quote,

"The provided buildroot will automatically be cleaned before commands in %install are called."

The rpmlint warning only applies to EPEL and not to Fedora anymore.

Comment 8 Christoph Wickert 2010-04-05 22:55:14 UTC
Although the BuildRoot tag and cleaning the buildroot are no longer stricktly necessary, what is wrong with having these two lines in the spec and making it work on more platforms?

If you really want to save a few letters, I suggest to replace

install -Dpm 644 %{SOURCE1} %{buildroot}%{_docdir}/%{name}-%{version}/README.md

and

%doc %{_docdir}/%{name}-%{version}/README.md

with a simple

%doc %{SOURCE1}

Comment 9 Rahul Sundaram 2010-04-05 23:11:55 UTC
 The RPM packages in Fedora are only meant for Fedora.   I don't see the point in cluttering the spec file but that is left to the packager.  I am only opposing any claims that is  *required*  anymore.   In fact, even a %clean is not necessary anymore in Fedora 13.  The rpmlint warnings can be ignored.

Comment 10 Christoph Wickert 2010-04-05 23:23:00 UTC
You are right, it's up to the packager, but here are some more things I'd like to point out: 

The versioning of the package is IMO wrong. The release 0.1git... indicates that it is a pre-release and a final version is still to come, but upstream does no releases and uses git only for hosting. Thus the package can simply be called 0.5.5-3

On the other hand the spec lacks a comment how to regenerate the exact source that was used. Upstream is already at 0.56 now, so there should be a comment how to get 0.5.5. See
https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

VCS keys would be nice too, although they are not yet ratified by FESCo. See 
http://fedoraproject.org/wiki/User:Walters/Packaging_VCS_key_proposal

Last but not least the requires for transmission-deamon should IMHO be versioned since the program only supports >= 1.80 but we have older versions in the 'everything' repos.

Comment 11 Satya Komaragiri 2010-04-07 11:05:10 UTC
> Satya, the mentioned guideline just allows you to omit the BuildRoot-tag at the
> beginning of the file. This is not the same as the %{buildroot} macro or the
> $RPM_BUILD_ROOT variable. rpmlint still claims, that the 'rm -rf %{buildroot}'
> command at the beginning of the %install section is missing. It wouldn't if it
> really was obsolete. Please add it before requesting CVS access. :)
> 
Corrected
> 
> Thanks very much for correcting the mentioned isssues and for your patience.
> I'm sorry I was a bit stressed the last weeks and had lot of things to do at
> work, which caused my delayed answer to this issue. Except the mentioned issue
> with the %install section, your package looks good now and is APPROVED.
> Remember to add the 'rm -rf %{buildroot}' to the %install section before
> requesting the CVS access.    

No problem :) Thanks a lot for reviewing!

Comment 12 Satya Komaragiri 2010-04-07 11:08:53 UTC
Thank you for the feedback :)

(In reply to comment #10)
> You are right, it's up to the packager, but here are some more things I'd like
> to point out: 
> 
> The versioning of the package is IMO wrong. The release 0.1git... indicates
> that it is a pre-release and a final version is still to come, but upstream
> does no releases and uses git only for hosting. Thus the package can simply be
> called 0.5.5-3
> 
Corrected.

> On the other hand the spec lacks a comment how to regenerate the exact source
> that was used. Upstream is already at 0.56 now, so there should be a comment
> how to get 0.5.5. See
> https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
> 
Corrected

> VCS keys would be nice too, although they are not yet ratified by FESCo. See 
> http://fedoraproject.org/wiki/User:Walters/Packaging_VCS_key_proposal
> 
Added

> Last but not least the requires for transmission-deamon should IMHO be
> versioned since the program only supports >= 1.80 but we have older versions in
> the 'everything' repos.   

Corrected.


The updated spec file can be found at http://sundaram.fedorapeople.org/packages/transmission-remote-cli.spec

The new SRPM is at http://sundaram.fedorapeople.org/packages/transmission-remote-cli-0.5.5-4.src.rpm

Comment 13 Satya Komaragiri 2010-04-07 11:26:00 UTC
New Package CVS Request
=======================

Package Name: transmission-remote-cli
Short Description: A console client for the Transmission BitTorrent client
Owners: satyak sundaram
Branches: F-13
InitialCC:

Comment 14 Kevin Fenzi 2010-04-08 02:39:12 UTC
CVS done (by process-cvs-requests.py).

Comment 15 Ville Skyttä 2010-06-22 15:28:07 UTC
Is it intentional that there are no files (except the ones created while doing the branch) for this in devel branch in CVS?

Comment 16 Satya Komaragiri 2010-06-23 10:50:35 UTC
Fixing an issue with my ssh keys. Will build for rawhide as soon as the problem is
resolved.

Comment 17 Satya Komaragiri 2010-06-23 16:47:01 UTC
Added version 6.5 (the latest upstream to devel branch.


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