Spec URL: https://dl.dropboxusercontent.com/u/12575912/reviews/ballerburg.spec SRPM URL: https://dl.dropboxusercontent.com/u/12575912/reviews/ballerburg-1.1.0-1.fc20.src.rpm Description: Two castles, separated by a mountain, try to defeat each other with their cannonballs, either by killing the opponent's king or by weakening the opponent enough so that the king capitulates. Ballerburg was originally written 1987 by Eckhard Kruse, for the Atari ST machines (which were brand new computers at that point in time). Over 25 years later, here's finally the adaption of the original source code to modern operating systems. Fedora Account System Username: musuruan
Only some hints as a manually informal review: > %install > rm -rf %{buildroot} Removal of %{buildroot} is no longer necessary, except for EPEL 5. - Do you plan to provide an EPEL package? > # Install additional docs > install -p -m 644 COPYING.txt LIESMICH.txt README.txt doc/authors.txt \ > %{buildroot}%{_pkgdocdir} This is not needed and should be handled in %files section with the %doc tag: %files %doc COPYING.txt LIESMICH.txt README.txt doc/authors.txt > convert -gravity south \ … I don't understand. Why do you manipulate the icons so much? You should put a comment in the spec file why that is necessary. Ask upstream why the icons are not provided in the tarball, may there be any legal reasons? Be careful with additional or legal questionable content cause Fedora does only allow free stuff: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Content_Licenses There are two patches available at upstream, committed after the 1.1.0 release: fullscreen fix and SDL2 usage. Maybe consider to include them.
Assigned, will have to look at it at the weekend :)
(In reply to Raphael Groner from comment #1) > > %install > > rm -rf %{buildroot} > > Removal of %{buildroot} is no longer necessary, except for EPEL 5. > - Do you plan to provide an EPEL package? Good catch. It's a leftover. Removed. > > # Install additional docs > > install -p -m 644 COPYING.txt LIESMICH.txt README.txt doc/authors.txt \ > > %{buildroot}%{_pkgdocdir} > > This is not needed and should be handled in %files section with the %doc tag: > %files > %doc COPYING.txt LIESMICH.txt README.txt doc/authors.txt If I did that on older version of rpm, I would have wiped out the doc already installed by "make install". > > convert -gravity south \ > … > > I don't understand. Why do you manipulate the icons so much? You should put > a comment in the spec file why that is necessary. So much? The original image is not a square, but the desktop icon are. Therefore I resize the image using the same aspect ratio and putting a white background at the top. > Ask upstream why the icons > are not provided in the tarball, may there be any legal reasons? Be careful > with additional or legal questionable content cause Fedora does only allow > free stuff: > https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Content_Licenses Why upstream should ship an icon when they do not ship a desktop file? This package is perfectly legal. Ballerburg was released for Atari ST in Public Domain: http://www.eckhardkruse.net/atari_st/baller.html?en This is a port for Linux based on that source code. > There are two patches available at upstream, committed after the 1.1.0 > release: fullscreen fix and SDL2 usage. Maybe consider to include them. Fullscreen mode already works - just press "F" to test it. The RPM is not linked against SDL2 but against SDL 1.2 therefore the patch is useless. Spec URL: https://dl.dropboxusercontent.com/u/12575912/reviews/ballerburg.spec SRPM URL: https://dl.dropboxusercontent.com/u/12575912/reviews/ballerburg-1.1.0-2.fc20.src.rpm
(In reply to Andrea Musuruane from comment #3) > So much? The original image is not a square, but the desktop icon are. > Therefore I resize the image using the same aspect ratio and putting a white > background at the top. IMHO installing one big enough icon as %{_datadir}/pixmaps/%{name}.png would do the trick. The desktop environment should be able to scale it down.
The package looks nice so far, but you (and upstream) are missing some details on licensing. Output of fedora-review license check below, please check the following files: ballerburg-1.1.0/src/font8x16.h ballerburg-1.1.0/src/sdlgfx.c ballerburg-1.1.0/src/sdlgfx.h The other "unknown" files are ok, there are short notes that they are GPL. (Or they are the matching headers of their GPLed source files). Please clarify the licenses (maybe ask upstream for details), the sdlgfx files are not a problem (zlib license), but I cannot see any info in font8x16.h Greetings Christian GPL (v2 or later) ----------------- ballerburg-1.1.0/src/dlgAlert.c ballerburg-1.1.0/src/psg.c ballerburg-1.1.0/src/psg.h GPL (v3 or later) ----------------- ballerburg-1.1.0/src/baller1.c ballerburg-1.1.0/src/baller1.h ballerburg-1.1.0/src/baller2.c ballerburg-1.1.0/src/baller2.h ballerburg-1.1.0/src/ballergui.c ballerburg-1.1.0/src/cannoneer.c ballerburg-1.1.0/src/market.c ballerburg-1.1.0/src/market.h ballerburg-1.1.0/src/music.c ballerburg-1.1.0/src/music.h ballerburg-1.1.0/src/screen.c ballerburg-1.1.0/src/screen.h ballerburg-1.1.0/src/settings.c Unknown or generated -------------------- ballerburg-1.1.0/cmake/config-cmake.h ballerburg-1.1.0/src/ballergui.h ballerburg-1.1.0/src/font8x16.h ballerburg-1.1.0/src/i18n.h ballerburg-1.1.0/src/paths.c ballerburg-1.1.0/src/paths.h ballerburg-1.1.0/src/sdlgui.c ballerburg-1.1.0/src/sdlgui.h ballerburg-1.1.0/src/settings.h zlib/libpng ----------- ballerburg-1.1.0/src/sdlgfx.c ballerburg-1.1.0/src/sdlgfx.h
(In reply to Christian Dersch from comment #5) > The package looks nice so far, but you (and upstream) are missing some > details on licensing. Output of fedora-review license check below, please > check the following files: > > ballerburg-1.1.0/src/font8x16.h > ballerburg-1.1.0/src/sdlgfx.c > ballerburg-1.1.0/src/sdlgfx.h > > The other "unknown" files are ok, there are short notes that they are GPL. > (Or they are the matching headers of their GPLed source files). > > Please clarify the licenses (maybe ask upstream for details), the sdlgfx > files are not a problem (zlib license), but I cannot see any info in > font8x16.h Upstream replied: The font8x16.h is a generated file, it's font8x16.bmp converted to source code with "convert font8x16.bmp xbm:font8x16.h". The font bmp itself is in the public domain, so I'd say the header is in the public domain, too, so I added a corresponding statement there now. Upstream commit: http://git.tuxfamily.org/baller/baller.git/?p=baller/baller.git;a=commitdiff;h=e8257c4c13347f9b048e797227770dea857ffc2c
Detailed review below :) There are two (small) points I want to discuss. One is the documentation already mentioned by Raphael. Can you explain if the part below is still required? At least my buildsystem the manual installation of the doc isn't a requirement and I think no current Fedora needs it. > > > > # Install additional docs > > > install -p -m 644 COPYING.txt LIESMICH.txt README.txt doc/authors.txt \ > > > %{buildroot}%{_pkgdocdir} > > > > This is not needed and should be handled in %files section with the %doc tag: > > %files > > %doc COPYING.txt LIESMICH.txt README.txt doc/authors.txt > > If I did that on older version of rpm, I would have wiped out the doc > already installed by "make install". > The second point: Please add a comment on zlib licensed files in your spec. The License tag itself is fine. Now the detailed review: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v2 or later)", "GPL (v3 or later)", "Unknown or generated", "zlib/libpng". 9 files have unknown license. Detailed output of licensecheck in /home/packaging/1151464-ballerburg/licensecheck.txt ==> See comment above, no unclear files now. But please mention the zlib licensed files as a comment in spec file. Whole package is GPLv3+ of course. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: The spec file handles locales properly. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: gtk-update-icon-cache is invoked in %postun and %posttrans if package contains icons. Note: icons in ballerburg [-]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 81920 bytes in 6 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [x]: 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 is included in %doc. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Package installs a %{name}.desktop using desktop-file-install or desktop- file-validate if there is such a file. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: Dist tag is present (not strictly required in GL). [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Uses parallel make %{?_smp_mflags} macro. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: ballerburg-1.1.0-2.fc22.x86_64.rpm ballerburg-1.1.0-2.fc22.src.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint ballerburg 1 packages and 0 specfiles checked; 0 errors, 0 warnings. # echo 'rpmlint-done:' Requires -------- ballerburg (rpmlib, GLIBC filtered): /bin/sh hicolor-icon-theme libSDL-1.2.so.0()(64bit) libc.so.6()(64bit) libm.so.6()(64bit) libpthread.so.0()(64bit) rtld(GNU_HASH) Provides -------- ballerburg: application() application(ballerburg.desktop) ballerburg ballerburg(x86-64) Source checksums ---------------- http://download.tuxfamily.org/baller/ballerburg-1.1.0.tar.gz : CHECKSUM(SHA256) this package : fddf26dcc11b58885740e7e8e4e7ce8c3d3feca88a005914d0a558546467aca9 CHECKSUM(SHA256) upstream package : fddf26dcc11b58885740e7e8e4e7ce8c3d3feca88a005914d0a558546467aca9 http://baller.tuxfamily.org/king.png : CHECKSUM(SHA256) this package : 1e82f4db17785e489e805fc2b9e043578a696d1fba8943e5951c11a30f8202f7 CHECKSUM(SHA256) upstream package : 1e82f4db17785e489e805fc2b9e043578a696d1fba8943e5951c11a30f8202f7 Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14 Command line :/bin/fedora-review -m fedora-rawhide-x86_64 -b 1151464 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
(In reply to Christian Dersch from comment #7) > Detailed review below :) There are two (small) points I want to discuss. One > is the documentation already mentioned by Raphael. Can you explain if the > part below is still required? At least my buildsystem the manual > installation of the doc isn't a requirement and I think no current Fedora > needs it. I don't want to sound harsh but please explain why my method is not good. AFAIK I could even patch CMake source files to include the installation of those doc files and it would be perfectly fine. The Fedora packaging guidelines just state that "Any relevant documentation [..] should be included in the package as %doc": https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation My spec file satisfies this requirement. > The second point: Please add a comment on zlib licensed files in your spec. > The License tag itself is fine. Now the detailed review: Again, I can't find any requirement to list the license of every source file (BTW, why just the zlib licensed ones and not the others?). Fedora guidelines requires to specify the License tag and that is the license of the contents of the *binary* RPM: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines The result binary RPM is GPLv3+ (PD & GPLv2+ & GPLv3+ & zlib = GPLv3+).
Approved! I just wanted you to point out these (cosmetic) parts. The package is fine, I mentioned this above and set the review + flag now ;) (In reply to Andrea Musuruane from comment #8) > (In reply to Christian Dersch from comment #7) > > Detailed review below :) There are two (small) points I want to discuss. One > > is the documentation already mentioned by Raphael. Can you explain if the > > part below is still required? At least my buildsystem the manual > > installation of the doc isn't a requirement and I think no current Fedora > > needs it. > > I don't want to sound harsh but please explain why my method is not good. > AFAIK I could even patch CMake source files to include the installation of > those doc files and it would be perfectly fine. Fine but not beautiful ;) > > The Fedora packaging guidelines just state that "Any relevant documentation > [..] should be included in the package as %doc": > https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation > > My spec file satisfies this requirement. I didn't say this is wrong or not good, I just want to know if/why it is required ;) The most common way is %doc COPYING.txt LIESMICH.txt README.txt doc/authors.txt Then you don't need the additional doc part in %install then. I tested it and it works. The magic of rpmbuild works fine ;) > > > The second point: Please add a comment on zlib licensed files in your spec. > > The License tag itself is fine. Now the detailed review: > > Again, I can't find any requirement to list the license of every source file > (BTW, why just the zlib licensed ones and not the others?). > > Fedora guidelines requires to specify the License tag and that is the > license of the contents of the *binary* RPM: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines > > The result binary RPM is GPLv3+ (PD & GPLv2+ & GPLv3+ & zlib = GPLv3+). Thats correct and License tag is ok ;) I just think it is nicer to add a comment on other used licenses to get a better overview. It is a little bit analogous to https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios but in this case not a requirement. Greetings, Christian
(In reply to Christian Dersch from comment #9) > Approved! I just wanted you to point out these (cosmetic) parts. The package > is fine, I mentioned this above and set the review + flag now ;) Thank you for the review!
New Package SCM Request ======================= Package Name: ballerburg Short Description: Two players, two castles, and a hill in between Upstream URL: http://baller.tuxfamily.org/ Owners: musuruan Branches: f20 f21 InitialCC:
Git done (by process-git-requests).
ballerburg-1.1.0-2.fc21 has been submitted as an update for Fedora 21. https://admin.fedoraproject.org/updates/ballerburg-1.1.0-2.fc21
ballerburg-1.1.0-2.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/ballerburg-1.1.0-2.fc20
ballerburg-1.1.0-2.fc21 has been pushed to the Fedora 21 testing repository.
ballerburg-1.1.0-2.fc20 has been pushed to the Fedora 20 stable repository.
ballerburg-1.1.0-2.fc21 has been pushed to the Fedora 21 stable repository.