Bug 229728 - Review Request: polyml - Poly/ML compiler and runtime system
Summary: Review Request: polyml - Poly/ML compiler and runtime system
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Bryan O'Sullivan
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-02-22 23:16 UTC by Gérard Milmeister
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-04-20 17:16:52 UTC
Type: ---
Embargoed:
bos: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Gérard Milmeister 2007-02-22 23:16:54 UTC
Spec URL: http://math.ifi.unizh.ch/fedora/spec/polyml.spec
SRPM URL: http://math.ifi.unizh.ch/fedora/6/i386/SRPMS.gemi/polyml-5.0-1.src.rpm
Description:
Poly/ML is a full implementation of Standard ML available as
open-source.  This release supports the ML97 version of the language
and the Standard Basis Library.

Comment 1 Bryan O'Sullivan 2007-03-27 03:18:50 UTC
This package has a few problems that need to be addressed.

The spec file contains wild cards in the %files section.  This makes it
difficult to verify that the package contains all the files it should.

The docs are huge (2 megabytes).  Please split them into a separate -doc package.

There are some rpmlint warnings:

W: polyml devel-file-in-non-devel-package /usr/lib64/libpolyml.so
W: polyml devel-file-in-non-devel-package /usr/lib64/libpolymain.a

These ought to be in a separate -devel package, I think.

In the spec file, please compress the BuldRequires lines into a single entry. 
Please provide a proper description for the -libs package.

Thanks.

Comment 2 Gérard Milmeister 2007-03-27 11:31:22 UTC
(In reply to comment #1)
> The spec file contains wild cards in the %files section.  This makes it
> difficult to verify that the package contains all the files it should.
ok, in moderation

> The docs are huge (2 megabytes).  Please split them into a separate -doc package.
ok

> There are some rpmlint warnings:
> 
> W: polyml devel-file-in-non-devel-package /usr/lib64/libpolyml.so
> W: polyml devel-file-in-non-devel-package /usr/lib64/libpolymain.a
Since polyml is a compiler, development files are ok (see also gcc, etc.)

> In the spec file, please compress the BuldRequires lines into a single entry. 
I prefer it this way, it is much clearer. I would even go so far as disallow
more than one entry per line.

> Please provide a proper description for the -libs package.
ok

http://math.ifi.unizh.ch/fedora/6/i386/SRPMS.gemi/polyml-5.0-2.src.rpm


Comment 3 Bryan O'Sullivan 2007-03-27 17:14:06 UTC
You write:

> I prefer it this way, it is much clearer. I would even go so far as disallow
> more than one entry per line.

Please understand that the issue is not what you prefer, but that folding
multiple depends clauses onto a single line is the norm for Fedora spec files. 
Please follow the established norm instead of trying to argue why your
preference should take precedence.

Thanks for your other changes.

Comment 4 Gérard Milmeister 2007-03-27 18:11:02 UTC
What makes you believe that this is the norm for Fedora spec files. All of my
packages use one BR per line, except for some older ones.

Comment 5 Dominik 'Rathann' Mierzejewski 2007-03-27 18:15:48 UTC
Mine too. It vastly improves readability and reduces diff size if BRs change.

Comment 6 Mamoru TASAKA 2007-03-27 18:54:54 UTC
Actually writing many BRs in one line really confuses me...

While some closely related packages may be put in one
line, please imagine what happens if a package has 29 BuildRequires
(yes, I reviewed such package!!)

Especially, when BRs change as Dominik comments, it makes
difficult to catch what is actually changed if all BRs are
kept in one line...

Comment 7 Bryan O'Sullivan 2007-04-18 05:36:09 UTC
Here is my review.

MUST:

+ rpmlint looks acceptable.

  W: polyml devel-file-in-non-devel-package /usr/lib64/libpolyml.so
  W: polyml devel-file-in-non-devel-package /usr/lib64/libpolymain.a
  W: polyml-libs no-documentation

+ Meets packaging guidelines.
+ Name of spec file is OK.
+ License is OK (LGPL).
+ License file content matches license.
+ License file is in both source package and %doc.
+ Spec file is in English.
+ Spec file is legible.
+ md5sums match.
+ Builds on x86_64.
+ BuildRequires looks OK.
+ Locales not an issue, since not used.
+ ldconfig is run correctly for -libs package.
+ Not relocatable.
+ Directory ownership OK.
+ No duplicate file names.
+ File permissions look OK.
+ %clean is present.
+ Macro use is consistent.
+ Package contents are permissible.
+ Docs are in -doc package.
+ Docs don't affect runtime.
+ Headers are in -devel package.
+ No libtool turds.
+ No incorrect directory ownership.
+ %install cleans buildroot.
+ File names are UTF-8.

Don't follow MUST guidelines, but acceptable since this is a compiler:

? Static library is not in -static package.
? The symlink %{_libdir}/libpolyml.so in the main package refers to
  libpolyml.so.0.0.0 in the -libs package.

Accordingly, package is APPROVED.

Comment 8 Gérard Milmeister 2007-04-18 16:05:20 UTC
New Package CVS Request
=======================
Package Name: polyml
Short Description: Poly/ML compiler and runtime system
Owners: gemi
Branches: FC-6
InitialCC: gemi

Comment 9 Gérard Milmeister 2007-04-20 17:16:52 UTC
Built on FC6 and devel for i386, x86_64 and ppc.
Thanks for the review!


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