Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225679 - Merge Review: dejagnu
Summary: Merge Review: dejagnu
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Miroslav Lichvar
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
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:
Clone Of:
Environment:
Last Closed: 2007-10-04 15:25:21 UTC
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.