[Bug 459878] Review Request: genome - Package for the Genome Project

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=459878





--- Comment #17 from Jeroen van Meeuwen <kanarip at kanarip.com>  2008-10-30 07:17:08 EDT ---
(In reply to comment #15)
> Well, from very quick glance:
> 
> * Using /var/www is not allowed on Fedora
>   https://fedoraproject.org/wiki/Packaging/Guidelines#Web_Applications
> 

Moved to /usr/share/genome-styling/{html,scripts}

> * License tag should be "GPLv2+"
>   Note that only putting GPLv2 license text does not mean
>   that the license is under GPLv2:
>   http://fedoraproject.org/wiki/Licensing/FAQ
> 

Fixed.

> * Usually dependencies between main package and subpackages
>   must be EVR (Epoch-Version-Release) specific
>   (ref:
>    https://fedoraproject.org/wiki/Packaging/ReviewGuidelines
>    "MUST: In the vast majority of cases, devel packages..."
>    This mentions only main <-> -devel packages, however
>    this should usually be applied to any binary rpms generated
>    from the same srpm)
> 

Fixed.

> * Virtual Provides must usually be EVR specific (Vitual Provides
>   without EVR information caused upgrading path problem many
>   times...)
> 

Fixed.

> * "Requires: genome" is redundant when "Requires: gnome-lib" is
>   present (currently, however see below)
> 
> * I guess that the dependency between genome and genome-lib
>   is opposite
> 

It allows for faster dep-solving if Requires: genome is in the requires for a
package next to Requires: genome-lib, because the "new" deps won't have to be
looped through (the dep is already resolved during the first run in the loop)

> * You have to write "BuildArch: noarch" only on the main
>   package (and don't have to write on each subpackages)
> 

Removed the redundant BuildArch: noarch

> * Use "BuildRequires: rubygem(rake)", not rubygem-rake.
>   c.f.
>   https://fedoraproject.org/wiki/Packaging/Perl#Perl_Requires_and_Provides
>   - By the way "BuildRequires: rubygem" is redundant when
>     "BuildRequires: rubygem(rake)" is present.
> 

Fixed.

> * When using "cp" or "install" commands, add "-p" option
>   to keep timestamps on installed files:
>   https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
> 

Fixed.

> * The scripts related to SysVinit script are wrong at
>   several points.
>   Also some Requires(preun) or so are missing.
> 
>   Please refer to:
>  
> https://fedoraproject.org/wiki/Packaging/SysVInitScripts#Initscripts_in_spec_file_scriptlets
>   https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax
> 

Fixed.

> * Please make it sure that directories which are created when
>   installing a package are correctly owned by the package.
>   There are some directories which are not owned by any packages
>   or when the package is installed:
>  
> https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
>   https://fedoraproject.org/wiki/Packaging/UnownedDirectories
> 

I think I got this one nailed but I'd appreciate some verification.

> * We recommend %defattr(-,root,root,-)
>   Also on some subpackages %defattr is completely missing
>   (please use rpmlint to detect this type of packagins error)
> 

rpmlint does not say anything about missing defattr's

> * Files under %_datadir/doc are automatically marked as %doc
> 

I'm not sure what this means. I get what it means but I'm not sure what it
applies to as far as the package is concerned.

> * %_sysconfdir/init.d must be %_initrddir
>  
> https://fedoraproject.org/wiki/Packaging/SysVInitScripts#Initscripts_on_the_filesystem
> 

Fixed.

> * /var must be %_localstatedir or so:
>   https://fedoraproject.org/wiki/Packaging/RPMMacros
> 

Fixed

> * Please use macros consistently. For example at one place %{_bindir}
>   is used (this is no problem because %{_bindir} is expanded as /usr/bin)
>   where at other place /%{_bindir} (i.e. //usr/bin) is used.
> 

Which ends up being the same but I see what you mean; in accumulative paths
though, such as %{buildroot}/%{_bindir} I do like to use the possibly redundant
'/' (it's better to my eyes *and* I'm not dependent on whether %{_bindir} or
%{bindir} has a prepending slash or is fully qualified or not).

In %files sections though; fixed

> I just glanced at your spec file quickly and examined license issue and
> have not checked the details of this package.

The largest part of the spec file comes directly from upstream, Red Hat's
Emerging Technologies team, which is also the reason I've not closely examined
the License that they choose to initially package genome with.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.