Bug 853784 - Review Request: tiled - Tiled Map Editor
Review Request: tiled - Tiled Map Editor
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Martin Gieseking
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-02 16:15 EDT by Erik Schilling
Modified: 2012-09-20 16:33 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-09-15 03:29:49 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
martin.gieseking: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Erik Schilling 2012-09-02 16:15:14 EDT
Spec URL: https://dl.dropbox.com/u/45541625/tiled.spec
SRPM URL: https://dl.dropbox.com/u/45541625/tiled-0.8.1-1.fc17.src.rpm
Koji Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4446448
Fedora Account System Username: ablu

Description:
Tiled is a general purpose tile map editor. It's built to be easy to use,
yet flexible enough to work with varying game engines, whether your game 
is an RPG, platformer or Breakout clone. Tiled is free software and written
in C++, using the Qt application framework.

I am not sponsered at the moment. I still need to work on getting sponsored ;)
Comment 1 Martin Gieseking 2012-09-03 05:32:44 EDT
Here are a couple of initial notes:

- Add a short comment above the License field explaining why we have a 
  multiple licensing scenario here (tiled/libtiled binary: GPLv2+, tmxviewer: 
  BSD).

- Since the package doesn't provide tiled only but also tmxviewer, you should
  add this information to the %description (and add a final period to the 
  sentence).

- As there are no development files (headers etc.) installed, you probably
  don't need to provide libtiled.so either. So I recommend to remove the file 
  and drop the devel package.

- The tarball contains a copy of zlib (src/zlib). Remove this folder in %prep
  and ensure that Fedora's zlib is linked.

- The locale files must be installed using %find_lang:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files

  Thus, add "%find_lang %{name} --with-qt" to the %install section, and append
  "-f %{name}.lang" to %files. Also, in the %files section, replace 
  %{_datadir}/tiled with %{_datadir}/tiled/images/ and add the following lines
  for proper directory ownership:
  %dir %{_datadir}/tiled/
  %dir %{_datadir}/tiled/translations

- Always try to avoid plain asterisks in the %files section. Instead it's much
  better to be more specific in order to avoid adding unexpected files to the
  package:
  %{_bindir}/tiled
  %{_bindir}/tmxviewer
  %{_mandir}/man1/tiled.1*
  %{_mandir}/man1/tmxviewer.1*

- Replace %{_libdir}/libtiled.so.1* with %{_libdir}/libtiled.so.* to simplify
  future updates.
Comment 2 Erik Schilling 2012-09-03 11:47:05 EDT
Thanks for notes

Ok i dropped the devel package though i will need to reintroduce it for the next release because it will contain the currently missing headers.

Updated the SPEC (same link): https://dl.dropbox.com/u/45541625/tiled.spec
New SRPM: https://dl.dropbox.com/u/45541625/tiled-0.8.1-2.fc17.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4448480

regards
Erik
Comment 3 Martin Gieseking 2012-09-03 13:40:40 EDT
(In reply to comment #2)
> Ok i dropped the devel package though i will need to reintroduce it for the
> next release because it will contain the currently missing headers.

OK, that's fine. 

Some additional remarks:
- Unlike mentioned in your spec, libtiled is not licensed under BSD at the 
  moment. According to the boilerplates of maprenderer.cpp, gidmapper.cpp, and 
  gidmapper.h, these files are GPLv2+. This is probably a mistake. Please ask 
  upstream for clarification. Currently, BSD- and GPLv2+-licensed code is linked 
  together, and the resulting binary must be licensed under GPLv2+ as the latter 
  it more restrictive than BSD.

- Preserve the timestamps by adding -p to the 3rd and 4th "install" statement.

- In the first two "install" statements, change the file permissions 664 to 644.
  Sorry, the 664 was a typo in one of my comments about your mana package and 
  should be fixed there as well. ;)
Comment 4 Erik Schilling 2012-09-03 15:48:08 EDT
Ok i talked with upstream. The license headers were outdated (was already fixed in master). I did a patch based on the git commit.

Fixed the other issues too:

SPEC: https://dl.dropbox.com/u/45541625/tiled.spec
SRPM: https://dl.dropbox.com/u/45541625/tiled-0.8.1-3.fc17.src.rpm

regards,
Erik
Comment 5 Martin Gieseking 2012-09-14 03:35:16 EDT
Here's the formal review of tiled. The package looks good now and is ready for check-in. Please keep in mind to adapt the License field and to update the license files in %doc once you update the package to a new upstream version that doesn't contain any GPLv2+ code any longer. 

$ rpmlint tiled*
tiled.src: W: name-repeated-in-summary C Tiled
tiled.src: W: spelling-error %description -l en_US platformer -> platformed, plat former, plat-former
tiled.src: W: spelling-error %description -l en_US tmxviewer -> interviewer
tiled.x86_64: W: name-repeated-in-summary C Tiled
tiled.x86_64: W: spelling-error %description -l en_US platformer -> platformed, plat former, plat-former
4 packages and 0 specfiles checked; 0 errors, 8 warnings.

The above warnings are expected and can be ignored.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - BSD according to boilerplates
    - some headers still mention GPLv2+ by mistake (already fixed in upstream repo)

[+] MUST: The License field in the package spec file must match the actual license.
[+] MUST: The file containing the text of the license(s) for the package must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ sha256sum tiled-qt-0.8.1.tar.gz*
    e5be7c38ceb24fbe0043648e8bc639804f3df5a60beb313eb039b2bcd56ad76c  tiled-qt-0.8.1.tar.gz
    e5be7c38ceb24fbe0043648e8bc639804f3df5a60beb313eb039b2bcd56ad76c  tiled-qt-0.8.1.tar.gz.upstream

[+] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work on an architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
[+] MUST: The spec file MUST handle locales properly.
[.] MUST: If a package installs files below %{_datadir}/icons, the icon cache must be updated.
[+] MUST: Packages storing shared library files (not just symlinks) must call ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: .so (without suffix) must go in a -devel package.
[.] MUST: devel packages must require the base package using a fully versioned dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop file. 
[+] MUST: .desktop files must be properly installed with desktop-file-install in the %install section.
[+] MUST: Packages must not own files or directories already owned by other packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: Patch files should be prefixed with %{name}-
[+] SHOULD: All patches should be commented in the spec file
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[+] SHOULD: Your package should contain man pages for binaries/scripts.

----------------
Package APPROVED
----------------
Comment 6 Erik Schilling 2012-09-14 04:50:13 EDT
Note that tiled itself is still GPL. Only libtiled got relicensed

Thanks a lot for review.
Comment 7 Martin Gieseking 2012-09-14 04:59:51 EDT
(In reply to comment #6)
> Note that tiled itself is still GPL. Only libtiled got relicensed

Ah OK. Thanks for the clarification.

 
> Thanks a lot for review.

You're welcome
Comment 8 Erik Schilling 2012-09-14 11:16:50 EDT
Thanks a lot for all!


New Package SCM Request
=======================
Package Name: tiled
Short Description: Tiled Map Editor
Owners: ablu
Branches: f16 f17 f18
InitialCC:
Comment 9 Gwyn Ciesla 2012-09-14 12:34:14 EDT
Git done (by process-git-requests).
Comment 10 Fedora Update System 2012-09-15 04:10:54 EDT
tiled-0.8.1-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/tiled-0.8.1-3.fc16
Comment 11 Fedora Update System 2012-09-15 04:12:11 EDT
tiled-0.8.1-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/tiled-0.8.1-3.fc17
Comment 12 Fedora Update System 2012-09-15 04:12:57 EDT
tiled-0.8.1-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/tiled-0.8.1-3.fc18
Comment 13 Fedora Update System 2012-09-20 16:33:36 EDT
tiled-0.8.1-3.fc18 has been pushed to the Fedora 18 stable repository.

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