Bug 178330 - Review Request: Azureus - a BitTorrent Client
Review Request: Azureus - a BitTorrent Client
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Chabot
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-19 10:07 EST by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-24 10:49:27 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 Anthony Green 2006-01-19 10:07:11 EST
Spec Name or Url: http://people.redhat.com/green/FE/devel/azureus.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-8.src.rpm
Description: 
Azureus implements the BitTorrent protocol using java language and
comes bundled with many invaluable features for both beginners and
advanced users.
Comment 1 Anthony Green 2006-01-19 10:57:55 EST
Andrew Overholt made some suggestions on IRC.  New files are here:

Spec Name or Url: http://people.redhat.com/green/FE/devel/azureus.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-9.src.rpm
Comment 2 Chris Chabot 2006-01-19 11:04:59 EST
I'll pick it up, i'm almost getting used to this java packaging thing :-)
Changing to FE-REVIEW.

It builds cleanly on fedora-devel-i386, and functionally it seems well in order too.

However during compile i got this message:
/usr/bin/build-classpath: error: could not find xml-commons-apis Java extension
for this JVM
/usr/bin/build-classpath: error: All specified jars were not found

Is this a missing BuildRequires? Or an ignorable error? Doing "yum install
xml-commons-api" seemed to make it happy again, so it appears to be a missing
BuildRequire, if so please add it

Spacing in the header of the spec file is a little confused, seems your not
using spaces (especially for the patches) but tabs, in different editors this
then looks messy; Could you please re-indent it with spaces?

It might be better to: install -m644 %{SOURCE2} and 3 instead of copy, this way
your always sure the permissions are correct, and same with -m775 for the script

**** edit *** I see that you just corrected this in release 9 :-)

If you could please look at the above mentioned issues (spacing in header and
probable missing build requires) i'll post the formal reviewlist
Comment 3 Chris Chabot 2006-01-19 11:09:38 EST
Ps i see your not using install yet for:
mkdir -p $RPM_BUILD_ROOT%{_datadir}/applications
cp %{SOURCE2} $RPM_BUILD_ROOT%{_datadir}/applications

mkdir -p $RPM_BUILD_ROOT%{_datadir}/application-registry
cp %{SOURCE3} $RPM_BUILD_ROOT%{_datadir}/application-registry

Could you please change this to:
mkdir -p $RPM_BUILD_ROOT%{_datadir}/applications
install -m644 %{SOURCE2} $RPM_BUILD_ROOT%{_datadir}/applications

mkdir -p $RPM_BUILD_ROOT%{_datadir}/application-registry
install -m644 %{SOURCE3} $RPM_BUILD_ROOT%{_datadir}/application-registry

Comment 4 Chris Chabot 2006-01-19 11:17:40 EST
File permissions of the %doc files come out all wrong too (775, should be 644).
You could do a simple chmod 644 below the dos2unix lines to fix this up.

Also fedora extra's guidelines say md5sum of the source package should match the
one from upstream, but upstream i can't seem to find the _nocrypto source, could
you please provide a link to it?

If its custom patched, the normal way to do this is thru a patch, and including
the original upstream source and patching it on the fly, only exceptions to this
i've seen so far is when patents prevent RH from distributing the original source

If neither those is the case (not on the upstream download server & can't patch
it or script it and not for legal reasons) please at the very least add a
comment to the spec file why not, and how people can reproduce this based on
future upstream release versions
Comment 5 Ignacio Vazquez-Abrams 2006-01-19 12:05:10 EST
(In reply to comment #4)
> Also fedora extra's guidelines say md5sum of the source package should match the
> one from upstream, but upstream i can't seem to find the _nocrypto source, could
> you please provide a link to it?
> 
> If its custom patched, the normal way to do this is thru a patch, and including
> the original upstream source and patching it on the fly, only exceptions to this
> i've seen so far is when patents prevent RH from distributing the original source

No, stuff like crypto, MP3 support, etc. must be removed from the tarball
entirely. See bmp for an example.
Comment 6 Anthony Green 2006-01-19 12:09:24 EST
I've made all your suggested changes, and more:

Spec Name or Url: http://people.redhat.com/green/FE/devel/azureus.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-8.src.rpm

Regarding the source ball... I wanted to remove export controlled crypto code
from the azureus distributions.  We're trying to make it use existing crypto
package (GNU Crypto and Jessie, for instance).  If testing tells us that we
absolutely need bouncycastle, then that should be a separate package.  I put a
comment in the spec file explaining what directory I deleted from the source
distribution.


Comment 7 Anthony Green 2006-01-19 12:11:30 EST
(In reply to comment #6)
> I've made all your suggested changes, and more:
> 
> Spec Name or Url: http://people.redhat.com/green/FE/devel/azureus.spec
> SRPM Name or Url:
http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-8.src.rpm

Oops - that was supposed to read:

http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-10.src.rpm
Comment 8 Chris Chabot 2006-01-19 12:15:29 EST
Taking a look at it now.

Reason for excluding crypto is definatly good enough (i mentioned that in my
original comment, patent & export restrictions are solid reasons :-)

I just noticed btw you forgot:
Requires(post):   desktop-file-utils                                           
                                        
Requires(postun): desktop-file-utils

Which are required for 'update-desktop-database'.

With the link you probably meant:
http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-10.src.rpm 
But i found it ok :-)
Comment 9 Chris Chabot 2006-01-19 12:19:08 EST
Ps the desktop file is supposed to be installed thru:

desktop-file-install --vendor fedora                            \
        --dir ${RPM_BUILD_ROOT}%{_datadir}/applications         \
        --add-category X-Fedora                                 \
        %{SOURCE2}

this adds the correct vendor and category tags

More details on this here:
http://fedoraproject.org/wiki/PackagingGuidelines#desktop

Then you also need:
BuildRequires: desktop-file-utils 

Comment 10 Chris Chabot 2006-01-19 12:29:03 EST
Formal review list so far:

MUST review items:
- Builds cleanly on FC5 devel.
- Source included doesn't match upsteam source, but for a good reason :-)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence is fedora extra's compatible & is included
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed
- Proper directory-ownerships
- **ERROR: No complete desktop file & install handling yet

Should items:
- Includes upstream licence(s) file
- No insane scriplets
- No unnescesarry requires
- Mock build cleanly

Almost there!
Comment 11 Chris Chabot 2006-01-19 12:34:49 EST
rpmlint's only warning is about libswt3-gtk, but since so auto-depedencies don't
pick this up rpmlint is wrong in this case.

Small side note: Why is it that it leaves 'plugins' dirs in any directory you
start this up in? Shouldn't it use ~/.Azureus/plugins/ for that? (which also exists)
Comment 12 Anthony Green 2006-01-19 12:44:42 EST
(In reply to comment #11)
> rpmlint's only warning is about libswt3-gtk, but since so auto-depedencies don't
> pick this up rpmlint is wrong in this case.

Ok, try..
http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-11.src.rpm
 
I think it has eveything you've suggested.

> Small side note: Why is it that it leaves 'plugins' dirs in any directory you
> start this up in? Shouldn't it use ~/.Azureus/plugins/ for that? (which also
exists)

I'm not sure, but I have an idea.  That would be a good issue to file in
bugzilla once this is accepted. :-)

Thanks for all your help!

AG


Comment 13 Jeffrey C. Ollie 2006-01-19 12:48:32 EST
According to the packaging guidelines, dos2unix shouldn't be used to convert
line endings.  The following should be used instead:

%{__sed} -i 's/\r//' filename
Comment 14 Chris Chabot 2006-01-19 12:59:31 EST
Woops Jeffrey is right, it reads:

E: foo-package wrong-script-end-of-line-encoding /path/to/somefile: This error
occurs because of DOS line breaks in a file. Fix it with sed in the %prep
section: %{__sed} -i 's/\r//' src/somefile -- DONT use dos2unix, that can cause
build fail on FC3.

Didn't know about the FC3 problem, could you please put that last fix in the
spec file?
Comment 15 Chris Chabot 2006-01-19 13:00:50 EST
(In reply to comment #9)
> Then you also need:
> BuildRequires: desktop-file-utils 

I'm still missing that part :-)
Comment 16 Anthony Green 2006-01-19 13:17:29 EST
(In reply to comment #15)
> (In reply to comment #9)
> > Then you also need:
> > BuildRequires: desktop-file-utils 
> 
> I'm still missing that part :-)
> 

Ok, fixed.  And I'm using sed instead of dos2unix.

http://people.redhat.com/green/FE/devel/azureus.spec
http://people.redhat.com/green/FE/devel/azureus-2.3.0.6-12.src.rpm
Comment 17 Linus Walleij 2006-01-19 14:02:26 EST
Why is junit in Requires:?

In my book that's a unit test program and not used in any application
by end-users. However junit may have gained new powers that I'm not
aware of...
Comment 18 Chris Chabot 2006-01-19 14:21:11 EST
New reviewlist based on release 12:

MUST review items:
- Builds cleanly on FC5 devel.
- Source included doesn't match upsteam source, but for a good reason :-)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence is fedora extra's compatible & is included
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed
- Proper directory-ownerships
- Correct desktop file & install handling & categories

Should items:
- Includes upstream licence(s) file
- No insane scriplets
- No unnescesarry requires
- Mock build cleanly (fedora-devel-i386)

I've even tried to compile w/o junit, but can tell you that Azureus does depend
on it
(azureus-2.3.0.6/org/gudy/azureus2/ui/console/multiuser/TestUserManager.java),
the how and what and why i will leave to AG, i'm not skilled in java enough to
comment on this, but this isn't a packaging issue, more something for future
bugzilla reports (i'm sure quite a few of those will follow, but better to do
that in bugzilla/cvs). So:

FE-ACCEPTED

Comment 19 Anthony Green 2006-01-19 14:23:50 EST
Thanks!

BTW, I've removed the junit dependency by removing the one bit of test code that
didn't really belong in the runtime.  So I'll be checking in a version -13 later
today.

Thanks again!

AG
Comment 20 Joël Bourquard 2006-01-30 11:31:10 EST
Hi Anthony !

Nice work you're doing... If possible, I would like to make an Azureus gcj
source package based on your ideas, and contribute it to gentoo linux.

I hope I'm not too off-topic on this bug..

Usually, a gentoo package (ebuild) is specified in terms of compilation,
installation and dependencies, listed inside a bash script.

What method would you recommend to get the latest Azureus source built using gcj
? Is extensive patching needed ? What dependencies would I need to gcj-compile
too ? Btw, for maintenance and gentoo's java policy reasons, I would prefer to
use the original Azureus source and patch it on the fly, if possible..

Thanks in advance !

Joël
Comment 21 Anthony Green 2006-01-30 20:29:13 EST
(In reply to comment #20)
> Hi Anthony !
> 
> Nice work you're doing... If possible, I would like to make an Azureus gcj
> source package based on your ideas, and contribute it to gentoo linux.

Let's take this to private mail.

AG

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