Bug 225679 - Merge Review: dejagnu
Summary: Merge Review: dejagnu
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Miroslav Lichvar
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 17:56 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-04 15:25:21 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mlichvar: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 17:56:22 UTC
Fedora Merge Review: dejagnu

http://cvs.fedora.redhat.com/viewcvs/devel/dejagnu/
Initial Owner: pmachata@redhat.com

Comment 1 Roozbeh Pournader 2007-02-05 14:40:51 UTC
BLOCKER: spec filename is not %{name}.spec

From the review guidelines:
  MUST: The spec file name must match the base package %{name}, in the format
  %{name}.spec


Comment 2 Petr Machata 2007-02-07 16:45:19 UTC
Spec renamed, commited into cvs, not built.

$ rpmlint noarch/dejagnu-1.4.4-6.noarch.rpm
W: dejagnu devel-file-in-non-devel-package /usr/share/dejagnu/testglue.c
W: dejagnu devel-file-in-non-devel-package /usr/include/dejagnu.h
W: dejagnu devel-file-in-non-devel-package /usr/share/dejagnu/stub-loader.c

These are ok, dejagnu is actually a development package.  These files are
compiled during the course of dejagnu's runtime.

Other than that, rpmlint is silent, for both source and binary rpm.

Comment 3 Miroslav Lichvar 2007-09-06 14:03:24 UTC
I'll look into this.

Comment 4 Miroslav Lichvar 2007-09-07 11:28:59 UTC
Ok:
- rpmlint output
  W: dejagnu devel-file-in-non-devel-package /usr/share/dejagnu/testglue.c
  W: dejagnu devel-file-in-non-devel-package /usr/include/dejagnu.h
  W: dejagnu devel-file-in-non-devel-package /usr/share/dejagnu/stub-loader.c
  - the *.c files are part of the testsuite
  - dejagnu.h could be placed in devel subpackage, but in this case I think it's
ok to keep it as it is
- the package is named according to the Package Naming Guidelines
- the spec file name matches the base package %{name}
- the package is licensed with a Fedora approved license
- the License field in the package spec file matches the actual license (GPLv2+)
- file containing the text of the license(s) for the package is included in %doc
- the spec file is written in American English
- the spec file for the package is legible
- the sources used to build the package match the upstream source
- the package owns all directories that it creates
- the package does not contain any duplicate files in the %files listing
- permissions on files are set properly
- the package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT
- the package consistently uses macros
- the package contains code, or permissible content
- files included as %doc don't affect the runtime of the application
- the package does not own files or directories already owned by other packages
- at the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT
- all filenames in rpm packages are valid UTF-8

Need some work:
- -n dejagnu-%{version} in %setup isn't necessary
- make in %build can be removed, there is nothing to build
- expect dependency doesn't need to use a version, 5.21 is very old
- tcl dependency is not required, expect depends on tcl
- jadetex docbook-utils-pdf tetex-dvips are not required during build, the
tarball contains generated overview.pdf 
- mv doc/html doc/overview doesn't seem to be necessary
- please use make DESTDIR=$RPM_BUILD_ROOT install-man for man installation
- BuildArchitecture should be just BuildArch
- %{version} could be used in Source
- expect should be in BuildRequires for %check. But the test suite seems to have
tests that are supposed to fail, so I'd suggest removing %check completely or
modifying the tests so make check doesn't need -k and || :, and regressions will
be caught in the build


Comment 5 Petr Machata 2007-10-03 22:35:53 UTC
I committed version cleaned up per your comments.

The testsuite problem is twofold: first, dejagnu self-testing module was
handling C++ streams erroneously, which was resolved with a simple patch;
second, mock doesn't give terminal to testsuite when it needs one.  This was
resolved with an ugly convoluted setup involving screen and temporary file.  I'd
like you to re-review the package again if possible, and either recommend better
alternative, or ack current approach.  Thanks.

Comment 6 Miroslav Lichvar 2007-10-04 15:25:21 UTC
Well, using screen in build is pretty ugly, but I can't think of a simpler way
how to provide a tty to the script.

APPROVED.

Comment 7 Miroslav Lichvar 2007-10-05 14:10:42 UTC
Ok, after a bit of thought, the screen hack can be replaced with script from
util-linux-ng. It would look better in spec. ;)


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