[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|ON_QA |CLOSED Fixed In Version||warmux-11.04.1-3.fc17 Resolution||ERRATA Last Closed||2012-03-06 15:24:39 --- Comment #10 from Fedora Update System upda...@fedoraproject.org 2012-03-06 15:24:39 EST --- warmux-11.04.1-3.fc17 has been pushed to the Fedora 17 stable repository. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 Jiri Popelka jpope...@redhat.com changed: What|Removed |Added Flag||fedora-cvs? --- Comment #5 from Jiri Popelka jpope...@redhat.com 2012-02-27 05:08:35 EST --- New Package SCM Request === Package Name: warmux Short Description: 2D turn-based artillery game Owners: jpopelka bruno Branches: f17 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 --- Comment #6 from Jon Ciesla limburg...@gmail.com 2012-02-27 08:32:32 EST --- Git done (by process-git-requests). -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 Bruno Wolff III br...@wolff.to changed: What|Removed |Added CC||br...@wolff.to --- Comment #7 from Bruno Wolff III br...@wolff.to 2012-02-27 08:55:42 EST --- I will note that provides are needed in some cases. While obsoletes will get the new package installed, it won't satisfy dependencies on the old package. Provides will do that. Given warmux is a game, there isn't likely to be an external (to the warmux packages) that depend on it. So it probably won't make a difference, but I think it is still good practice to add the provides. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 --- Comment #8 from Fedora Update System upda...@fedoraproject.org 2012-02-27 09:52:57 EST --- warmux-11.04.1-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/warmux-11.04.1-3.fc17 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|VERIFIED|MODIFIED -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|MODIFIED|ON_QA --- Comment #9 from Fedora Update System upda...@fedoraproject.org 2012-02-27 17:28:31 EST --- warmux-11.04.1-3.fc17 has been pushed to the Fedora 17 testing repository. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773419
Karel Volný kvo...@redhat.com changed:
What|Removed |Added
Status|ASSIGNED|VERIFIED
Flag|fedora-review? |fedora-review+
--- Comment #3 from Karel Volný kvo...@redhat.com 2012-01-18 11:15:46 EST ---
(In reply to comment #2)
(In reply to comment #1)
1) package renaming - FAIL
there is missing
Provides: wormux = %{version}-%{release}
so the package does NOT provide a clean update path, see
https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages
I know, but actually it DOES, see:
https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#Do_I_need_to_Provide_my_old_package_names.3F
Anyway, I've added the Provides.
ah, I've missed that bit, thanks for pointing that out (maybe the guidelines
would need some updating, but ...)
however, I thought the way it works is
1. someone has wOrmux installed
2. runs yum upgrade
3. yum looks into repos what provides wOrmux
no provides:
4. wOrmux is provided only by wOrmux, yum sees no new version = no upgrade
wArmux has Provides: wOrmux:
4. yum finds that wOrmux is provided by wArmux and the provided version is
newer = upgrade from wOrmux to wArmux
i.e. without Provides, it needs user action to find out that the replacement is
available, and to explicitly install wArmux
am I mistaken?
- is it enough to have Obsoletes? - I though that Obsoletes is more like a
Conflicts, that it would expel the old package (saying it is safe to uninstall
it, unlike Conflicts which needs manual intervention to decide which one to
keep) ... reading RPM Guide, I'm really not sure how does that work when it
comes to upgrading :-(
... anyways :-) there is now
Provides: wormux = %{version}-%{release}
Provides: wormux-data = %{version}-%{release}
- ok
* MUST: The package MUST successfully compile and build into binary rpms on
at
least one primary architecture.
- FAIL - this doesn't build in rawhide, due to missing zlib include, see
http://koji.fedoraproject.org/koji/taskinfo?taskID=3697088
I've added the include, but there's still some other problem that I can't
figure out. I think it could be related to new GCC 4.7 used in rawhide as I
can't see any problem in the code.
http://koji.fedoraproject.org/koji/taskinfo?taskID=3705602
I've saw some mentions about build failures that were fixed by Petr Písař - not
sure if this could be the same issue, but you can try asking ...
--- Summary ---
I think I've fixed all the problems you mentioned except the rawhide building
problem which I'll try to narrow down later.
Spec URL: http://jpopelka.fedorapeople.org/warmux.spec
SRPM URL: http://jpopelka.fedorapeople.org/warmux-11.04.1-2.fc16.src.rpm
agreed, thanks
unfortunately, one new issue slipped in:
SPECS/warmux.spec:60: W: macro-in-comment %configure
- not a blocker, but please fix on next update
and rpmlint now reports a new error:
warmux.x86_64: E: invalid-lc-messages-dir
/usr/share/locale/cpf/LC_MESSAGES/warmux.mo
- which is a false positive, bug #782818 filed for that
(generally, the locales look okay, I've tried to play the game in Czech)
= Approved
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=773419 --- Comment #4 from Jiri Popelka jpope...@redhat.com 2012-01-18 12:02:06 EST --- (In reply to comment #3) (In reply to comment #2) i.e. without Provides, it needs user action to find out that the replacement is available, and to explicitly install wArmux am I mistaken? - is it enough to have Obsoletes? - I though that Obsoletes is more like a Conflicts, that it would expel the old package (saying it is safe to uninstall it, unlike Conflicts which needs manual intervention to decide which one to keep) ... reading RPM Guide, I'm really not sure how does that work when it comes to upgrading :-( Obsoletes is sufficient when updating. Provides is only needed when installing new package. unfortunately, one new issue slipped in: SPECS/warmux.spec:60: W: macro-in-comment %configure Already fixed in that uploaded spec file. The uploaded srpm contains this problem because I was lazy to upload (it's quite big) also fixed srpm. warmux.x86_64: E: invalid-lc-messages-dir /usr/share/locale/cpf/LC_MESSAGES/warmux.mo - which is a false positive, bug #782818 filed for that Thanks, I should have filled it myself. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773419
--- Comment #2 from Jiri Popelka jpope...@redhat.com 2012-01-16 07:27:28 EST
---
(In reply to comment #1)
Thanks for outstanding review.
1) package renaming - FAIL
there is missing
Provides: wormux = %{version}-%{release}
so the package does NOT provide a clean update path, see
https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages
I know, but actually it DOES, see:
https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#Do_I_need_to_Provide_my_old_package_names.3F
Anyway, I've added the Provides.
warmux.x86_64: E: incorrect-fsf-address /usr/share/doc/warmux-11.04.1/COPYING
- please report upstream (note that it needs to be changed in source files
too)
I'll try.
* MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
- FAIL - this doesn't build in rawhide, due to missing zlib include, see
http://koji.fedoraproject.org/koji/taskinfo?taskID=3697088
I've added the include, but there's still some other problem that I can't
figure out. I think it could be related to new GCC 4.7 used in rawhide as I
can't see any problem in the code.
http://koji.fedoraproject.org/koji/taskinfo?taskID=3705602
* MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
- FAIL, warmux puts icon into %{_datadir}/icons/hicolor/32x32/apps/ but it
doesn't seem that its dependency chain includes anything that would own that
Added
Requires: hicolor-icon-theme
- the desktop file should no longer be prefixed fedora-, renaming the
package
is a nice opportunity to change the desktop file name
Fixed.
For new packages, do not apply a vendor tag to desktop files.
- fail, the icon should NOT have .png suffix specified
Fixed.
- hm, shouldn't be that touch done only if the icon cache is to be really
updated (i.e. when the following condition is met, gtk-update-icon-cache is
available)?
Fixed according to
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
- I don't see any reason why -data should require the executables (OTOH, what
to do with data without the game?)
You actually answered that, i.e. data requires executable because it's useless
without it.
- btw, does the game work with older data? - shouldn't be the dependency of
warmux on warmux-data also versioned?
Fixed.
I do not understand one thing:
%configure --disable-nls --disable-dependency-tracking
... why disable-nls?
Changed to
%configure --enable-fribidi
and using %find_lang macro now.
--- Summary ---
I think I've fixed all the problems you mentioned except the rawhide building
problem which I'll try to narrow down later.
Spec URL: http://jpopelka.fedorapeople.org/warmux.spec
SRPM URL: http://jpopelka.fedorapeople.org/warmux-11.04.1-2.fc16.src.rpm
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 773419] Review Request: warmux - 2D turn-based artillery game
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=773419
Karel Volný kvo...@redhat.com changed:
What|Removed |Added
Status|NEW |ASSIGNED
URL||http://www.wormux.org/
CC||kvo...@redhat.com
AssignedTo|nob...@fedoraproject.org|kvo...@redhat.com
Flag||fedora-review?
--- Comment #1 from Karel Volný kvo...@redhat.com 2012-01-15 15:11:54 EST ---
1) package renaming - FAIL
Obsoletes: wormux 0.9.2.1-7
Obsoletes: wormux-data 0.9.2.1-7
- this is ok (the latest available version is 0.9.2.1-6), but there is missing
Provides: wormux = %{version}-%{release}
Provides: wormux-data = %{version}-%{release}
so the package does NOT provide a clean update path, see
https://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages
note especially Provides should be assumed to be deprecated and short lived
and removed in the distro release after the next one ... and the distro version
where it is planned to be dropped documented in a comment in the specfile -
such comment is also missing
2) must items
* MUST: rpmlint must be run on the source rpm and all binary rpms the build
produces. The output should be posted in the review.
$ rpmlint -v SPECS/warmux.spec SRPMS/warmux-11.04.1-1.fc16.src.rpm
RPMS/x86_64/warmux-11.04.1-1.fc16.x86_64.rpm
RPMS/noarch/warmux-data-11.04.1-1.fc16.noarch.rpm
SPECS/warmux.spec: I: checking-url
http://download.gna.org/warmux/warmux-11.04.1.tar.bz2 (timeout 10 seconds)
warmux.src: I: checking
warmux.src: W: spelling-error %description -l en_US toon - tun, too, ton
- http://en.wikipedia.org/wiki/Toon
warmux.src: W: spelling-error %description -l en_US firefox - Firefox,
firebox, fire fox
warmux.src: W: spelling-error %description -l en_US wilber - wilier, wilder,
Wilbert
- ok, these are names of the characters - not a single person name which would
be written with a capital letter
warmux.src: W: spelling-error %description -l en_US pre - per, ore, pee
- http://www.thefreedictionary.com/pre-eminently
warmux.src: I: checking-url http://www.wormux.org (timeout 10 seconds)
warmux.src: I: checking-url
http://download.gna.org/warmux/warmux-11.04.1.tar.bz2 (timeout 10 seconds)
warmux.x86_64: I: checking
warmux.x86_64: W: spelling-error %description -l en_US toon - tun, too, ton
warmux.x86_64: W: spelling-error %description -l en_US firefox - Firefox,
firebox, fire fox
warmux.x86_64: W: spelling-error %description -l en_US wilber - wilier,
wilder, Wilbert
warmux.x86_64: W: spelling-error %description -l en_US pre - per, ore, pee
- same as above
warmux.x86_64: I: checking-url http://www.wormux.org (timeout 10 seconds)
warmux.x86_64: W: obsolete-not-provided wormux
- see above the renaming review
warmux.x86_64: E: incorrect-fsf-address /usr/share/doc/warmux-11.04.1/COPYING
- please report upstream (note that it needs to be changed in source files too)
warmux.x86_64: W: no-manual-page-for-binary warmux-list-games
- not a problem, it is a helper utility
warmux-data.noarch: I: checking
warmux-data.noarch: I: checking-url http://www.wormux.org (timeout 10 seconds)
warmux-data.noarch: W: obsolete-not-provided wormux-data
- see above the renaming review
warmux-data.noarch: W: no-documentation
- it is part of the main package
3 packages and 1 specfiles checked; 1 errors, 12 warnings.
* MUST: The package must be named according to the Package Naming Guidelines.
- ok
* MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption.
- ok
* MUST: The package must meet the Packaging Guidelines.
- fail ... the specific problems are described under other items of this review
* MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
* MUST: The License field in the package spec file must match the actual
license.
- both okay
... except that xml_document.cpp itself doesn't mention libxml/BSD license -
report upstream?
* MUST: If (and only if) the source package includes the text of the license(s)
in its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.
- ok, %doc AUTHORS COPYING ChangeLog
* MUST: The spec file must be written in American English.
- ok
* MUST: The spec file for the package MUST be legible.
- ok; it'be nice to have more consistent indentation ...
* MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task.
- ok, 26ff65c43a9bb61a3f0529c98b943e35 matches my download (sum not provided by
upstream)
