Bug 815135 - Review Request: atf - Automated Testing Framework
Review Request: atf - Automated Testing Framework
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-22 15:22 EDT by Julio Merino
Modified: 2013-02-14 17:16 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-03 14:51:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
michel: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Julio Merino 2012-04-22 15:22:34 EDT
Spec URL: ftp://ftp.NetBSD.org/pub/NetBSD/misc/jmmv/fedora/atf.spec
SRPM URL: ftp://ftp.NetBSD.org/pub/NetBSD/misc/jmmv/fedora/atf-0.15-1.fc17.src.rpm
Description: ATF is a testing framework: it provides libraries for C, C++ and sh to aid developers in writing tests, and it includes a set of tools to allow end users to run such tests in an automated manner (without having to have developer-only tools installed!).  ATF is the testing framework currently used in the NetBSD operating system.

-----

A few random things:

* The end-user tools included in ATF are deprecated in favor of Kyua, although the libraries are alive and will still be for a while. A package for Kyua itself will be coming "soon" after this one.


* One thing that makes ATF different from other testing frameworks is that its tools are designed to be able to run "installed tests".  The installation of tests is controversial because the location that ATF currently expects is not within the expectations of the LSB nor the directory structure of Fedora.  Because I would like to make an initial package for Fedora available, I have chosen to NOT install any of the tests provided by ATF.  This way, I can leave the controversial decision for a latter stage, at which point we can make a new revision of the package to include new subpackages to provide the tests.  Some context can be found in this discussion:

    http://lists.fedoraproject.org/pipermail/packaging/2012-February/008128.html

The installation of tests is, of course, optional.  The current package works just fine and allows users to develop their own tests using the libraries.


* Here is the output of rpmlint:

SPECS/atf.spec: W: invalid-url Source0: http://kyua.googlecode.com/files/atf-0.15.tar.gz HTTP Error 404: Not Found
atf.src: W: invalid-url Source0: http://kyua.googlecode.com/files/atf-0.15.tar.gz HTTP Error 404: Not Found
atf.x86_64: W: devel-file-in-non-devel-package /usr/bin/atf-config
libatf-c++.x86_64: W: no-documentation
libatf-c.x86_64: W: no-documentation
9 packages and 1 specfiles checked; 0 errors, 5 warnings.

The warning about invalid-url is expected and can be ignored.

Not sure if the no-documentation warning is important for the subpackages.  There is nothing useful that I could install...

The warning about atf-config is not accurate, because atf-config is a tool for users to query the current configuration of ATF; it's not the same as "pkg-config".  Wish I'd tag this condition in the spec file itself to permanently suppress the warning, but rpmlint does not seem to support this.


* The 'atf' package contains a long description, while all the subpackages contain a tiny description.  I think it'd be nice if all of these subdescriptions contained the generic description of ATF.  However, I'm not sure if that's desirable (standard practice), or if there is a way to do it without having to copy/paste the same text multiple times.


And, as usual, thanks for the review!
Comment 1 Michel Alexandre Salim 2012-04-23 23:43:47 EDT
(In reply to comment #0)
> * The 'atf' package contains a long description, while all the subpackages
> contain a tiny description.  I think it'd be nice if all of these
> subdescriptions contained the generic description of ATF.  However, I'm not
> sure if that's desirable (standard practice), or if there is a way to do it
> without having to copy/paste the same text multiple times.
> 
There are some examples in the Haskell packaging template -- you can define a %global variable for the description text, and then use it in the subpackages:

http://git.fedorahosted.org/git/?p=cabal2spec.git;a=blob;f=spectemplate-ghc-binlib.spec;h=e710d720f1a359e225b2def7270114094368d9bc;hb=HEAD

(will proceed with the review without waiting for this, though -- it's not urgent, and the example file is quite straightforward)
Comment 2 Michel Alexandre Salim 2012-05-01 00:33:27 EDT
I've tested the generated against lutok, and atf-run works fine if libatf-c++-devel is installed when lutok is configured and built.

One change needs to be made to the packaging -- it's generally fine if subpackages do not have documentation, if they depend on the main package -- but in your case, since the libatf-* subpackages are not dependent on atf, some of them need to include at least the license text (COPYING). Basically any installable subset of atf needs to carry the license text.

Looks like adding the LICENSE text to libatf-c is sufficient since all the other subpackages depend on it eventually.

* TODO Review [90%]
  - [X] Names [2/2]
    - [X] Package name
    - [X] Spec name
  - [X] Package version [2/2]
	http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Package_Versioning
    - [X] Version number
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Version_Tag
    - [X] Release tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Release_Tag
	  http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
  - [X] Meets [[http://fedoraproject.org/wiki/Packaging/Guidelines][guidelines]]
  - [X] Source files match upstream
  $ sha256sum atf-0.15.tar.gz ../SOURCES/atf-0.15.tar.gz 
  0c7242a107c7e308feed8fac45a194a6f6c8d90283add576cfc3dab0fcd61b2b  atf-0.15.tar.gz
  0c7242a107c7e308feed8fac45a194a6f6c8d90283add576cfc3dab0fcd61b2b  ../SOURCES/atf-0.15.tar.gz

  - [X] [[http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries][No bundled libraries]]
  - [X] License [4/4]
    - [X] License is Fedora-approved
    - [X] No licensing conflict
    - [X] License field accurate
    - [X] License included iff packaged by upstream
  - [-] rpmlint [1/2]
    - [X] on src.rpm
      ✗ rpmlint ./atf-0.15-1.fc17.src.rpm 
      1 packages and 0 specfiles checked; 0 errors, 0 warnings.
    - [ ] on x86_64.rpm
      ✗ rpmlint ~/Downloads/*atf*.rpm
      atf.x86_64: W: devel-file-in-non-devel-package /usr/bin/atf-config
      libatf-c.x86_64: W: no-documentation
      libatf-c++.x86_64: W: no-documentation
      8 packages and 0 specfiles checked; 0 errors, 3 warnings.
      
      Note: since libatf-c and libatf-c++ do not require the base package,
      you must at least include the COPYING file as documentation

  - [X] Language & locale [2/2]
    - [X] Spec in US English
    - [X] Spec legible
  - [X] Build [3/3]
    - [X] Koji results
      http://koji.fedoraproject.org/koji/taskinfo?taskID=4038205
    - [X] BRs complete
    - [X] Directory ownership
  - [X] Spec inspection [8/8]
    - [X] ldconfig for libraries
    - [X] No duplicate files
    - [X] File permissions
    - [X] Filenames must be UTF-8
    - [X] no BuildRoot ([[https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag][except if targeting RHEL5]])
    - [X] Macro usage consistent
    - [X] Documentation [1/1]
      - [X] %doc files are non-essential
    - [X] Development [4/4]
      - [X] Headers in -devel
      - [X] If versioned .so's, unversioned in -devel
      - [X] -devel, -static requires main
      - [X] No .la
Comment 3 Julio Merino 2012-05-01 21:29:58 EDT
Thanks for the review.

I have uploaded a new copy of the spec and srpm files to the same location.  The output of rpmlint on all the files is now:

atf.spec: W: invalid-url Source0: http://kyua.googlecode.com/files/atf-0.15.tar.gz HTTP Error 404: Not Found
atf.src: W: invalid-url Source0: http://kyua.googlecode.com/files/atf-0.15.tar.gz HTTP Error 404: Not Found
atf.x86_64: W: devel-file-in-non-devel-package /usr/bin/atf-config
9 packages and 1 specfiles checked; 0 errors, 3 warnings.

Things I have changed:

- Added the common_description variable.  It's a pity that the first line of the text has to be listed along the %define directive, as that pushes the line over the 80 column boundary.  Putting a \ in the first line causes a spurious blank line to appear at the beginning of the descriptions, which I find ugly.

- Added missing %defattr sections.

- Added %doc lines to libatf-c, libatf-c++ and libatf-sh.  I know you mentioned that because everything depends on libatf-c, only libatf-c had to include these.  However, these cross-dependencies among the libatf-* packages are a weird artifact, and the resulting binary packages should not rely on them.

Koji results: http://koji.fedoraproject.org/koji/taskinfo?taskID=4043303
Comment 4 Michel Alexandre Salim 2012-05-03 07:21:50 EDT
As (In reply to comment #3)
> - Added the common_description variable.  It's a pity that the first line of
> the text has to be listed along the %define directive, as that pushes the line
> over the 80 column boundary.  Putting a \ in the first line causes a spurious
> blank line to appear at the beginning of the descriptions, which I find ugly.
> 
That's the same problem with Haskell packages, don't worry about it.

> - Added missing %defattr sections.
Actually this is no longer necessary:
http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

RHEL 5 already has rpm 4.4, so unless you really want to target RHEL 4 as well, you only need defattr if you need some unusual permissions

> - Added %doc lines to libatf-c, libatf-c++ and libatf-sh.  I know you mentioned
> that because everything depends on libatf-c, only libatf-c had to include
> these.  However, these cross-dependencies among the libatf-* packages are a
> weird artifact, and the resulting binary packages should not rely on them.
> 
> Koji results: http://koji.fedoraproject.org/koji/taskinfo?taskID=4043303

Sounds good, I was only suggesting the bare minimum.

APPROVED
Comment 5 Julio Merino 2012-05-03 13:12:35 EDT
New Package SCM Request
=======================
Package Name: atf
Short Description: Automated Testing Framework
Owners: jmmv
Branches: f17
InitialCC:
Comment 6 Gwyn Ciesla 2012-05-03 13:19:20 EDT
Git done (by process-git-requests).
Comment 7 Julio Merino 2012-05-03 14:51:40 EDT
Builds for rawhide and f17 have been requested. I think this can be closed now.

Thanks a lot for the review and the advice!
Comment 8 Michel Alexandre Salim 2012-05-04 00:53:08 EDT
You're welcome! Don't forget to submit an update request -- F17 is branched so packages for it do not automatically enter the repo anymore.
Comment 9 Fedora Update System 2012-05-05 13:07:59 EDT
atf-0.15-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/atf-0.15-1.fc17
Comment 10 Fedora Update System 2012-05-26 03:40:37 EDT
atf-0.15-1.fc17 has been pushed to the Fedora 17 stable repository.
Comment 11 Julio Merino 2013-02-14 15:39:15 EST
Package Change Request
======================
Package Name: atf
New Branches: f18
Comment 12 Gwyn Ciesla 2013-02-14 16:04:16 EST
Misformatted request, no owner specified.
Comment 13 Julio Merino 2013-02-14 17:02:03 EST
Package Change Request
======================
Package Name: atf
New Branches: f18
Owners: jmmv
Comment 14 Gwyn Ciesla 2013-02-14 17:15:45 EST
f18 branch already exists.

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