Bug 178169 - Review Request: jigdo
Review Request: jigdo
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
David Lawrence
:
Depends On: 178310
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-18 02:29 EST by Ian Burrell
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-03-22 14:28:35 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 Ian Burrell 2006-01-18 02:29:32 EST
Spec Name or Url: http://znark.com/fedora/extras/jigdo.spec
SRPM Name or Url: http://znark.com/fedora/extras/jigdo-0.7.2-1.src.rpm

Description: Jigsaw Download, or short jigdo, is a tool designed to ease the
distribution of very large files over the internet, for example CD or
DVD images.  Its aim is to make downloading the images as easy for
users as a click on a direct download link in a browser, while
avoiding all the problems that server administrators have with hosting
such large files.  It accomplishes this by using the separate pieces
of any big file (such as the files contained within a CD/DVD image) to
create a special "template" file which makes reassembly of the big
file very easy for users who only have the pieces.

This is my first package and I need a sponsor.
Comment 1 John Mahowald 2006-01-29 09:46:17 EST
Not a review, but you've got some duplicate BuildRequires. According to
fedora-qa script:  

Duplicate BuildRequires: zlib-devel (by openssl-devel), openssl-devel (by
curl-devel), atk-devel (by gtk2-devel), glib2-devel (by gtk2-devel), pango-devel
(by gtk2-devel), XFree86-devel (by gtk2-devel), freetype-devel (by pango-devel)
Comment 2 Kevin Fenzi 2006-01-29 15:17:44 EST
Here's a review:

MUST items:

OK      Package name.
OK      Spec file name.
OK      License good (GPL).
OK      License in spec matches.
OK      License is included (GPL COPYING)
OK      Spec english/readable
OK      Sources match upstream (md5: 031756ff6c7084a139dc9550a27f6906)
OK      No packages in BR that are in the exceptions list.
OK      Correctly uses find_lang
OK      No dynamic libs, so no ldconfig needed.
OK      Builds on fc4.
OK      No rpmlint output (on fc4 build). Good!
OK      Package owns all it's created directories.
OK      No duplicate files in files section.
OK      Permissions look good.
OK      Clean section looks good.
OK      desktop file looks good and good install.
OK      Runs fine on my fc4 machine.

ISSUES:

1. Doesn't build in current devel/mock. It has two unavailable BuildRequires:
No Package Found for w3c-libwww-devel >= 0:5.3.2
No Package Found for XFree86-devel
The w3c-libwww package no longer seems available in core in devel/fc5.
XFree86-devel has been replaced with modular xorg packages.

You may need to create and import a w3c-libwww package before this one if that
support is needed by this package. Can you see if that support is optional,
and/or some new package now provides that support for fc5?

Also, if you could review/comment on some other in review packages to show your
understanding of the package guidelines, that would assist in deciding to
sponsor you. ;) You can find a list in bugzilla at:
https://bugzilla.redhat.com/bugzilla/showdependencytree.cgi?id=FE-REVIEW&hide_resolved=1
Comment 3 Ville Skyttä 2006-01-29 15:22:24 EST
w3c-libwww is bug 178310
Comment 4 Ian Burrell 2006-01-29 19:13:15 EST
I modified the spec to remove the duplicate BuildRequires.  Removing
XFree86-devel means the same spec builds on devel after the w3c-libwwww-devel in
178310 is installed.  I added a patch to build on gcc 4.1.

Spec: http://znark.com/fedora/extras/jigdo.spec
SRPM: http://znark.com/fedora/extras/jigdo-0.7.2-2.src.rpm
Comment 5 Peter Gordon 2006-01-30 20:46:55 EST
The specfile shows that it owns everything in /usr/share/applications
(%{_datadir}/applications/*), as well as all of the standard binaries
(%{_bindir}/*), which is probably very bad. ;-)

Also, your %files section contains:
 %dir %{_datadir}/%{name}
 %{_datadir}/%{name}/*

Wouldn't simply owning the directory be simpler?
 %{_datadir}/%{name}

My two cents.
Comment 6 Kevin Fenzi 2006-01-30 23:03:13 EST
>The specfile shows that it owns everything in /usr/share/applications
>(%{_datadir}/applications/*), as well as all of the standard binaries
>(%{_bindir}/*), which is probably very bad. ;-)

Those globs are expanded at build time under the $RPM_BUILD_ROOT, so they only
match files that are installed in the %install step of the rpm building. :)
In this case they expand to: 

/usr/share/applications/fedora-jigdo.desktop
/usr/bin/jigdo
/usr/bin/jigdo-file
/usr/bin/jigdo-lite

Which is fine. 
Some folks like using * in files to match a bunch of files, while others prefer 
to list each file out. It's just a matter of taste as long as there are no
duplicate files listed or files/dirs that are owned by another package already. 
Comment 7 Ian Burrell 2006-01-31 00:03:01 EST
I thought just listing the directory would work.  And it did work for building
on FC4.  But when I built the package on rawhide, it complained about the files
in usr/shared/jigdo not being included.  So I put in the glob.

Comment 8 Peter Gordon 2006-01-31 01:22:42 EST
(In reply to comment 6) Ack. I had not realized that. Thanks, Kevin!
Comment 9 Kevin Fenzi 2006-02-12 14:18:22 EST
Hey Ian. 

Can you build against the latest w3c-libwww from 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=178310
and confirm that the package still works as expected?

Comment 10 Kevin Fenzi 2006-03-05 15:12:05 EST
w3c-libwww is now in extras.

Can you confirm that this package functions as expected compiled against that
package?

In the gcc41 patch you move the outOfMemory function around. Is that really
needed? I don't see any changes made to the function, just moving it. 

Comment 11 Ian Burrell 2006-03-08 23:51:50 EST
Sorry about taking so long to respond. I just discovered that jigdo
doesn't use w3c-libwww. I hadn't noticed until that the binary package
didn't depend on w3c-libwww package. It looks like upstream replaced
using libwww with libcurl in version 0.7.2.  I just rebuilt it without
the BuildRequires on w3c-libwww-devel and the result works. I posted
the new spec and srpm.

http://znark.com/fedora/extras/jigdo.spec
http://znark.com/fedora/extras/jigdo-0.7.2-1.src.rpm

The change is actually moving the cmdOptions method and the enum out
of the local namespace block while keeping outOfMemory inside the
namespace. GCC 4.1 seems to have stricter rules about namespaces.
Comment 12 Kevin Fenzi 2006-03-11 18:46:38 EST
Sorry for the delay in getting back to you on this review. 

This version looks good. The removal of the w3c-libwww-devel BR looks good. 
The namespace move makes sense. GCC 4.1 seems stricter in general on g++. 

Everything looks good. This package is APPROVED. 

I would be happy to sponsor you. 
Please continue the process from the "Get a Fedora Account" section at: 
http://fedoraproject.org/wiki/Extras/Contributors

If you have any questions at all, please don't hesitate to drop me an email. 
Comment 13 Ian Burrell 2006-03-22 14:28:35 EST
Built for extras devel.

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