Spec URL: http://parasense.fedorapeople.org/rpmbuild/SPECS/glmark2.spec SRPM URL: http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc21.src.rpm http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc20.src.rpm http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc19.src.rpm Description: Glmark2 is an OpenGL ES 2.0 benchmark tool. Fedora Account System Username: parasense
A few initial comments: 0. rpmlint mostly clean, just cosmetic: $ rpmlint *.rpm glmark2.src:85: W: mixed-use-of-spaces-and-tabs (spaces: line 85, tab: line 1) 1 packages and 0 specfiles checked; 0 errors, 1 warnings. please fix. 1. SHOULD omit deprecated Group: tag (unless you intend to support old rpm versions, like whats in rhel5) 2. what's the justification/rationale for all the subpackages? Could this be simplified into a single package? If continuing with keeping things split up, please include justification in a .spec comment. 3. MUST omit these extraneous BuildRequires: BuildRequires: libstdc++ BuildRequires: gcc-c++ see https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2 3. SHOULD simplify subpackage handling to not use -n, for example, from %package -n glmark2-assets to %package assets and %files -n glmark2-assets to %files assets 4. MUST improve Summary for -assets subpkg, Summary: Benchmark for OpenGL 2.0 (while we're at it, consider using a more conventional subpkg name, like -common) 5. MUST use versioned subpkg dependencies, for example, use Requires: %{name}-assets = %{version}-%{release} instead of Requires: glmark2-assets for similar reason(s) as outlined here: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Requiring_Base_Package 6. SHOULD consider using virtual/compat BuildRequires: libjpeg-devel instead of BuildRequires: libjpeg-turbo-devel since it really only wants any libjpeg, not libjpeg-turbo specifically (or does it?) Similarly, consider more-compatible BuildRequires: pkgconfig(libpng12) instead of BuildRequires: libpng12-devel
0. rpmlint issue resolved rpmlint ~/rpmbuild/SPECS/glmark2.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. 1. Removed depreciated group tags. 2. Added justification/rational for sub-packaged in .spec. Please say if the rational is reasonable in your opinion. 3. Removed extraneous BuildRequires. 3*. simplified subpackage handling to not use -n. 4. Fixed summaries, and renamed -assets to -common 5. added versioned subpkg dependencies. 6. Actually libjpeg-turbo is preferable for performance, and other reasons. This is the one and only that I would resist, only because performance is important in benchmark. 7. All BRs that have .pc files were convered to pkgconfig(foo) style. Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6479303 updated spec files here: http://parasense.fedorapeople.org/rpmbuild/SPECS/glmark2.spec updated SRPMS here (all three supported branch): http://parasense.fedorapeople.org/rpmbuild/SRPMS/ updated RPMS here: http://parasense.fedorapeople.org/rpmbuild/RPMS/ and sources: http://parasense.fedorapeople.org/rpmbuild/SOURCES/ Additionally I've added two more sub-packages for KMS graphics, might be going crazy on sub-packges for single bins... my first package with sub packages, but the rational is each bin is identical but with some fundamentally diff features in-built during compilation. These are useful to embedded developers who may only have openGL es2 DRM hardware... no x11, no good regular openGL... just es2 extensions, etc... Please check it out and let me know. Thanks
2 thanks, though I still personally don't think that rationale is good enough to justify the split binary packages. Now, if each binary subpkg pulled in different and big dependencies, then maybe. -common is good. There is no guideline about this, so it's not a review blocker, but I would urge you to reconsider a simpler solution of shipping only a main and -common subpkg. (If you do, I'd be happy to look over the changes prior to committing anything). 6 is still bogus imho. you're benchmarking *opengl* here, not jpg loading speed. not a blocker. and, please do remember to always bump Release and and changelogs when updating the package (even for reviews!), see https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Changelogs The koji scratch build you referenced doesn't include the fixes (still has -assets subpkg, and and only a -es2 subpkg) Looking good, I'll try to go over nitty-gritty stuff like licensing, source verification later today.
Have eliminated all but -common (noarch) sub-package. This one I would like to keep to save space on the distribution, and 3rd party mirrors. The main/single package bundles all the bins, .desktop files, and icons. But there is only one appdata file, and rpmlint complains about appdata as an error. $ rpmlint /var/lib/mock/fedora-19-x86_64/root//builddir/build//RPMS/glmark2-2012.12-2.fc19.x86_64.rpm ... glmark2.x86_64: E: script-without-shebang /usr/share/appdata/glmark2.appdata.xml ... Should I remove all the extra .desktop files to avoid spamming the Graphics category of a app menu in whatever DE ? Maybe choose only the .desktop files that goes with the appdata? Let the user run the other variants from cmdline? Removed libjpeg-turbo, but I really do feel that one is a better library. No matter, it's too trivial as you point out loading jpeg is not intense. Bumped the release. http://parasense.fedorapeople.org/rpmbuild/SPECS/glmark2.spec http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-2.fc21.src.rpm http://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-2012.12-2.fc21.x86_64.rpm http://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-common-2012.12-2.fc21.noarch.rpm
Regarding the nitty-gritty stuff like licensing, source verification: license is GPLv3 [1] Is there anything I can do to help with the src verification? [1] https://launchpad.net/glmark2
Sorry for the delay, reviewing recent changes now. One thing I'm noticing now that *may* be a problem at some point... I see the build is using gcc option -Werror, while a good developer switch/tool, is often problematic for distro/release type builds.
OK, a few more items, and I think we're close to golden: 8. MUST fix dir ownership, currently, you include: %files common ## assets: models, shaders, textures %{_datadir}/glmark2/models/* %{_datadir}/glmark2/shaders/* %{_datadir}/glmark2/textures/* but then nothing owns %{_datadir}/glmark2 %{_datadir}/glmark2/models/ %{_datadir}/glmark2/shaders/ %{_datadir}/glmark2/textures/ either switch to just: %{_datadir}/glmark2/ or add %dir %{_datadir}/glmark2/ %dir %{_datadir}/glmark2/models/ %dir %{_datadir}/glmark2/shaders/ %dir %{_datadir}/glmark2/textures/ 9. SHOULD remove redundant .desktop file stuff in %check. desktop-file-install run in %install section already validates stuff while it installs 10. SHOULD consider adding transitive dep in -common: Requires: %{name} = %{version}-%{release} unless this content is useful beyond this package, where users may have a use-case to have this -common subpkg installed without the associated binaries 11. MUST remove bundled libraries, prior to building src/libjpeg-turbo src/libpng I'd suggest adding to %prep, right after %setup, rm -rv src/libjpeg-turbo src/libpng See also: https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries 12. SHOULD consider moving 'waf configure' to %build stage. I don't think we have any waf-specific guideline about this, but it feels akin to autoconf's ./configure to me, and this fits better in %build licensing: ok licensecheck -r glmark2-2012.12 | cut -d: -f2 | sort -u GPL (v3 or later) MIT/X11 (BSD like) looks like you could even do GPLv3+ here, but if upstream's intend is to be GPLv3-only, that takes precedence. sources: ok, verified 4f306664aa3886fa0cf93853603603f8 glmark2-2012.12.tar.gz
ping, any update? (assuming you're still interested in finishing the review)
Hi Rex, Yes I'm still interested in finishing this package. Thank you for the reminder. Will sit down on Saturday and knock this out. Thanks!
Hi Rex, Sry for the long long pause in glmark2 progress. Here we go: I've updated the package to the latest bits. https://parasense.fedorapeople.org/rpmbuild/SPECS/glmark2.spec https://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2014.03-1.fc21.src.rpm https://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-2014.03-1.fc21.x86_64.rpm https://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-common-2014.03-1.fc21.noarch.rpm and the koji scratch build http://koji.fedoraproject.org/koji/taskinfo?taskID=8514783 I hope all the previous "MUST" requirements and "SHOULD" items were addressed. Thank you for reviewing. -Jon
sources: ok 739859cf57d4c8a23452c43e84f66e56 glmark2-2014.03.tar.gz 13. SHOULD use better appdata validation, current best practice is to: and use %if 0%{?fedora} > 19 BuildRequires: libappstream-glib %endif and %check appstream-util validate-relax --nonet %{buildroot}%{_datadir}/appdata/%{name}.appdata.xml ||: you can omit the conditional, if you're already intending to target f20+ but that's not a blocker, and the rest looks great to me now, APPROVED.
next step, https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
New Package SCM Request ======================= Package Name: glmark2 Short Description: OpenGL benchmark Upstream URL: https://github.com/glmark2/glmark2 Owners: parasense Branches: f20 f21 f22 InitialCC: parasense
Git done (by process-git-requests).
closing, imported long ago