Bug 1073978 - Review Request: photocollage - An image assembler with a Gtk GUI
Review Request: photocollage - An image assembler with a Gtk GUI
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kalev Lember
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-07 10:18 EST by Adrien Vergé
Modified: 2014-03-30 22:09 EDT (History)
3 users (show)

See Also:
Fixed In Version: photocollage-1.0.2-1.fc20
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-21 13:01:58 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kalevlember: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Adrien Vergé 2014-03-07 10:18:56 EST
Spec URL: http://adrienverge.fedorapeople.org/photocollage.spec
SRPM URL: http://adrienverge.fedorapeople.org/photocollage-1.0-1.src.rpm
Description:
PhotoCollage is a graphical tool to create photo collage posters.  It assembles
the input photographs you give it to generate a big poster.  Photos are
arranged to fill the whole poster, however you can influence the algorithm to
change the final layout.  You can also set a custom border between photos, and
save the generated image in the resolution you want.
PhotoCollage does more or less the same as many commercial websites do, but
for free and with open-source code.

The SRPM builds on Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=6609469

This is my first package, so I need a sponsor. Thank you!
Comment 1 Antonio Trande 2014-03-07 15:11:31 EST
Hi Adrien.

You don't need to define Name, Version and Release macros because they are already defined. 
Vendor tag must not be used.
SourceX tag is a complete link to the source archive.
Prefix tag is unnecessary.
Buildroot tag is used only for EPEL5 packages or for old Fedora releases (see http://fedoraproject.org/wiki/EPEL:Packaging#BuildRoot_tag and http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag).
As Buildroot, %clean section is unnecessary for recent Fedora releases (http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean) like so %defattr line (http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions).
Comment 2 Adrien Vergé 2014-03-07 18:56:12 EST
Hi Antonio,

Thank you for your remarks, I've changed all that needed to be corrected. The new spec and SRPM are now at:

http://adrienverge.fedorapeople.org/packages/photocollage.spec
http://adrienverge.fedorapeople.org/packages/photocollage-1.0-1.fc19.src.rpm
Comment 3 Christopher Meng 2014-03-08 00:41:42 EST
# - python3-pillow      for PIL.Image and PIL.ImageDraw
# - python3-gobject     for gi.repository
# - gettext             for gettext
# - copy, io, math, multiprocessing, os.path, random
#   and threading are built-in python3-libs

You can remove these.

-------------

What does this line stand for?

%{__install} -d %{buildroot}%{_datadir}/icons


-------------

update-mime-database %{_datadir}/mime &> /dev/null || :

Please read carefully:

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo

-------------

You can drop group tag.

-------------

You are the upstream of this package, please ship an appdata file for packages with desktop file:

http://blogs.gnome.org/hughsie/2013/08/19/appdata-proposal-a-k-a-how-to-make-your-application-appear-in-the-software-center/

http://people.freedesktop.org/~hughsient/appdata/
Comment 4 Adrien Vergé 2014-03-08 12:50:12 EST
Hi Christopher, thank you for the review.

I've created an AppData file and removed the unnecessary lines.

I thought the line
%{__install} -d %{buildroot}%{_datadir}/icons
was needed to install icons to /usr/share, it appears that I was wrong :)

The spec and SRPM have been updated.
Comment 5 Kalev Lember 2014-03-20 05:42:18 EDT
Taking for review.
Comment 6 Kalev Lember 2014-03-20 18:25:29 EDT
So, you are a new packager. Welcome to Fedora, Adrien! I hope you'll enjoy this.

As a new packager, you'll need to get sponsored into the packager group. This is a one time process and it's much easier to get other packages in once you've cleared this initial step and are part of the packager group.

All packagers are also automatically reviewers, so it's expected that everyone knows how to perform package reviews. It's common to ask for new people to show their understanding of the packaging guidelines by asking them to perform one package review. Could you choose a ticket of your liking from the list in http://fedoraproject.org/PackageReviewStatus/NEW.html and go through the package review checklist in https://fedoraproject.org/wiki/Packaging:ReviewGuidelines and post your findings in the ticket?


Regarding the package here, I've taken a quick look and it looks nice and clean. Good work!


One thing I've noticed is that the source files don't have license headers. It would be great if you could add them upstream as per https://www.gnu.org/licenses/old-licenses/gpl-2.0.html#SEC4

Fedora is quite strict with legal matters and things that concern licensing. In the licensing FAQ, there's a long section what to do when there's only a COPYING file and no license headers: https://fedoraproject.org/wiki/Licensing:FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F It's probably easier to just add the license headers and clear any confusion with that.


The spec file looks largely fine. I have some nitpick comments below:

> License:        GPLv2

Is it GPLv2 or GPLv2+ ? The rpm spec file and the PKG-INFO file seem to disagree. And no license headers in the .py files to double check this.


> %setup -q -n %{name}-%{version}

Remove the "-n %{name}-%{version}" part, that's the default.


> desktop-file-install --vendor="" \
>         --dir=%{buildroot}%{_datadir}/applications/ \
>         %{buildroot}%{_datadir}/applications/%{name}.desktop

It would be better to use desktop-file-validate here. desktop-file-install is mostly for if you need to edit the .desktop file (e.g. add or remove a category), or when you are including an external desktop file that doesn't come from upstream. None of this applies here.


Also, I see you've done a few changes to the upstream tarball without changing the version. It's better to bump the version each time you need to roll a new release. It can be very confusing for other people if a tarball with the same version gets silently replaced.

http://www.scrye.com/wordpress/nirik/2014/03/11/just-say-no-to-re-releasing-the-same-version-of-software/
Comment 7 Adrien Vergé 2014-03-20 20:09:19 EDT
Hi Kalev,

Thank you for reviewing!

It's good you point the license issue. Actually, the project is under GPLv2+. It's been updated and included in headers.

Also, PhotoCollage will now bump the version at every change. Current is now 1.0.1.

The new spec and SRPM:
http://adrienverge.fedorapeople.org/packages/photocollage.spec
http://adrienverge.fedorapeople.org/packages/photocollage-1.0.1-1.src.rpm

I have reviewed the latest submission in the list:
https://bugzilla.redhat.com/show_bug.cgi?id=1079064
and I'll do others soon.
Comment 8 Christopher Meng 2014-03-20 22:02:59 EDT
Additional help:

http://koji.fedoraproject.org/koji/taskinfo?taskID=6657552
Comment 9 Kalev Lember 2014-03-21 05:43:37 EDT
(In reply to Adrien Vergé from comment #7)
> I have reviewed the latest submission in the list:
> https://bugzilla.redhat.com/show_bug.cgi?id=1079064
> and I'll do others soon.

Excellent! Unfortunately, it's another FE-NEEDSPONSOR ticket and you won't be able to officially approve that package once you are sponsored. Oh well, I guess we could use more sponsors.


Anyway, regarding the comments in the other ticket:

> Replace $RPM_BUILD_ROOT with %{buildroot}

Either one is actually fine as per https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

> From what I read in your Makefile:
> LIBS = $(SDL_LDFLAGS) -lSDL_image -lSDL_mixer -lexpat -lSDL_ttf -lphysfs \
> -lboost_filesystem -lboost_system -lpng
> your package depends on some libraries. You need to declare them explicitely with Requires entries. Please refer to this section:

For C and C++, rpm includes a shared library dependency generator. It looks at what libraries the executable uses (DT_NEEDED entries), and adds rpm Requires automatically based on that.

In your case here, the package is a Python program and RPM does not have a dependency generator for this, so you need to specify the Requires manually. For C code like btbuilder, it's actually the other way around -- it's recommended to have rpm take care of dependencies and not add manual Requires.

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
Comment 10 Kalev Lember 2014-03-21 05:57:40 EDT
Fedora review photocollage-1.0.1-1.src.rpm 2014-03-21

$ rpmlint photocollage \
          photocollage-1.0.1-1.src.rpm
photocollage.noarch: W: no-manual-page-for-binary photocollage
photocollage.src: W: non-coherent-filename photocollage-1.0.1-1.src.rpm photocollage-1.0.1-1.fc20.src.rpm
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

+ OK
! needs attention

+ rpmlint warnings are harmless and can be ignored
+ The package is named according to Fedora packaging guidelines
+ The spec file name matches the base package name.
+ The package meets the Packaging Guidelines
+ The package is licensed with a Fedora approved license and meets the
  Licensing Guidelines.
+ The license field in the spec file matches the actual license
+ The package contains the license file (LICENSE)
+ Spec file is written in American English
+ Spec file is legible
+ Upstream sources match the sources in the srpm
  4334865175d8e12287155766930de57d  photocollage-1.0.1.tar.gz
  4334865175d8e12287155766930de57d  Download/photocollage-1.0.1.tar.gz
+ The package builds in koji
n/a ExcludeArch bugs filed
+ BuildRequires look sane
+ The spec file handles locales properly
n/a ldconfig in %post and %postun
+ Package does not bundle copies of system libraries
n/a Package isn't relocatable
+ Package owns all the directories it creates
+ No duplicate files in %files
+ Permissions are properly set
+ Consistent use of macros
+ The package must contain code or permissible content
n/a Large documentation files should go in -doc subpackage
+ Files marked %doc should not affect package
n/a Header files should be in -devel
n/a Static libraries should be in -static
n/a Library files that end in .so must go in a -devel package
n/a -devel must require the fully versioned base
+ Packages should not contain libtool .la files
+ Proper .desktop file handling
+ Doesn't own files or directories already owned by other packages
+ Filenames are valid UTF-8

Just a small nit with %changelog section. In Fedora, it's common to have an empty newline between two changelog entries, such as:

* Thu Mar 20 2014 Adrien Vergé <adrienverge@gmail.com> - 1.0.1-1
- Add license headers in source files

* Wed Mar  5 2014 Adrien Vergé <adrienverge@gmail.com> - 1.0-1
- initial build

Another thing I've noticed is that the package crashes about half the time after clicking on 'Preview poster' with the following spew on the console. No idea where this comes from, maybe something wrong down in the stack.

[xcb] Unknown sequence number while processing queue
[xcb] Most likely this is a multi-threaded client and XInitThreads has not been called
[xcb] Aborting, sorry about that.
python3: xcb_io.c:274: poll_for_event: Assertion `!xcb_xlib_threads_sequence_lost' failed.
Aborted


Anyway, the package looks good to me to go in!

APPROVED
Comment 11 Kalev Lember 2014-03-21 06:06:12 EDT
I've now sponsored you to the packager group. Welcome and use your new powers with care!

It might take up to an hour for the permissions to sync everywhere (e.g. bugzilla) before you can set the flags in the ticket to request git repo creation.

Feel free to send me emails or ask on IRC if you have any questions or need help with the processes. I am kalev on #fedora-devel on freenode and also on #fedora-desktop on irc.gnome.org.
Comment 12 Adrien Vergé 2014-03-21 10:41:30 EDT
Thank you for sponsoring and for your explanations! It's good that RPM auto-detects C lib dependencies. Managing such a thing for Python shouldn't be complicated, just some scripting and 'repoquery --whatprovides' I guess.

I've given the changelog some air to breathe.

Thanks for reporting the crash, I will try to reproduce and fix it.

Time for SCM request now! I won't put photocollage in EPEL branch though, since the latter lacks python3-devel.
Comment 13 Adrien Vergé 2014-03-21 10:42:46 EDT
New Package SCM Request
=======================
Package Name: photocollage
Short Description: An image assembler with a Gtk GUI
Owners: adrienverge
Branches: f19 f20
InitialCC:
Comment 14 Gwyn Ciesla 2014-03-21 12:51:54 EDT
Git done (by process-git-requests).
Comment 15 Adrien Vergé 2014-03-21 13:01:03 EDT
Thanks everyone. I'm closing this.
Comment 16 Fedora Update System 2014-03-21 15:12:15 EDT
photocollage-1.0.2-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/photocollage-1.0.2-1.fc19
Comment 17 Fedora Update System 2014-03-21 15:14:41 EDT
photocollage-1.0.2-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/photocollage-1.0.2-1.fc20
Comment 18 Fedora Update System 2014-03-30 22:09:39 EDT
photocollage-1.0.2-1.fc19 has been pushed to the Fedora 19 stable repository.
Comment 19 Fedora Update System 2014-03-30 22:09:47 EDT
photocollage-1.0.2-1.fc20 has been pushed to the Fedora 20 stable repository.

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