Bug 616120 (spacecmd) - Review Request: spacecmd - Command-line interface to Spacewalk and Satellite servers
Summary: Review Request: spacecmd - Command-line interface to Spacewalk and Satellite ...
Alias: spacecmd
Product: Fedora
Classification: Fedora
Component: Package Review
Version: 13
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Maxim Burgerhout
QA Contact: Fedora Extras Quality Assurance
Depends On:
Blocks: F-Spacewalk
TreeView+ depends on / blocked
Reported: 2010-07-19 17:36 UTC by Miroslav Suchý
Modified: 2014-11-01 08:29 UTC (History)
4 users (show)

Clone Of:
Last Closed: 2010-08-02 18:19:39 UTC
maxim: fedora-review+
opensource: fedora-cvs+

Attachments (Terms of Use)

Description Miroslav Suchý 2010-07-19 17:36:06 UTC

spacecmd is a command-line interface to Spacewalk and Satellite servers

Scratch build in koji:

$ rpmlint /tmp/spacewalk-build/spacecmd-0.5.1-1.el6.src.rpm /tmp/spacewalk-build/noarch/spacecmd-0.5.1-1.el6.noarch.rpm
spacecmd.noarch: W: non-conffile-in-etc /etc/bash_completion.d/spacecmd
2 packages and 0 specfiles checked; 0 errors, 1 warnings.

This warning should be OK, as I checked others bash_completation filse and they do not mark files in /etc/bash_completion.d as %config too.

Comment 1 Maxim Burgerhout 2010-07-22 19:51:20 UTC
== MUST ==
[x] Run rpmlint on all parts of the package and post output

    spacecmd.noarch: W: non-conffile-in-etc /etc/bash_completion.d/spacecmd
    1 packages and 0 specfiles checked; 0 errors, 1 warnings.

    I agree files in /etc/bash_completion.d should not have %config

[x] Package is named according to Naming Guidelines
[x] Specfile matches %{name}.spec
[x] Package matches Packaging Guidelines
[x] Package license matches Licensing Guidelines
[x] Actual license is in License tag in spec file
[x] License text in %doc if applicable
[x] Spec file is in English
[x] Spec file is legible
[x] MD5 sum of sources match upstream MD5 sum (through Source0)
[x] Must compile on at least *one* major architecture
[-] If package does not compile, it has ExcludeArch for that architecture,
with a bug filed for the architecture the package does not build on
[-] Build-time dependencies are in BuildRequires
[-] Uses %find_lang if appropriate; *not* %{_datadir}/locale/*
[-] Calls ldconfig in %post and %postun if carries libraries
[x] Does not carry copies of system libraries
[x] Doesn't do explicit relocatability
[!] Owns all directories it creates; if it uses a directory it does not create itself, it should depend on the package that *does* create it

    spacecmd should own /etc/bash_completion.d:

    %{rhnroot} (/usr/share/rhn) is not owned by any packages: https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes

    spacecmd should either depend on the package that creates %{rhnroot} or own it.        

[x] Has sane permissions, defattr() in every %files section
[!] Consistently uses macros
    According to
    http://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define it is preferred to use %global instead of %define 

[x] Contains only code or permissable content
[-] Large documentation goes in -doc package
[-] Header files go in -devel package
[-] static libraries go in -static package
[-] Libaries without a numeric suffix (.so.1.1) mostly go in -devel package
[-] Devel package requires base package like this
[-] All .la libtool archives are removed from the package
[-] GUI apps include a %{name}.desktop file
[x] Does not conflict with other packages in file ownership
[-] Filenames are in UTF-8

=== SHOULD ==
[-] If license text is not in separate file, query upstream to do so
[-] If available, the summary and description in the spec can be in other languages than English too
[x] Test build in mock
[x] Builds on all architectures
[x] Runs
[x] Scriptlets in spec file must be sane
[-] Subpackages require the base package
[-] .pc files go in -devel, mostly, unless the package is a devel tool, like gcc
[-] requires packages, not files in packages
[-] Try to have upstream create and add manpages for all binaries

There's a couple of issues with the package at the moment. They're marked with a [!] above.

There is one bigger issue with the RPM as it is now though and that is the fact that it does not contain *.pyc and *.pyo files when built in mock or Koji. The RPM *should* contain those files in order to be able to properly cleaning up when upgrading or uninstalling. See http://fedoraproject.org/wiki/Packaging:Python#Files_to_include

Comment 2 Aron Parsons 2010-07-22 20:50:10 UTC
The directory ownerships and %global fixes have been committed.

It seems that rpmbuild is not doing what it should in regards to byte compiling the Python files.  I tried the following to see if the spec was wrong:
- moving the *.py into %{python_sitelib}
- listing %{rhnroot}/spacecmd/*.py in the %files section

The only way I could get rpmbuild to byte-compile the Python files was to explicitly call 'brp-python-bytecompile' in %install, which probably isn't something that should be done.  Any ideas on what's going on?

Comment 3 Maxim Burgerhout 2010-07-22 21:33:41 UTC
You must add

BuildRequires: python-devel

to your specfile.

Comment 4 Aron Parsons 2010-07-22 22:10:20 UTC
Ah, I missed that part on the wiki. :-(

Fixes have all been committed and are tagged as spacecmd-0.5.2-1.

Comment 5 Maxim Burgerhout 2010-07-24 21:05:40 UTC
Aron, I like the fact that you have moved the whole lot into site_packages.

I see you have changed the specfile to require just 'python'. This is not necessary: the Python requirement is inserted by rpmbuild during creation of the RPM in the form of python(abi). This works for both EL5 and F13, so the 'python' requirement can be taken out.

One more request is to add the -p flag to your install commands. I forgot about this initially, sorry. This is to keep the original timestamps on the files you install; see http://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps for futher info. Take care to also keep the orignal timestamp on the source tarball when building srpms.

Please upload the corrected spec and srpm files somewhere and provide a link to them.

Am I right in assuming Miroslav is going to actually maintain this package when it is approved?

Comment 6 Miroslav Suchý 2010-07-26 07:51:34 UTC
I addressed issues from #5.

New files:

> Am I right in assuming Miroslav is going to actually maintain this package when
it is approved?

Yes. We are both from upstream team. But he is more upstream and I'm more maintainer for Fedora.

Comment 7 Maxim Burgerhout 2010-07-26 19:43:21 UTC
Looks good. Looking forward to using this.

spacecmd was approved by Maxim  Burgerhout d.d. 20100726

Comment 8 Miroslav Suchý 2010-07-27 08:51:11 UTC
New Package CVS Request
Package Name: spacecmd
Short Description: Command-line interface to Spacewalk and Satellite servers
Owners: msuchy
Branches: F-13, F-12, EL-5, EL-6, EL-4

Comment 9 Kevin Fenzi 2010-07-30 20:31:02 UTC
GIT done (by process-cvs-requests.py).

With f14 branch added.

Comment 10 Fedora Update System 2010-08-02 15:40:54 UTC
spacecmd-0.5.4-1.fc13 has been submitted as an update for Fedora 13.

Comment 11 Fedora Update System 2010-08-03 01:12:08 UTC
spacecmd-0.5.4-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 12 Miroslav Suchý 2014-10-31 10:26:39 UTC
Package Change Request
Package Name: spacecmd
New Branches: epel7
Owners: msuchy

Comment 13 Till Maas 2014-11-01 08:29:53 UTC
Git done (by process-git-requests).

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