This service will be undergoing maintenance at 03:30 UTC, 2016-05-27. It is expected to last about 2 hours
Bug 498218 - Review Request: picturetile - Tiles a bunch of images into one large "photo wall"
Review Request: picturetile - Tiles a bunch of images into one large "photo w...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On: 498222
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-29 09:55 EDT by Edwin ten Brink
Modified: 2009-11-20 13:19 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-11-20 13:19:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
oget.fedora: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Edwin ten Brink 2009-04-29 09:55:19 EDT
Spec URL: http://www.tenbrink-bekkers.nl/fedora/picturetile.spec
SRPM URL: http://www.tenbrink-bekkers.nl/fedora/picturetile-20050314-1.fc10.src.rpm

Introduction:
Users need this perl script in order to use the Create Photowall option (provided by extension PictureTile 0.5.0.0 in F-Spot). The script itself is not packaged anywhere in Fedora until now. When user click Create Photowall in F-Spot, only the message "picturetile.pl not found" appears, which is not very user-friendly.

Unfortunately, F-Spot has a problem with its extensions at the moment, therefore live testing of the interaction with F-Spot is at the moment impossible. I already filed a bug upstream, and add I will add a tracker bug here in Bugzilla as well.

Since it is not required for F-Spot to have this package or vice versa, I deliberately left out these dependencies.

This is my second package under review, but I have no sponsor up to now. My first package is under review under bug 497525.


Description of the script:
Takes a bunch of images and tiles them into one large image.
Not so much a collage, as a "photo wall."

The images are selected randomly from the given directory, and packed
together as tightly as possible. Images that are very small are
assumed to be thumbnails and are ignored. No image will be used more
than once, and the images need not be of the same size or aspect ratio.

This script can be used stand-alone from the command-line, or from the
F-Spot plugin "create photowall" (PictureTile).
Comment 1 Edwin ten Brink 2009-07-21 16:02:05 EDT
(In reply to comment #0)
> This is my second package under review, but I have no sponsor up to now.
In the meantime, my first package has been approved. I'm now sponsored by Christoph Wickert.


Additionally, due to a server restructuring, the URL's have changed (files are still identical):
Spec URL: http://fedora.tenbrink-bekkers.nl/picturetile/picturetile.spec
SRPM URL: http://fedora.tenbrink-bekkers.nl/picturetile/picturetile-20050314-1.fc10.src.rpm
Comment 2 Thomas Spura 2009-10-15 09:24:27 EDT
Just a few comments, not a review:

This bug should block your tracker bug and not depend on it.
The tracker bug can only be closed, if all bugs 'blocking' the tracker bugs are closed. If they depend all on the tracker, they all can only be closed, if the tracker gets closed first.

Are you sure about the version generation?
See [1]. So it should have the name picturetile-0-1.20050314.fc10.src.rpm, isn't it?
I'm not sure about this, too, but simply using the date as a version number 'looks bad'.^^

e.g. from the guidelines:
kismet-0-0.1.20040110svn (this is a pre-release, svn checkout of kismet)
kismet-0-0.2.20040110svn (this is a bugfix to the previous package)
kismet-0-0.3.20040204svn (this is a new svn checkout, note the increment of %{X})
kismet-1.0-1 (this is the formal release of kismet 1.0)

Of course, there will maybe never been any real version, but how about using always version 0 and append the date after the release number?


[1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease
Comment 3 Christoph Wickert 2009-10-19 16:19:39 EDT
(In reply to comment #2)
> Just a few comments, not a review:
> 
> This bug should block your tracker bug and not depend on it.

I think what Edwin wants to point out it that first the bug with fspot should be fixed before he will maintain this package. Edwin, am I correct?

> Are you sure about the version generation?
> See [1]. So it should have the name picturetile-0-1.20050314.fc10.src.rpm,
> isn't it?
> I'm not sure about this, too, but simply using the date as a version number
> 'looks bad'.^^
...
> [1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease  

This applies to numeric releases with characters such as "RC", "BETA" and so on. The date *is* numeric. As time moves forward *hint* *hint* the version will always increase. So using the date is ok.

However prefixing it with a 0. is not a bad idea. Otherwise we might get into trouble if upstream one day decides to switch to another versioning.
Comment 4 Edwin ten Brink 2009-10-21 13:15:42 EDT
First of all, Thomas, Christoph: thanks for your comments. I was unfortunately not available the last few days to get back to you earlier.

(In reply to comment #3)
> (In reply to comment #2)
> > This bug should block your tracker bug and not depend on it.
> 
> I think what Edwin wants to point out it that first the bug with fspot should
> be fixed before he will maintain this package. Edwin, am I correct?

Christoph is entirely correct. Since it will only be possible to effectively test the package functionality on the command-line only, and this package is meant to extend the functionality of an add-on of fspot, I believe it will only make sense if the fspot add-on actually functions.

Recently, fspot has released a new version, as well as the add-on. For some reason I have not found out yet, it does no longer crash fspot, but does not seem to generate the desired output. I'm still working on this, therefore I have not yet closed bug 498222 and the dependency.

> > Are you sure about the version generation?
> This applies to numeric releases with characters such as "RC", "BETA" and so
> on. The date *is* numeric. As time moves forward *hint* *hint* the version will
> always increase. So using the date is ok.
> 
> However prefixing it with a 0. is not a bad idea. Otherwise we might get into
> trouble if upstream one day decides to switch to another versioning.  

I agree, an oversight on my part. I will incorporate it in my next submission.


I'll get back to this as soon as the combination fspot / addon / picturetile seems to be functioning.
Comment 5 Edwin ten Brink 2009-10-29 14:28:07 EDT
The add-in PictureTile version 0.6.0.1 for f-spot-0.6.1.2-3.fc11.i586 works with the package under review (picturetile-0.20050314-2).

I incorporated the version-comment, so the version is now prefixed with "0." The package is otherwise unchanged.

Uploaded files:
Spec URL: http://fedora.tenbrink-bekkers.nl/picturetile/picturetile.spec
SRPM URL:
http://fedora.tenbrink-bekkers.nl/picturetile/picturetile-0.20050314-2.fc11.src.rpm
Comment 6 Orcan Ogetbil 2009-11-07 02:54:34 EST
Thanks for packaging this nice small handy script. I made the full review on this. The *'s are the only blockers. The rest is remarks and questions:

- rpmlint is silent

* The license is MIT
   https://fedoraproject.org/wiki/Licensing/MIT
old style. One of the oldest software licenses out there.

! 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.

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

! 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..

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

? 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.

* 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?
Comment 7 Orcan Ogetbil 2009-11-07 02:57:17 EST
! ah. also it would be nice to have the source file downloaded with
   wget -N http://...
so we have the correct timestamp.
Comment 8 Edwin ten Brink 2009-11-08 10:23:49 EST
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
Comment 9 Orcan Ogetbil 2009-11-08 19:51:31 EST
(In reply to comment #8)
> Orcan, thanks for your review. My responses marked with + have been
> incorporated, -, not, and ? are pending your response (not yet incorporated).
> 

You are welcome. I see that almost everything is resolved. Yet, there is a misunderstanding that was probably my fault.

> (In reply to comment #6)
> > 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?
> 

I am really not sure about this. The guideline might be interpreted either way. Could you ask this in the fedora-packaging list?

> > ? 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.
> 

Your choice :) I am fine either way.

> 
> > ? 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 say, drop it. One RPM package providing two different versions of the same thing might confuse people or depsolvers.

> > ? 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

Oh I meant something like
Version: 0
Release: 1.20050314

I prefer that we leave the Version to upstream vendors and use our conventions only in Release. But as you said, it is unlikely that this will cause a problem in the future, thus I leave this up to you.


> 
> > ! 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)?
> 

Here is the misunderstanding. No, you can't use wget in a spec file. koji has no outside access while building a package. Besides this may be considered as a security risk.

What I meant was roughly:
- Go to your SOURCES directory and use wget -N there, so that your original source0 file has the correct timestamp.
- Then go to your SPECS directory and and use rpmbuild -ba (Don't use wget inside the SPEC file)
- Now you can check your newly built rpm via
   rpm -qplv picturetile-<blah blah>.rpm 
and you will see that your /usr/bin/picturetile.pl file does have the correct timestamp.

Sorry that I caused a confusion.
Comment 10 Orcan Ogetbil 2009-11-08 19:55:00 EST
(In reply to comment #9)
> (In reply to comment #8)
> > 
> > > ? 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 say, drop it. One RPM package providing two different versions of the
> same thing might confuse people or depsolvers.
> 

I realized that what I said is not true, since the package name is not "picturetile.pl". So having that Provides is harmless. I don't know if it brings us anything though.

I leave it up to you.
Comment 11 Edwin ten Brink 2009-11-12 13:38:33 EST
(In reply to comment #9)
> > (In reply to comment #6)
> > > 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?
> > 
> 
> I am really not sure about this. The guideline might be interpreted either way.
> Could you ask this in the fedora-packaging list?

When writing my mail to fedora-packaging, I re-read the actual line several times. It seems I've over-interpreted it at the time of packaging:
"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." The crux here is the *own file*. It does not include the text in it's own file COPYING. It is therefore incorrect to generate the file (because if the "If (and only if)").

Therefore, I'll strip out the generation of these files, as per your original suggestion.
Comment 12 Orcan Ogetbil 2009-11-12 13:42:46 EST
This only applies to the COPYING filse though. You can keep the other one.
Comment 13 Edwin ten Brink 2009-11-12 13:50:26 EST
(In reply to comment #12)
> This only applies to the COPYING filse though. You can keep the other one.  

As a gesture to the user, that would probably not be harmful. Agreed.
Comment 14 Edwin ten Brink 2009-11-12 13:50:41 EST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > 
> > > > ? 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 say, drop it. One RPM package providing two different versions of the
> > same thing might confuse people or depsolvers.
> > 
> 
> I realized that what I said is not true, since the package name is not
> "picturetile.pl". So having that Provides is harmless. I don't know if it
> brings us anything though.
> 
> I leave it up to you.  

I'll leave it in, in order to make it easier for users to find this package.
The typical use-case would be following:

- user installs f-spot
- user downloads additional extension PictureTile via fspot's Add-In manager
- user runs Tools -> Create Photowall
- user receives error message "picturetile.pl not found"
- user tries to find "picturetile.pl", not "picturetile"

Including the Provides makes it easier for rpmfind and the likes to serve this
use-case.
Comment 15 Edwin ten Brink 2009-11-12 14:09:23 EST
Ok, I've incorporated all your comments. I'd like to go for a final review. New files can be found at:

Spec URL: http://fedora.tenbrink-bekkers.nl/picturetile/picturetile.spec
SRPM URL:
http://fedora.tenbrink-bekkers.nl/picturetile/picturetile-0-4.20050314.fc11.src.rpm
Comment 16 Orcan Ogetbil 2009-11-12 15:34:31 EST
It seems all good now. Thanks!

----------------------------------------------
This package (picturetile) is APPROVED by oget
----------------------------------------------
Comment 17 Edwin ten Brink 2009-11-14 09:11:50 EST
New Package CVS Request
=======================
Package Name: picturetile
Short Description: Tiles a bunch of images into one large "photo wall"
Owners: edwintb
Branches: F-12
InitialCC:
Comment 18 Jason Tibbitts 2009-11-14 11:42:22 EST
CVS done.
Comment 19 Fedora Update System 2009-11-17 15:10:27 EST
picturetile-0-4.20050314.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/picturetile-0-4.20050314.fc12
Comment 20 Fedora Update System 2009-11-20 00:30:09 EST
picturetile-0-4.20050314.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

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