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!
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).
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
# - 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/
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.
Taking for review.
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/
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.
Additional help: http://koji.fedoraproject.org/koji/taskinfo?taskID=6657552
(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
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> - 1.0.1-1 - Add license headers in source files * Wed Mar 5 2014 Adrien Vergé <adrienverge> - 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
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.
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.
New Package SCM Request ======================= Package Name: photocollage Short Description: An image assembler with a Gtk GUI Owners: adrienverge Branches: f19 f20 InitialCC:
Git done (by process-git-requests).
Thanks everyone. I'm closing this.
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
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
photocollage-1.0.2-1.fc19 has been pushed to the Fedora 19 stable repository.
photocollage-1.0.2-1.fc20 has been pushed to the Fedora 20 stable repository.