[Bug 498218] Review Request: picturetile - Tiles a bunch of images into one large "photo wall"

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=498218





--- Comment #8 from Edwin ten Brink <fedora at tenbrink-bekkers.nl>  2009-11-08 10:23:49 EDT ---
Orcan, thanks for your review. My responses marked with + have been
incorporated, -, not, and ? are pending your response (not yet incorporated).

(In reply to comment #6)
> * The license is MIT
>    https://fedoraproject.org/wiki/Licensing/MIT
> old style. One of the oldest software licenses out there.

+ Thanks, missed that one.

> ! Guidelines say: "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 must be included in %doc." in
>    http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text"
> But this package does not even have a tarball. Is there any other examples of
> Fedora packages that just cut the license text from the soure file? If not, I
> would say it is best to obey the guideline.

? I'm unaware of any such practices (or cases where it might be applicable for
that matter). I preferred to have the README out of the script in a %doc file,
and figured when I did so, I could just as well generate a COPYING. It seems
that I either should do it this way, or drop all generated %doc files entirely.
Which option do you recommend?

> ? Shall we rename the executable to just picturetile? That is the upstream
> project name.

- I agree that an extension of .pl is not necessary, but fspot looks for an
executable picturetile.pl in $PATH, so unless we want a symlink from
picturetile.pl to picturetile, I suggest to drop this comment.

> ! It would be better to install the executable with "install -pm 755 ..." and
> not use %attr(0755,root,root) to comply with general Fedora conventions..

+ Agreed.

> ? I am a bit reluctant about the provides:
>    Provides:       picturetile.pl = 20050314
> Do we really need this?

? No, not really. I just added it in case someone decides to make a Requires in
fspot. I can drop this without any adverse effects, should I?

> ? I would go with the suggestion in comment #2 for version and release numbers.
> Even if upstream decides to make a 0.1 release we'll be in trouble.

+ Ok. Although the version number is now fool proof, I doubt that it will be
necessary.

> * The first 5 lines of the %build should probably go into %prep. Also could you
> use the "%setup -qcT" macro in %prep so that the package gets built in its own
> directory?  

+ Agreed.

> ! ah. also it would be nice to have the source file downloaded with
>    wget -N http://...
> so we have the correct timestamp. 

+ Agreed. Though I unfortunately need some fussing around with timestamping
since the file has to be converted to UTF-8.
? I'm not quite happy with the hardcoded URL in two places though. Isn't there
a way to refer to Source0 directly (without rpm rewriting it to the local
SOURCES directory)?


Changes so far have been included in the following files:
Spec URL: http://fedora.tenbrink-bekkers.nl/picturetile/picturetile.spec
SRPM URL:
http://fedora.tenbrink-bekkers.nl/picturetile/picturetile-0.1.20050314-3.fc11.src.rpm

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.