Bug 441759 - Review Request: mono-cecil-flowanalysis - Flowanalysis engine for Cecil
Review Request: mono-cecil-flowanalysis - Flowanalysis engine for Cecil
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alex Lancaster
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-09 16:56 EDT by Tom "spot" Callaway
Modified: 2008-04-12 18:46 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-12 18:46:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
alexl: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Comment 1 Alex Lancaster 2008-04-10 08:53:18 EDT
 - Package meets naming and packaging guidelines: YES
 - Spec file matches base package name: YES
 - Spec has consistant macro usage: YES
 - Meets Packaging Guidelines: YES
 - License: YES, MIT,
http://anonsvn.mono-project.com/viewcvs/trunk/cecil/flowanalysis/README?view=markup
 - License field in spec matches: YES
 - License file included in package: YES, text included in README
 - Spec in American English: YES
 - Spec is legible.: YES
 - Sources match upstream md5sum:
No source upstream, only SVN.  Tried verifying the tarball, but it's only in
.tar.gz format, whereas the tarball in package is .tar.bz2, so won't match.
Should probably add comment in spec file about exactly which SVN revision used
as per http://fedoraproject.org/wiki/Packaging/SourceURL
 - Package needs ExcludeArch: YES, ppc64
 - BuildRequires correct: YES, mono-devel
 - Spec handles locales/find_lang: N/A
 - Package is relocatable and has a reason to be. N/A
 - Package has %defattr and permissions on files is good.: YES
 - Package has a correct %clean section.: YES
 - Package has correct buildroot: YES
    %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
 - Package is code or permissible content. YES
 - Doc subpackage needed/used.: N/A
 - Packages %doc files don't affect runtime. YES
 - Headers/static libs in -devel subpackage. N/A
 - Package is a GUI app and has a .desktop file N/A
 - Package compiles and builds on at least one arch: YES.  successful koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=560739

 - Package has no duplicate files in %files. YES
 - Package doesn't own any directories other packages own. YES
 - Package owns all the directories it creates. YES
 - rpmlint output:
mono-cecil-flowanalysis.i386: E: no-binary
mono-cecil-flowanalysis.i386: E: only-non-binary-in-usr-lib
these can be ignored as per 
http://fedoraproject.org/wiki/Packaging/Mono

 - final provides and requires are sane:

mono(Mono.Cecil) = 0.5.0.1
mono(mscorlib) = 1.0.5000.0
mono(Cecil.FlowAnalysis) = 0.1.0.0
mono-cecil-flowanalysis = 0.1-0.1.20080409svn100264.fc8

 no bogus provides there it appears, unlike certain other packages

SHOULD Items:

 - Should build in mock. YES (see koji build above)
 - Should build on all supported archs: YES (see koji)
 - Should have dist tag: YES
 - Should package latest version: YES (SVN version)

Issues:
1. Add comment in spec showing which exact SVN version used for this particular
 tarball as per:
http://fedoraproject.org/wiki/Packaging/SourceURL#head-615f6271efb394ab340a93a6cf030f2d08cf0d49

Fix this and I think I can approve.
Comment 2 Alex Lancaster 2008-04-10 09:00:52 EDT
Also, I just noticed that there are some testcases in the SVN tree, is it worth
packaging those and running them before installation or not?  Are these unit
tests for the app?  Probably could be done after import as well.
Comment 3 Tom "spot" Callaway 2008-04-11 11:12:47 EDT
New Package CVS Request
=======================
Package Name: mono-cecil-flowanalysis
Short Description: Flowanalysis engine for Cecil
Owners: spot
Branches: F-7 F-8 F-9 devel
InitialCC: 
Cvsextras Commits: yes
Comment 4 Tom "spot" Callaway 2008-04-11 11:14:10 EDT
Whoops. I did cvs before finishing the review. :/

Here's the new spec file, does it look ok?

http://www.auroralinux.org/people/spot/review/new/mono-cecil-flowanalysis.spec
Comment 5 Alex Lancaster 2008-04-11 21:26:36 EDT
(In reply to comment #4)
> Whoops. I did cvs before finishing the review. :/
> 
> Here's the new spec file, does it look ok?
> 
> http://www.auroralinux.org/people/spot/review/new/mono-cecil-flowanalysis.spec


I hadn't noticed that the source contained a .pc pkgconfig file earlier, so well
spotted.  It looks to be correctly packaged in the separate -devel subpackage as
per:

http://fedoraproject.org/wiki/Packaging/Mono

So, looks fine.  APPROVED.

I've also re-raised the fedora-cvs flag to save some time.

Comment 6 Tom "spot" Callaway 2008-04-12 11:22:46 EDT
cvs is done.
Comment 7 Tom "spot" Callaway 2008-04-12 18:46:30 EDT
Built, thanks for the review.

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