https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Bug ID: 1058163 Summary: Review Request: glmark2 - opengl benchmark tool Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: jdisnard@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://parasense.fedorapeople.org/rpmbuild/SPECS/glmark2.spec
SRPM URL: http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc21.src.... http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc20.src.... http://parasense.fedorapeople.org/rpmbuild/SRPMS/glmark2-2012.12-1.fc19.src....
Description: Glmark2 is an OpenGL ES 2.0 benchmark tool.
Fedora Account System Username: parasense
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |rdieter@math.unl.edu Assignee|nobody@fedoraproject.org |rdieter@math.unl.edu Alias| |glmark2 Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #1 from Rex Dieter rdieter@math.unl.edu --- 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#...
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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #2 from Jon Disnard jdisnard@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #3 from Rex Dieter rdieter@math.unl.edu --- 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#...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #4 from Jon Disnard jdisnard@gmail.com --- 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....
http://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-2012.12-2.fc21.x86_6...
http://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-common-2012.12-2.fc2...
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #5 from Jon Disnard jdisnard@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #6 from Rex Dieter rdieter@math.unl.edu --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #7 from Rex Dieter rdieter@math.unl.edu --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jdisnard@gmail.com Flags| |needinfo?(jdisnard@gmail.co | |m)
--- Comment #8 from Rex Dieter rdieter@math.unl.edu --- ping, any update? (assuming you're still interested in finishing the review)
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Jon Disnard jdisnard@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(jdisnard@gmail.co | |m) |
--- Comment #9 from Jon Disnard jdisnard@gmail.com --- 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!
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #10 from Jon Disnard jdisnard@gmail.com ---
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...
https://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-2014.03-1.fc21.x86_...
https://parasense.fedorapeople.org/rpmbuild/RPMS/glmark2-common-2014.03-1.fc...
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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #11 from Rex Dieter rdieter@math.unl.edu --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #12 from Rex Dieter rdieter@math.unl.edu --- next step, https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=Pa...
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Jon Disnard jdisnard@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #13 from Jon Disnard jdisnard@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
--- Comment #14 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=1058163
Rex Dieter rdieter@math.unl.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2016-04-13 12:49:49
--- Comment #15 from Rex Dieter rdieter@math.unl.edu --- closing, imported long ago
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla