Bug 851859 - Review Request: mana - Opensource 2D MMORPG platform client
Review Request: mana - Opensource 2D MMORPG platform client
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-08-26 11:39 EDT by Erik Schilling
Modified: 2012-09-20 16:39 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-20 16:39:36 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-08-26 11:39:02 EDT
Spec URL: https://dl.dropbox.com/u/45541625/mana.spec
SRPM URL: https://dl.dropbox.com/u/45541625/mana-0.6.1-0.src.rpm
Description:
The Mana client is developed as part of The Mana Project, which aims to build
a complete 2D MMORPG platform. This includes a client, server and web
component, as well as a library of free content that you can use under the
terms of the GPL.

This version of the client can connect to a specific version of the eAthena
server known as tmwAthena, a version with adaptations made as part of The Mana
World project.

Fedora Account System Username: ablu


This is my very first try to get a package into Fedora. I hope i did everything right.

Note about manaworld.
The package manaworld currently packages a branded version of this too. As soon this package is in i will try make that package depending on this one and make it delivering the branding data only (atm it copies the whole source and brands it).
Comment 1 Erik Schilling 2012-08-26 13:05:21 EDT
Here is the koji build btw: https://koji.fedoraproject.org/koji/taskinfo?taskID=4423650
Comment 2 Terje Røsten 2012-08-26 14:07:21 EDT
Looking good!

Comments:
o you can use fedorapeople.org to store spec and srpm
  http://fedoraproject.org/wiki/Infrastructure/fedorapeople.org
o consider to use %cmake macro, please visit:
  https://fedoraproject.org/wiki/Packaging:Cmake
Comment 3 Martin Gieseking 2012-08-26 14:41:49 EDT
Some further quick notes:

- The Release number should start with 1, and needs to be followed by %{?dist}:
  http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag

- You can drop the BuildRoot field. It's no longer required in Fedora.
  http://fedoraproject.org/wiki/PackagingGuidelines#BuildRoot_tag

- Check the validity of the .desktop file with desktop-file-validate 
  (in the %install section).
  http://fedoraproject.org/wiki/PackagingGuidelines#desktop-file-install_usage

- replace "mana" with %{name} in %find_lang and %files

- You can also drop the %defattr line.
  http://fedoraproject.org/wiki/PackagingGuidelines#File_Permissions
Comment 4 Erik Schilling 2012-08-26 15:33:56 EDT
Hello. Thanks a lot for replies.
I cannot upload stuff to fedorapeople.org. It says Permission denied (publickey) (i uploaded my public key already earlier this day).

I fixed the mentioned issue + i fixed a rpmlint warning about a not utf8 file.

Updated specfile (same link): https://dl.dropbox.com/u/45541625/mana.spec
Updated SRPM (same link): https://dl.dropbox.com/u/45541625/mana-0.6.1-1.fc17.src.rpm
koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=4423840

Best regards
Erik Schilling (Ablu)
Comment 5 Martin Gieseking 2012-08-26 16:18:34 EDT
Hi Erik,

your spec file looks almost fine now. There are a still a few things to be addressed, though:

- When compiling C or C++ code, you must ensure that Fedora's %{optflags} are 
  applied (http://fedoraproject.org/wiki/PackagingGuidelines#Compiler_flags).
  As you can see in the logs of your koji build log, the C++ sources are built 
  without them. Only the C file (physfsrwops.c) is compiled properly.

- The tarball contains a copy of the enet library. As far as I can see, it's not
  used. However, you must remove the enet folder in the %prep section to ensure 
  that the library code is not linked. 
  Building packages with bundled third-party libraries is not allowed in Fedora.
  Such libraries must be packaged separately.
  http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

- Please replace "%files -f mana.lang" with "%files -f %{name}.lang". 

- Finally, increase the Release number and add a corresponding %changlog entry
  every time you provide a new revision of your packages. The changelog should
  document all changes that have been applied to the preceding revision.
Comment 6 Terje Røsten 2012-08-27 02:28:31 EDT
> - Finally, increase the Release number and add a corresponding %changlog
> entry every time you provide a new revision of your packages. The changelog
> should document all changes that have been applied to the preceding revision.

Let me just said that I agree on this, some packagers seems to want keep the changelog "clean". That is a misunderstanding. A correct changelog is important for reviewers and other package maintainers. It also helps in trusting a packager if he or she is transparent with her/his actions.

Any wanted message to the end user of a package should be done in bodhi.
Comment 7 Erik Schilling 2012-08-27 13:40:06 EDT
Ok. Fixed that stuff.
I added the missing changelog entries starting with release 1 (not 0 as in first version).
Please let me know if this was ok.

Updated specfile (same link): https://dl.dropbox.com/u/45541625/mana.spec
Updated SRPM (same link): https://dl.dropbox.com/u/45541625/mana-0.6.1-3.fc17.src.rpm
koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=4427211

Thanks a lot for your feedback!

Best regards
Erik
Comment 8 Martin Gieseking 2012-08-28 09:21:25 EDT
Here comes the formal review of your package. There are some minor things left that could be fixed easily.

If you don't have a sponsor yet, I can sponsor you if you're willing to do a couple of informal reviews (aka simulated package reviews). Since you're allowed to review and approve packages of other packagers once you're a member of the packager group, it's important to practice the review procedure a bit. For further information let's email privately.


Summary of the review results:

- Replace "Opensource" with "Open source".

- In the %changelog, replace the single percent signs with double ones to 
  make rpmlint happy.

- The incorrect FSF addresses in the source files mentioned below should be 
  fixed upstream. That's not a blocker, though.

- Drop "%{buildroot}/" from the rm statement in %prep. All paths are relative 
  to the root of decompressed tarball.
  rm -rf  %{buildroot}/libs/enet => rm -rf libs/enet

- There's a manpage (mana.6) present in docs/. It should be installed 
  properly in order to make use of it:
  * add the following lines to the %install section:
    install -D -p -m664 docs/%{name}.6 %{buildroot}/%{_mandir}/man6/%{name}.6
    rm -f docs/%{name}.6
  * add %{_mandir}/man6/%{name}.6* to %files

- Macros/Variables must be used consistently, so don't mix $RPM_BUILD_ROOT and 
  %{buildroot}. Choose one variant and stick with it. 
  => replace $RPM_BUILD_ROOT with %{buildroot}


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

$ rpmlint *.rpm
mana.i686: W: spelling-error Summary(en_US) Opensource -> Open source, Open-source, Outsource
mana.i686: W: spelling-error %description -l en_US eAthena -> Athena, e Athena, heathen
mana.i686: W: spelling-error %description -l en_US tmwAthena -> Athena
mana.i686: W: dangling-symlink /usr/share/mana/data/fonts/dejavusans-bold.ttf /usr/share/fonts/dejavu/DejaVuSans-Bold.ttf
mana.i686: W: dangling-symlink /usr/share/mana/data/fonts/dejavusans-mono.ttf /usr/share/fonts/dejavu/DejaVuSansMono.ttf
mana.i686: W: dangling-symlink /usr/share/mana/data/fonts/dejavusans.ttf /usr/share/fonts/dejavu/DejaVuSans.ttf
mana.i686: W: no-manual-page-for-binary mana
mana.src: W: spelling-error Summary(en_US) Opensource -> Open source, Open-source, Outsource
mana.src: W: spelling-error %description -l en_US eAthena -> Athena, e Athena, heathen
mana.src: W: spelling-error %description -l en_US tmwAthena -> Athena
mana.src:59: W: rpm-buildroot-usage %prep rm -rf  %{buildroot}/libs/enet
mana.src:89: W: macro-in-%changelog %{mana}
mana.src:91: W: macro-in-%changelog %{optflags}
mana.src:94: W: macro-in-%changelog %cmake
mana.src:98: W: macro-in-%changelog %{name}
mana.src:99: W: macro-in-%changelog %defattr
mana-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/mana-0.6.1/src/gui/textpopup.h
mana-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/mana-0.6.1/src/chatlogger.h
mana-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/mana-0.6.1/src/gui/textpopup.cpp
mana-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/mana-0.6.1/src/chatlogger.cpp
3 packages and 0 specfiles checked; 4 errors, 16 warnings.

- dangling symlinks are expected and OK here


---------------------------------
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.
    GPLv2+ according to source file headers

[+] 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 mana-0.6.1.tar.gz*
    b945cd3e32489dfa5b8e82d571cc11e0e5308576307fca4d8dd7cf3cf6ed8c55  mana-0.6.1.tar.gz
    b945cd3e32489dfa5b8e82d571cc11e0e5308576307fca4d8dd7cf3cf6ed8c55  mana-0.6.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, ...
[+] 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.
[X] MUST: Each package must consistently use macros.
    - replace $RPM_BUILD_ROOT with %{buildroot}

[+] 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: If a package contains library files with a suffix (e.g. libfoo.so.1.1), ...
[.] 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.
[X] SHOULD: Your package should contain man pages for binaries/scripts.
    - manpage docs/mana.6 not installed properly
Comment 9 Erik Schilling 2012-08-28 11:10:57 EDT
Hi,

Thanks a lot for the review!
I am part of upstream. I will fix this incorrect fsf warnings in upstream directly.
Can you tell me how you ran rpmlint to make these show up btw? For me rpmlint does not show this warnings...

Here is fixed specfile again: https://dl.dropbox.com/u/45541625/mana.spec
The SRPM: https://dl.dropbox.com/u/45541625/mana-0.6.1-4.fc17.src.rpm

koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=4430361

Regards,
Erik
Comment 10 Martin Gieseking 2012-08-28 13:15:08 EDT
(In reply to comment #9)
> Thanks a lot for the review!

You're welcome.

> I am part of upstream. I will fix this incorrect fsf warnings in upstream
> directly.

OK, great.

> Can you tell me how you ran rpmlint to make these show up btw? For me
> rpmlint does not show this warnings...

As you can see above, the messages are the results of checking the debuginfo package. Maybe you ran rpmlint only on the source rpm. It's important to check all built packages too.


> Here is fixed specfile again: https://dl.dropbox.com/u/45541625/mana.spec
> The SRPM: https://dl.dropbox.com/u/45541625/mana-0.6.1-4.fc17.src.rpm

OK, I'll have a more detailed look later, and will approve it as soon as you've been sponsored.
Comment 11 Erik Schilling 2012-09-03 14:03:00 EDT
Fixed typo in permissions:
SPEC: https://dl.dropbox.com/u/45541625/mana.spec
new SRPM: https://dl.dropbox.com/u/45541625/mana-0.6.1-5.fc17.src.rpm
Comment 12 Martin Gieseking 2012-09-14 03:43:28 EDT
The package looks good now. Therefore, it's

---------
APPROVED.
---------
Comment 13 Erik Schilling 2012-09-14 11:16:41 EDT
Thanks a lot for all!


New Package SCM Request
=======================
Package Name: mana
Short Description: Opensource 2D MMORPG platform client
Owners: ablu
Branches: f16 f17 f18
InitialCC:
Comment 14 Martin Gieseking 2012-09-14 11:32:03 EDT
Erik, don't forget to set the fedora-cvs flag (in the "Flags" section above) to "?". Otherwise, your request doesn't get noticed. 
If you can't access the flags yet, just wait a couple of hours until your packager permissions have been propagated to all servers. Then try again.
Comment 15 Erik Schilling 2012-09-15 02:12:04 EDT
eh thx. i forgot it here and only did it for tiled. I thought i did it for both :/
Comment 16 Gwyn Ciesla 2012-09-15 10:51:02 EDT
Git done (by process-git-requests).
Comment 17 Fedora Update System 2012-09-15 11:43:51 EDT
mana-0.6.1-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mana-0.6.1-5.fc17
Comment 18 Fedora Update System 2012-09-15 11:44:02 EDT
mana-0.6.1-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/mana-0.6.1-5.fc18
Comment 19 Fedora Update System 2012-09-15 11:44:14 EDT
mana-0.6.1-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/mana-0.6.1-5.fc16
Comment 20 Fedora Update System 2012-09-16 15:17:47 EDT
mana-0.6.1-5.fc18 has been pushed to the Fedora 18 testing repository.
Comment 21 Fedora Update System 2012-09-20 16:39:36 EDT
mana-0.6.1-5.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.