Bug 445126

Summary: Review Request: mirrormanager - Fedora MirrorManager server and client
Product: [Fedora] Fedora Reporter: Jon Stanley <jonstanley>
Component: Package ReviewAssignee: Jon Stanley <jonstanley>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, matt_domsch, notting, paul, tjb
Target Milestone: ---Flags: wolfy: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-11-05 23:03:30 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Jon Stanley 2008-05-04 01:32:28 EDT
Spec URL: http://jstanley.fedorapeople.org/report-mirror.spec
SRPM URL: http://jstanley.fedorapeople.org/report-mirror-0.1-2.git20080504.fc8.src.rpm

This is the client for MirrorManager, the Fedora mirror management system.
The configuration file should be edited prior to using this package.  For
more information, For further information, please see:

[jstanley@rugrat ~]$ rpmlint /var/lib/mock/fedora-8-i386/result/report-mirror-0.1-2.git20080504.fc8.noarch.rpm
[jstanley@rugrat ~]$ 

I need a sponsor :).  I'm also copying Matt Domsch on this request, since he is the upstream author.

A little about me:  I'm currently heading up the bug triage team within Fedora, and noticed that this wasn't packaged, but probably should be.  Let me know if further information is required.
Comment 1 Jon Stanley 2008-05-04 01:35:24 EDT
One thing that I'm not sure about in this package is the version number.  Matt,
can you help me out here?  Is there some canonical "released version" of MM that
I can base the version number off of?
Comment 2 Jeroen van Meeuwen 2008-05-04 02:02:03 EDT
RPM Lint: OK
Package name: OK
Spec file: OK
License: OK
Actual License: OK
%doc License: OK
Spec file language: OK
Spec file readable: OK
Upstream source vs. used tarball: No tarball available from upstream, no
instructions included to retrieve this exact same source from upstream
Compile and Build:
 - F-7: OK
 - F-8: OK
 - rawhide: OK
 - EL-5: OK

Applicable Package Guidelines: OK

Locales: N/A
Shared libs: N/A

Relocatable: N/A
Directory and file ownership: OK
No duplicate files in %files: OK
File Permissions: OK
Macro usage: OK
Code vs. Content: OK
(Large) Documentation: OK
%doc affecting runtime: OK
Header files in -devel package: N/A
Static Libraries in -static package: N/A
pkgconfig Requires: N/A
Library files: N/A
Devel requires base package: N/A
.la libtool archives: N/A
Duplicate ownership of files/directories: OK
Remove BuildRoot: OK
UTF-8 filenames: OK

Review pending on the instructions in the .spec file on how to retrieve this
source from upstream's SCM.
Comment 4 Jeroen van Meeuwen 2008-05-04 10:41:04 EDT
Congratulations ;-) Time to make CVS request and get sponsored
Comment 5 Ralf Corsepius 2008-05-04 12:22:06 EDT
MUSTFIX: Package uses hardcoded /usr/sbin, /etc. It should use %_sbindir,
Comment 6 Matt Domsch 2008-05-04 14:04:48 EDT
Jon, thanks for doing this.

I haven't made release tarballs of MirrorManager, as so far the only running
instance of it has been in Fedora Infrastructure.  So, just add instructions for
pulling it from git, as of a particular commit.

As for package name, instead of report-mirror, how would 
mirrormanager-report-mirror sound?  Ideally, there would be 3 subpackages built
from a single source checkout:  mirrormanager; mirrormanager-mirrorlist-server;
mirrormanager-report-mirror.  But yes it'll make for a more involved spec file.
Comment 7 Jon Stanley 2008-05-04 14:45:34 EDT
Yeah, I wanted to get this one out the door first, since packaging the server is
going to probably be a bit more complex, since there are varying licenses for
various pieces, and I'm not exactly sure what Apache configuration might be
required to make it functional.  Also, as you said, the only installation is in
Fedora Infrastructure right now, and this piece is usable beyond that.

I'll put it on the TODO list to package the whole thing, and maybe we'll get
some installations outside of infrastructure!  In the meantime, I'll change the
name of the package.  The thinking for this name was that
mirrormanager-report-mirror could obsolete it when it's ready. but come to think
of it, a newer NVR should take care of that, even if the SRPM is different. 
Comment 8 Matt Domsch 2008-05-04 16:57:07 EDT
fair enough.
Comment 9 Jon Stanley 2008-05-06 13:02:11 EDT
Removing need sponsor blocker since I've been sponsored.  This review though is
going to be obsolete, since my sponsor mentioned that it's going to be hard to
fix this mess if this gets in and then I package the entirety of MM.  I'll
package the rest this weekend or so if I have time.
Comment 10 Jon Stanley 2008-05-15 11:31:35 EDT
I haven't had time to package the rest of MM (it's going to require some patches
to make it comply to the TurboGears packaging standard).  I just noticed someone
else CC'ed themselves to this review.  Would it be an issue to stub out an MM
package, and just have the 'client' subpackage for now?  The newer package would
have a later NVR, wouldn't have to dead.package and block this one, and all in
all a cleaner solution.

Or must the entire thing be packaged at once?
Comment 11 Ralf Corsepius 2008-05-15 12:00:37 EDT
(In reply to comment #10)
> I haven't had time to package the rest of MM (it's going to require some patches
> to make it comply to the TurboGears packaging standard).  I just noticed someone
> else CC'ed themselves to this review.  Would it be an issue to stub out an MM
> package, and just have the 'client' subpackage for now?
That's up to you. Does the client without a server make any sense?

>  The newer package would
> have a later NVR, wouldn't have to dead.package and block this one, and all in
> all a cleaner solution.
This won't fly, if the "client" and the "server" share the same upstream source
tarball (and thus src.rpm-package-name). 

In this case, it's much easier to build everything from this one tarball and to
gradually add subpackages being built from a common tarball "when they are
ready". ... or (and preferred) resubmit your package when you have "finished
packaging" and consider it "done" ... It's what almost all other contributors do.

> Or must the entire thing be packaged at once?
That's the preferred way. The real decision however is tied to 
"upstream", whether they ship one single tarball or several tarballs.
Comment 12 Matt Domsch 2008-09-26 16:13:27 EDT
as upstream maintainer, I have now packaged this into tarballs and prepared it a spec for it, as one SRPM (mirrormanager) with two RPMs (mirrormanager - the server side, and mirrormanager-client - the downstream mirror side).

http://domsch.com/linux/fedora/mirrormanager has the SPEC and SRPM for review.

Jon and/or Jeroen, would you mind reviewing this?

Comment 13 manuel wolfshant 2008-09-27 14:09:43 EDT
Source URL seems incorrect:
https://fedorahosted.org/releases/m/i/mirrormanager/mirrormanager-1.2.2.tar.bz2 gives:
      The requested URL /releases/m/i/mirrormanager/mirrormanager-1.2.2.tar.bz2 was not found on this server.
Which is no surprise, since https://fedorahosted.org/releases/m/i/mirrormanager/ is empty

Is  /usr/share/mirrormanager/mirrors/mirrors really the intended path ? It looks a bit odd.
Comment 14 Matt Domsch 2008-09-27 14:57:33 EDT
wolfy: thanks for the review.  I'll get the tarball posted soon; 1.2.2 was buggy and useless, so I didn't post the tarball.

As for the mirrors/mirrors part, yes, that is intentional, though I understand it looks odd.  It stems from the very earliest days of the code, before it was named mirrormanager.
Comment 15 Matt Domsch 2008-09-29 00:35:12 EDT
I've tagged a version 1.2.3 and uploaded a new spec and the tarball to the Source0 URL as one would expect.  I have rearranged the upstream source tree to eliminate the whole mirrors/mirrors part of the directory name, and added a few Requires that some scripts brought to light.

Comment 16 manuel wolfshant 2008-09-30 08:58:43 EDT
I've done a local mock build and except of %Source0 (which once again does not match the content of https://fedorahosted.org/releases/m/i --- there is an extra mirrormanager/ folder in the way ) everything seems fine.
If Jon does not have the time for a review and/or you want a fast forward, let me know.
Comment 17 Jon Stanley 2008-09-30 10:01:28 EDT
Fast forward sounds good to me :). Source0 in the 1.2.2 spec (the last one that I looked at) did have m/i/mirrormanager in it.
Comment 18 manuel wolfshant 2008-09-30 10:18:30 EDT
ref #17: indeed, I missed a %{name} in the link

Package Review

 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM: empty
binary RPMs:empty
 [x] Package is not relocatable.
 [x] Buildroot is correct (%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX))
 [x] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:MIT and GPLv2
 [x] If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package match the upstream source, as provided in the spec URL.
     SHA1SUM of package: b584c6e192bea7b9a1d1e7eab0d9ac61d6e19a24  /tmp/mirrormanager-1.2.3.tar.bz2
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present (dependency is not needed)
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: devel/x86_64
 [?] Package should compile and build into binary rpms on all supported architectures.
     Tested on:
 [x] Package functions as described.
note: only the client was tested
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.

As far as I am I concerned, this packages seems OK so I am glad to say:
*** APPROVED ***
Comment 19 Matt Domsch 2008-09-30 10:29:55 EDT
Sweet.  Jon, I would sponsor you, but as I rewrote the whole spec from scratch here myself, I haven't reviewed any of your work. :-)

Jon, do you want to maintain this package in Fedora, or should I?

Comment 20 Jon Stanley 2008-09-30 14:10:36 EDT
No worries on the sponsorship, I've been sponsored for awhile now for other packages I've done :)

New Package CVS Request
Package Name: mirrormanager
Short Description: A mirror management system
Owners: mdomsch jstanley
Branches: EL-5 F-9
Comment 21 Kevin Fenzi 2008-10-01 15:00:05 EDT
cvs done.
Comment 22 Paul Howarth 2008-10-02 11:38:11 EDT
What's the point of "%define debug_package %{nil}" in this package?

noarch packages don't generate debuginfo by default...
Comment 23 Fedora Update System 2008-10-14 15:41:29 EDT
mirrormanager-1.2.6-1.fc9 has been submitted as an update for Fedora 9.
Comment 24 Fedora Update System 2008-10-20 16:26:12 EDT
mirrormanager-1.2.6-1.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update mirrormanager'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-8911
Comment 25 Fedora Update System 2008-11-05 23:03:26 EST
mirrormanager-1.2.6-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.