Bug 182175 - Review Request: libast - handy routines and drop-in substitutes for some good-but-non-portable functions (needed by eterm)
Review Request: libast - handy routines and drop-in substitutes for some good...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Package Reviews List
: Reopened
Depends On:
Blocks: FE-ACCEPT 182173
  Show dependency treegraph
 
Reported: 2006-02-20 16:25 EST by Terje Røsten
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-24 14:33:42 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2006-02-20 16:25:03 EST
Spec file: http://web.phys.ntnu.no/~terjeros/eterm/libast.spec
SRPM: http://web.phys.ntnu.no/~terjeros/eterm/libast-0.7-1.src.rpm

Description: 
LibAST is the Library of Assorted Spiffy Things.  It contains various
handy routines and drop-in substitutes for some good-but-non-portable
functions.  It currently has a built-in memory tracking subsystem as
well as some debugging aids and other similar tools.

The reason to include this package is as needed lib. by eterm, see another
Package Review Request.
Comment 1 Jochen Schmitt 2006-02-21 11:43:49 EST
Bad:

- rpmlint of source rpm failed.

pmlint libast-0.7-1.src.rpm
E: libast tag-not-utf8 %changelog
E: libast non-utf8-spec-file libast.spec

rpmlint libast-0.7-1.i686.rpm
E: libast tag-not-utf8 %changelog

rpmlint libast-devel-0.7-1.i686.rpm
E: libast-devel tag-not-utf8 %changelog


- Local build show following warning:

configure: WARNING: *** Imlib2 support has been disabled because Imlib2 ***
configure: WARNING: *** was not found or could not be linked.           ***
Comment 2 Jochen Schmitt 2006-02-21 12:33:30 EST
Further Bads:

- BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- The BuildRoot must be cleaned at the beginning of %install
? Encoding should be UTF-8
- Test of /sbin/ldconfig in %post and %pustun is unnecessary.
- Static libs in devel package.
- Missing text of the license in %doc.
Comment 3 Terje Røsten 2006-02-21 16:59:13 EST
OK, thanks for the feedback, fixed most problems, however I am unsure how to
deal with 

- Missing text of the license in %doc.

there are no LICENSE file in the tarball, but every .c file has this header (see
below).

Any hints?

/*
 * Copyright (C) 1997-2004, Michael Jennings
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to
 * deal in the Software without restriction, including without limitation the
 * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
 * sell copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in
 * all copies of the Software, its documentation and marketing & publicity
 * materials, and acknowledgment shall be given in the documentation, materials
 * and software packages that this Software was used.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
 * THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
 * IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 */
Comment 4 Jochen Schmitt 2006-02-22 09:48:09 EST
I think at first you should ask the upstream author to include a LICENSE file 
into the tar ball.
Comment 5 Jochen Schmitt 2006-02-22 10:04:14 EST
A fast solution may be to copy the license text from the source file into a 
LICENSE file, which may be included via the Source1 tag into the SPEC file.
Comment 6 Michael Jennings (KainX) 2006-02-22 23:56:47 EST
Or you could just get rid of your silly rules which discriminate against
packages for no real reason.

The license is clearly posted in the spec file and in every .c file in the
package.  The correct procedure for checking the license of a package on an
RPM-based system is "rpm -q --qf '%{LICENSE}\n' <package>".  If that works and
returns a standard response ("GPL," "BSD," "MIT," or similar), there should be
no problem.

Furthermore, the following requirements are just stupid and demonstrate either
pointless fascism on the part of your policy makers or flaws in the design of
your build system:

> - BuildRoot should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> - The BuildRoot must be cleaned at the beginning of %install

The build system, not the individual RPM's, should ensure that the buildroot
path is unique and clean prior to invoking rpmbuild.
Comment 7 Paul Howarth 2006-02-23 02:59:41 EST
(In reply to comment #6)
> Or you could just get rid of your silly rules which discriminate against
> packages for no real reason.
> 
> The license is clearly posted in the spec file and in every .c file in the
> package.  The correct procedure for checking the license of a package on an
> RPM-based system is "rpm -q --qf '%{LICENSE}\n' <package>".  If that works and
> returns a standard response ("GPL," "BSD," "MIT," or similar), there should be
> no problem.

The point of the rule is to ensure that the license tag in the RPM matches the
actual license of the upstream package; that's something the reviewer needs to
check.

IMHO the presence of the license in the source files itself satisfies the "text
included" requirement and there's no need for a separate LICENSE file.

> Furthermore, the following requirements are just stupid and demonstrate either
> pointless fascism on the part of your policy makers or flaws in the design of
> your build system:
> 
> > - BuildRoot should be
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> > - The BuildRoot must be cleaned at the beginning of %install
> 
> The build system, not the individual RPM's, should ensure that the buildroot
> path is unique and clean prior to invoking rpmbuild.

Ideally yes, but rpm doesn't do this by default so it has to be done in each
package. Even if rpm was changed to do this automatically, packages desiring
compatibility with older distributions would still need to clean the buildroot
themselves.
Comment 8 Jochen Schmitt 2006-02-23 10:11:40 EST
(In reply to comment #7)

>the point of the rule is to ensure that the license tag in the RPM matches the
>actual license of the upstream package; that's something the reviewer needs to
>check.
>IMHO the presence of the license in the source files itself satisfies 
>the text.

But anyoone whoe only get the binary package doen't recieve a verbatry copy of
the license, so you don't aware about you rights given on the license.

Event a pointer to a URL is not enough, becouse you don't know, if the URL will
be exist in the future and for the other hand, the text publish on the URL
may be changend in the funter. But for the reciever of the package only the 
contest of the license text of the time of packaging may be relevant.

Comment 9 Terje Røsten 2006-02-26 13:34:02 EST
New updated files available:

Spec: http://web.phys.ntnu.no/~terjeros/eterm/libast.spec
SRPM: http://web.phys.ntnu.no/~terjeros/eterm/libast-0.7-3.src.rpm

Most problems should be fixed.
Comment 10 Jochen Schmitt 2006-03-05 15:29:37 EST
Good:

+ rpmlint for source rpm ok.
+ local build work fine.
+ rpmlint of binaries rpm fine.
+ mock build worked fine.

your package is APPROVED.


Comment 11 Jochen Schmitt 2006-04-20 10:17:46 EDT
Please close this bug.
Comment 12 Terje Røsten 2006-04-22 13:09:52 EDT
Seems like Eterm has too confusing license terms for Extras, see #182173.

I give up and setting WONTFIX, sorry.
Comment 13 Ignacio Vazquez-Abrams 2006-04-22 13:20:27 EDT
Eterm may not be able to get in, but would it be worth having libast regardless?
Comment 14 Terje Røsten 2006-04-22 13:29:03 EDT
Maybe, however I don't know of any software project using the lib except Eterm.

If anyone will use the spec file I have created, they are welcome...

Comment 15 Michael Jennings (KainX) 2006-08-23 14:49:07 EDT
(In reply to comment #7)
> Ideally yes, but rpm doesn't do this by default so it has to be done in each
> package.

One, RPM *does* do this by default now.

Two, RPM is a package manager, not a build system.  You'll note I said "build
system."

> Even if rpm was changed to do this automatically, packages desiring
> compatibility with older distributions would still need to clean the buildroot
> themselves.

Not if you're using a sane build system, like Mezzanine, which handles all that
for you like a good build system should.

For what it's worth, there's now a LICENSE file in LibAST.
Comment 16 Terje Røsten 2006-09-02 12:50:37 EDT
(In reply to comment #15)
> For what it's worth, there's now a LICENSE file in LibAST.

URL?
 
$ wget http://www.eterm.org/download/libast-0.7.tar.gz
$ tar tzvf libast-0.7.tar.gz | grep -ic LIC             
0
Comment 17 Michael Jennings (KainX) 2006-09-02 13:13:51 EDT
(In reply to comment #16)
> URL?
>  
> $ wget http://www.eterm.org/download/libast-0.7.tar.gz
> $ tar tzvf libast-0.7.tar.gz | grep -ic LIC             
> 0

Sorry, should've specified.  libast 0.7.1 in CVS has a LICENSE file.  You can
pull the tarball from the cAos SRPM if you'd like:

http://mirror.caosity.org/cAos-2/ext/autobuilder/i386/00_LOGS/e/libast/SRPMS/libast-0.7.1-0.20060818.src.rpm
Comment 18 Terje Røsten 2006-09-02 16:05:27 EDT
(In reply to comment #17)
> Sorry, should've specified.  libast 0.7.1 in CVS has a LICENSE file.  You can
> pull the tarball from the cAos SRPM if you'd like:
>
http://mirror.caosity.org/cAos-2/ext/autobuilder/i386/00_LOGS/e/libast/SRPMS/libast-0.7.1-0.20060818.src.rpm

Thanks, new package available:

SPEC: http://web.phys.ntnu.no/~terjeros/eterm/libast.spec
SRPM: 
 http://web.phys.ntnu.no/~terjeros/eterm/libast-0.7.1-0.1.20060818cvs.src.rpm
Comment 19 Kevin Fenzi 2006-10-01 16:18:37 EDT
What state is this review in? I suspect it needs the FE-NEW and FE-NEEDSPONSOR 
blockers. Without those, no one is likely to see it. 


Comment 20 Ed Hill 2006-10-01 22:16:14 EDT
Hi Terje, heres another review of the latest version:

  sha1sum:
    b2a70e12f25099c4565f54fae7a25e66e478a22f  
    libast-0.7.1-0.1.20060818cvs.src.rpm

 + rpmlint reports: "W: libast-devel no-documentation"
   which can be safely ignored
 + spec file name and package name OK
 + license OK and correctly included
 + spec is legible and looks sane
 + source appears to match upstream (pulled from CVS)
 + builds in mock for FC5 i386
 + no locale(s)
 + shared lib handling looks OK
 + no *.la or *.a
 + not relocatable
 + dir ownership OK
 + no duplicate files
 + permissions look OK
 + clean OK
 + macros look OK
 + code not content
 + no large docs
 + no runtime doc dependencies
 + correct use of -devel

There were a few warnings during the compile [mostly, ignored return types 
and pointer type mismatches] but I don't see any actual blockers.  This is
somewhat redundant (since Jochen already approved in comment #10 but he is 
not currently a sponsor):

APPROVED.

So if you haven't already been sponsored then please go ahead and request 
sponsorship and I'll approve it.

And I'll look at the updated Eterm submission next...
Comment 21 Ed Hill 2006-10-14 12:32:11 EDT
Hi Terje, are you still interested in this submission?

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