[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Fixed In Version||photocollage-1.0.2-1.fc19 Resolution|NEXTRELEASE |ERRATA --- Comment #18 from Fedora Update System upda...@fedoraproject.org --- photocollage-1.0.2-1.fc19 has been pushed to the Fedora 19 stable repository. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Fixed In Version|photocollage-1.0.2-1.fc19 |photocollage-1.0.2-1.fc20 --- Comment #19 from Fedora Update System upda...@fedoraproject.org --- photocollage-1.0.2-1.fc20 has been pushed to the Fedora 20 stable repository. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978
--- Comment #9 from Kalev Lember kalevlem...@gmail.com ---
(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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Kalev Lember kalevlem...@gmail.com changed: What|Removed |Added Flags||fedora-review+ --- Comment #10 from Kalev Lember kalevlem...@gmail.com --- 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é adrienve...@gmail.com - 1.0.1-1 - Add license headers in source files * Wed Mar 5 2014 Adrien Vergé adrienve...@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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #11 from Kalev Lember kalevlem...@gmail.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Kalev Lember kalevlem...@gmail.com changed: What|Removed |Added Blocks|177841 (FE-NEEDSPONSOR) | Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #12 from Adrien Vergé adrienve...@gmail.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #13 from Adrien Vergé adrienve...@gmail.com --- New Package SCM Request === Package Name: photocollage Short Description: An image assembler with a Gtk GUI Owners: adrienverge Branches: f19 f20 InitialCC: -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Adrien Vergé adrienve...@gmail.com changed: What|Removed |Added Flags||fedora-cvs? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #14 from Jon Ciesla limburg...@gmail.com --- Git done (by process-git-requests). -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Jon Ciesla limburg...@gmail.com changed: What|Removed |Added Flags|fedora-cvs? |fedora-cvs+ -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #15 from Adrien Vergé adrienve...@gmail.com --- Thanks everyone. I'm closing this. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Adrien Vergé adrienve...@gmail.com changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution|--- |NEXTRELEASE Last Closed||2014-03-21 13:01:58 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #16 from Fedora Update System upda...@fedoraproject.org --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #17 from Fedora Update System upda...@fedoraproject.org --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Kalev Lember kalevlem...@gmail.com changed: What|Removed |Added Status|NEW |ASSIGNED CC||kalevlem...@gmail.com Assignee|nob...@fedoraproject.org|kalevlem...@gmail.com --- Comment #5 from Kalev Lember kalevlem...@gmail.com --- Taking for review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978
--- Comment #6 from Kalev Lember kalevlem...@gmail.com ---
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/
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #7 from Adrien Vergé adrienve...@gmail.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #8 from Christopher Meng cicku...@gmail.com --- Additional help: http://koji.fedoraproject.org/koji/taskinfo?taskID=6657552 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978
--- Comment #4 from Adrien Vergé adrienve...@gmail.com ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Adrien Vergé adrienve...@gmail.com changed: What|Removed |Added Blocks||177841 (FE-NEEDSPONSOR) Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 Antonio Trande anto.tra...@gmail.com changed: What|Removed |Added CC||anto.tra...@gmail.com --- Comment #1 from Antonio Trande anto.tra...@gmail.com --- 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). -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978 --- Comment #2 from Adrien Vergé adrienve...@gmail.com --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1073978] Review Request: photocollage - An image assembler with a Gtk GUI
https://bugzilla.redhat.com/show_bug.cgi?id=1073978
--- Comment #3 from Christopher Meng cicku...@gmail.com ---
# - 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/
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
