Bug 238366

Notice to reviewers, reviewing this package will make much more sense, when also
reviewing / looking at the xu4 review, which uses this.

The xu4 review is bug 238367

I'll look into doing this review soon (I can't right now), but an inital note:

Description: 
"[...]
Notice that autodownloader itself is a piece of software / tool that is fully
free software, written in Python and licensed under the GNU GPL."

I'd nearly question if this should be removed?

Rational:
1. Fedora only accepts 'free' software licenses
2. It does not appear to be a program directly interfaced with by users (so why
would a user care that it's written in python?)
3. If you want to stress the fact that it's python then way not mention it as
"Autodownloader is a python based tool which can be used as [...]"

I'd also note that the other two paragraphs seem to be in the wrong order ;)

The 3rd paragraph is there to make it clear / emphasize that although
autodownloader is mean't to download files which we cannot distribute, that
autodownloader itself is 100% OSS.

The order of the 1st and 2nd paragraph is like this, because I think the proper
order to describe the use is first describing why and then how. The how only
makes little sense without first knowing the why.

Okay, happy to review.

(In reply to comment #3)
> The 3rd paragraph is there to make it clear / emphasize that although
> autodownloader is mean't to download files which we cannot distribute, that
> autodownloader itself is 100% OSS.
Autodownloader would have to be OSS to make it into Fedora, if you want to make 
it clear, I'd perfer something to the effect of:
"NOTE: Autodownloader while open source may download files which are not 
permitted to be distributed in Fedora"
> The order of the 1st and 2nd paragraph is like this, because I think the 
proper
> order to describe the use is first describing why and then how. The how only
> makes little sense without first knowing the why.
I think what it is, is more important than why you want to use it, why you want 
to use it, is what a developer or another packager would want to know, and they 
most likely have had a tip off from another maintainer that the package is 
useful for what they wish to do

The reason you have what it is first, is so users can decide if they really 
want to have it on their system without been bored to death first.

Also, I'm tempted to suggest that before inclusion, a database of downloaded 
files may need to be created, (also meaning that programs that have downloaded 
files, should ALWAYS require autodownloader) with a %preun to remove downloaded 
files (remembering that you wouldn't be able to remove autodownloader without 
removing dependencies that require the files first).  This prevents 
unneeded/unwanted files been left behind after uninstall.

Both are only suggestions as I don't think anything specific is in 
PackagingGuidelines regarding this yet, but I will check while testing.

(In reply to comment #4)
> Okay, happy to review.
> 
> (In reply to comment #3)
> > The 3rd paragraph is there to make it clear / emphasize that although
> > autodownloader is mean't to download files which we cannot distribute, that
> > autodownloader itself is 100% OSS.
> Autodownloader would have to be OSS to make it into Fedora, if you want to make 
> it clear, I'd perfer something to the effect of:
> "NOTE: Autodownloader while open source may download files which are not 
> permitted to be distributed in Fedora"

Okay, I'll change it into the above.

> > The order of the 1st and 2nd paragraph is like this, because I think the 
> proper
> > order to describe the use is first describing why and then how. The how only
> > makes little sense without first knowing the why.
> I think what it is, is more important than why you want to use it, why you want 
> to use it, is what a developer or another packager would want to know, and they 
> most likely have had a tip off from another maintainer that the package is 
> useful for what they wish to do
> 
> The reason you have what it is first, is so users can decide if they really 
> want to have it on their system without been bored to death first.
> 

Well the second paragraph is hard to understand for someone not into the matter
without first reading the first, it can be reworded to not depend on the first,
but that won't make things clearer IMHO, also I think its important to have the
first paragraph there, to make it clear how this is different from yum / a wget gui.

> Also, I'm tempted to suggest that before inclusion, a database of downloaded 
> files may need to be created, (also meaning that programs that have downloaded 
> files, should ALWAYS require autodownloader) with a %preun to remove downloaded 
> files (remembering that you wouldn't be able to remove autodownloader without 
> removing dependencies that require the files first).  This prevents 
> unneeded/unwanted files been left behind after uninstall.
> 

Interesting point, but autodownloader is not suid anything, and thus cannnot
write to such a global log file. It was designed to run as a normal user and
download files to dirs under the users $HOME. Yes this has a few downsides, but
from a security POV, this really is the best solution IMHO.

We might need to take another look at this (adding a suid helper written in C)
which can install files under /usr if autodownloader becomes popular and is used
to download big(ger) files.

(In reply to comment #5)
> Interesting point, but autodownloader is not suid anything, and thus cannnot
> write to such a global log file. It was designed to run as a normal user and
> download files to dirs under the users $HOME. Yes this has a few downsides, but
> from a security POV, this really is the best solution IMHO.
> 
> We might need to take another look at this (adding a suid helper written in C)
> which can install files under /usr if autodownloader becomes popular and is used
> to download big(ger) files.

Note that the "oddjob" package can help with this, without needing to set things
suid or some other mechanism...  It provides a simple, modular dbus-enabled
mechanism for non-privlidged processes to invoke predefined privlidged
operations.  I'd definitely take a look at this first.

(In reply to comment #5)
> (In reply to comment #4)
> > Also, I'm tempted to suggest that before inclusion, a database of downloaded 
> > files may need to be created, (also meaning that programs that have downloaded 
> > files, should ALWAYS require autodownloader) with a %preun to remove downloaded 
> > files (remembering that you wouldn't be able to remove autodownloader without 
> > removing dependencies that require the files first).  This prevents 
> > unneeded/unwanted files been left behind after uninstall.
> > 
> 
> Interesting point, but autodownloader is not suid anything, and thus cannnot
> write to such a global log file. It was designed to run as a normal user and
> download files to dirs under the users $HOME. Yes this has a few downsides, but
> from a security POV, this really is the best solution IMHO.
Okay in that case, it's like firefox and it's profile directories, they exist in
/home so they are the users responsibility to maintain.  It also means that a
database of downloaded files to /home directories are a 'bad thing' as home
directory names have susceptible to change.
> 
> We might need to take another look at this (adding a suid helper written in C)
> which can install files under /usr if autodownloader becomes popular and is used
> to download big(ger) files.
Agreed, files in /usr need to have something taking responsibility for them,
normally the package manager, but a %preun for something like this would be just
as fine.

I'm happy to continue with the review.

Package name:             OK (autodownloader)
License:                  OK (GPL)
Spec Legible:             OK (en_US)
md5sum matches:           OK (5ec9b35266be518b8384a7fea581680c)
rpmlint clean:            OK
Builds correctly:         OK (noarch)
Spec has %clean:          OK
Macro use consistant:     OK
Contains code/content:    OK
-doc subpackage:          NA
-devel subpackage:        NA
-static subpackage:       NA
pkgconfig depend:         NA
Contains %doc:            OK (COPYING ChangeLog GladeWindow-license.txt
README.txt TODO example.autodlrc)
Library suffix:           NA
No .la files:             NA
Use desktop-file-install: NA
No duplicate ownerships:  OK
rm -rf %{buildroot}:      OK
RPM uses valid UTF-8:     OK
%defattr is set:          OK
No duplicate %files:      OK
Not relocatable:          OK
Calls ldconfig:           NA
Supports Locales:         NA
BR's are correct:         OK

Other notes:
* Builds in mock (obviously)
* I have not been able to test but it appears to be okay
* As downloaded files are written to home, it's generally not rpm's
responsibility to cleanup (but the users)
* When downloads start going to /usr then autodownloader should provide some way
of deleting on uninstall.

APPROVED

(p.s. Any chance you could take a look at ocamlSDL (235804)?  I've still got to
get camlimages & windowlab back in shape but that one is ready)

(In reply to comment #8)
> APPROVED
> 

Thanks.

> (p.s. Any chance you could take a look at ocamlSDL (235804)?  I've still got to
> get camlimages & windowlab back in shape but that one is ready)

I'll try to make some time for this tomorrow.

New Package CVS Request
=======================
Package Name:      autodownloader
Short Description: GUI-tool to automate the download of certain files
Owners:            j.w.r.degoede
Branches:          FC-6 devel
InitialCC:         <empty>

Imported and build, closing