Bug 530205 - Review Request: AntTweakBar - GUI library for videogame property editing UIs
Summary: Review Request: AntTweakBar - GUI library for videogame property editing UIs
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Spura
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-10-21 20:57 UTC by Sean Middleditch
Modified: 2014-07-08 03:47 UTC (History)
4 users (show)

Fixed In Version: 1.13-5.fc11
Clone Of:
Environment:
Last Closed: 2009-11-04 12:17:38 UTC
Type: ---
Embargoed:
tomspur: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sean Middleditch 2009-10-21 20:57:04 UTC
Spec URL: http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar.spec
SRPM URL: http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar-1.13-1.fc12.src.rpm
Description:
Library for easily creating and using tweakable properties in an OpenGL
or SDL application, designed primarily for professional game developers.


There are a few ugly hacks I put in the spec file to get things to build and to get rid of rpmlint warnings.  Upstream is Windows-centric and has a pretty crappy build setup for Linux.

I also had to apply two patches, and these are the first time I've applied patches with any of my packages, so hopefully I did that part right.

The only thing I'm particularly unsure about with this package is whether or not I should put the examples/demos into a sub-package.  I'm not sure they're really useful to people without the source.  I've seen a few packages that include the example source in the docs directory, but I'm not a huge fan of that approach myself.  I decided to just leave them out entirely for now.

rpmlint of spec file:
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

rpmlint of source RPM:
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint of binary RPMs:
AntTweakBar-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

(I was told during the libtelnet review that that warning was bogus and to ignore it, since there's no particular extra documentation to add to the -devel package.)

The Koji scratch build for F12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1761658

And Koji scratch build for F11:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1761680

Comment 1 Sean Middleditch 2009-10-22 19:00:55 UTC
Doh.  I was dumb and had an obvious bug that I missed because I had AntTweakBar already installed elsewhere, and rpmlint apparently didn't catch it.  I was installing the shared object as libAntTweakBar.so.0, and I wasn't created any links for libAntTweakBar.so in the -devel package making linking rather difficult.

1.13-2 spec file and SRPM with the links added to make it Right(tm):

http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar.spec
http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar-1.13-2.fc12.src.rpm

Comment 2 Thomas Spura 2009-10-22 22:39:02 UTC
(In reply to comment #0)
> 
> I also had to apply two patches, and these are the first time I've applied
> patches with any of my packages, so hopefully I did that part right.

Looks good ;)
How about using #if defined(linux) || defined(__linux) around the include patch? This way, upstream can apply this and won't break their windows build.

The second patch with optflags could be done differently:
"MINGWFLAGS="%{optflags}" make" should do the same job.


A patch name should be in the format %{name}-whatever.patch.


 
> The only thing I'm particularly unsure about with this package is whether or
> not I should put the examples/demos into a sub-package.  I'm not sure they're
> really useful to people without the source.  I've seen a few packages that
> include the example source in the docs directory, but I'm not a huge fan of
> that approach myself.  I decided to just leave them out entirely for now.

Some examples are DirectX only, but TwSimpleGLFW, TwSimpleGLUT, TwSimpleSDL, TwAdvanced1 and TwString (only .cpp files) should be included (glfw.h as depency) in the main package as doc, extra doc package only for huge examples and so on, but ~100k is not huge to me ;)



When converting to utf8, it is better to preserve timestamps, see http://fedoraproject.org/wiki/PackagingTricks#Convert_encoding_to_UTF-8

Comment 3 Sean Middleditch 2009-10-23 09:05:04 UTC
I don't believe the patch will break the Windows build, actually.  I'm rather surprised that it builds in MSVC and not GCC by default, given my own experience with both compilers; usually it's GCC that pollutes the C++ namespace when you pull in random headers due to its reliance on C headers for implementing libstdc++ and MSVC that requires strict and proper C++ header inclusion.  Go figure.  :)

I'll rename the patch files.  I suppose the MingW trick could work, but I'm honestly more included to send the patch upstream as-is to make clearly point out the problem.  The Makefile shouldn't be defining its own CXXFLAGS, it should use that name for the external configuration, and another name for internal variables.  IMO, anyways.  Granted, I'll probably just send upstream an entirely cleaned up Makefile in this case.  There are some warnings generated during build I'd like to send him another patch for anyway, and a bug I need to track down (not a showstopper at all, just a little annoying thing with SDL keybindings and the ENTER key).

I will add in the examples, not a big deal if you think it's preferable.  I put them in the -devel package, since example source doesn't make a lot of sense in the main binary IMO.

I copied the iconv thing right out of another example, but I'll fix it up to preserve the timestamp.  :)

http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar.spec
http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar-1.13-3.fc12.src.rpm

Comment 4 Thomas Spura 2009-10-23 10:12:16 UTC
(In reply to comment #3)
> I don't believe the patch will break the Windows build, actually.  I'm rather
> surprised that it builds in MSVC and not GCC by default, given my own
> experience with both compilers; usually it's GCC that pollutes the C++
> namespace when you pull in random headers due to its reliance on C headers for
> implementing libstdc++ and MSVC that requires strict and proper C++ header
> inclusion.  Go figure.  :)

Hmm, let me think about it… No I won't buy MSVC to try it ;)
I just wanted to make your patch only work for linux, as I don't know, what do to on windows and it builded without it.

Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines
 [x] Package successfully compiles and builds into binary rpms on at least one supported architecture.
     Tested on: 
       [] devel/i386 
       [] devel/x86_64
       [] F11/i386 
       [x] F11/x86_64
 [x] Rpmlint output:
     $ rpmlint AntTweakBar.spec AntTweakBar-1.13-3.fc11.src.rpm x86_64/AntTweakBar-*
4 packages and 1 specfiles checked; 0 errors, 0 warnings.

 [x] Buildroot is correct
     (%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))

//////////

 [?] Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.

The license file says: "If you use this software in a product, an acknowledgment in the product documentation would be appreciated." and in the official zlib says at the end "appreciated but is not required.", but this should be the same…

//////////

 [x] License field in the package spec file matches the actual license.
     License type: zlib
 [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] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided in the spec URL.
     Upstream source: 2c02dd71d0f86c62f022eed7e0bcb5b8
     Build source:    2c02dd71d0f86c62f022eed7e0bcb5b8
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot}.
 [x] Package consistently uses macros.

In the Source0 link is not a macro for the source version, when you sent the patches upstream, you could ask for a rename to 1.13 so %{version} will match this at the next release… Or use %global major 1 %global minor 13 as macros.


 [x] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [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] Reviewer should test that the package builds in mock.

 [x] Package functions as described (no hardware to test with).
     TwSimpleSDL works great.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.



Three simple issue:
- https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

- Patches have no comments, if they are already send upstream. Do so if they are.

- Should there bee more Requires depencies at runtime for the examples to work? I believe, if you *always* use SDL and *never* GLUT it can be pretty anoying to always need to install GLUT anyway. On the other side, without GLUT you can't compile some examples…
What's your opinion towards this?


_________________

I'd approve this now, but want to get a last answer/opionion to the last issue… ;)

Comment 5 Sean Middleditch 2009-10-23 22:52:09 UTC
MSVC Express is a free download.  :p  I still don't recommend using it if you can avoid it either way, though.  Horrifically crappy development environment compared to vim.  ;)

I emailed upstream about the license discrepancy.  In my totally non-legal opinion I don't see a problem with the slight difference in wording... but if upstream is non-responsive, what should I do to get this looked at and approved by the appropriate parties?

1) I'll replace with %global.  I copied what I had there right out of a KDE package... which is the only place I could even find that option documented.  I know it's not your fault at all, but I really have to express how immensely frustrating the documentation for Fedora packaging is.  There've been a few cases I've copied code right out of the wiki and then told it didn't meet policy.  :/

2) There are comments above the patches in the spec file... I thought that's what the policy meant.  Further expanded that patches have been mamiled.

3) My belief is that no extra Requires should be added at all.  The examples are just example code, not part of anything you actually have to compile to use AntTweakBar.  Further, the AntTweakBar.h header and API are explicitly designed to work independently of any mainloop.  The SDL, GLUT, and GLFW examples are just examples of how to integrate with those mainloops, but the user can integrate with Allegro or GTK or Qt or anything else they want to use (so long as it provides an OpenGL context).  You can easily write and compile code that links against AntTweakBar and some other toolkit that uses OpenGL internally without ever using any OpenGL header though, so even a requirement on OpenGL is not required to use AntTweakBar.  It's fine the way it is, IMO.

Updated spec and SRPM with the patch comments added and the %define->%global change made.

http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar.spec
http://middleditch.us/sean/fedora/AntTweakBar/AntTweakBar-1.13-4.fc12.src.rpm

Comment 6 Thomas Spura 2009-10-23 23:16:59 UTC
(In reply to comment #5)
> MSVC Express is a free download.  :p  I still don't recommend using it if you
> can avoid it either way, though.  Horrifically crappy development environment
> compared to vim.  ;)

Yes, another vim lover ;)

> 
> I emailed upstream about the license discrepancy.  In my totally non-legal
> opinion I don't see a problem with the slight difference in wording... but if
> upstream is non-responsive, what should I do to get this looked at and approved
> by the appropriate parties?
> 

I don't believe this is a problem, just wanted to point to the difference. When they say nothing, it's the same as not requires. If they would like to require it, they should say it…

> 1) I'll replace with %global.
OK
> 
> 2) There are comments above the patches in the spec file... I thought that's
> what the policy meant.  Further expanded that patches have been mamiled.

That they have been mailed should be the what the policy meant.
OK

> 3) My belief is that no extra Requires should be added at all.  The examples
> are just example code, not part of anything you actually have to compile to use
> AntTweakBar.  Further, the AntTweakBar.h header and API are explicitly designed
> to work independently of any mainloop.  The SDL, GLUT, and GLFW examples are
> just examples of how to integrate with those mainloops, but the user can
> integrate with Allegro or GTK or Qt or anything else they want to use (so long
> as it provides an OpenGL context).  You can easily write and compile code that
> links against AntTweakBar and some other toolkit that uses OpenGL internally
> without ever using any OpenGL header though, so even a requirement on OpenGL is
> not required to use AntTweakBar.  It's fine the way it is, IMO.

I found this argumentation in a bug elsewhere too. Even if it's sourcecode, it's just considered as documentation so the user sees from compiler errors (or needs to know) what to link.


One last thing todo:

Instead changelog:
* Wed Oct 23 2009 Sean Middleditch <sean> 1.13-4
- Use %global instead of %define

write
* Wed Oct 23 2009 Sean Middleditch <sean> 1.13-4
- Use %%global instead of %%define
- Note that patches have been sent to upstream.

This way the macros are not expanded in the changelog (what you want here ;))
Do that and it's

_________________________


APPROVED

Comment 8 Sean Middleditch 2009-10-24 20:36:32 UTC
New Package CVS Request
=======================
Package Name: AntTweakBar
Short Description: GUI library for videogame property editing UIs
Owners: elanthis
Branches: F-11 F-12

Comment 9 Kevin Fenzi 2009-10-26 20:14:57 UTC
cvs done.

Comment 10 Fedora Update System 2009-10-27 08:08:06 UTC
AntTweakBar-1.13-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/AntTweakBar-1.13-5.fc11

Comment 11 Fedora Update System 2009-10-29 03:01:18 UTC
AntTweakBar-1.13-5.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update AntTweakBar'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10888

Comment 12 Fedora Update System 2009-11-04 12:17:31 UTC
AntTweakBar-1.13-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 David Brown 2012-08-16 15:30:25 UTC
Package Change Request
======================
Package Name: AntTweakBar
New Branches: el5 el6
Owners: david.brown

I thought I did this once before but I can't find it.

Comment 14 Gwyn Ciesla 2012-08-16 15:40:52 UTC
Use your fas account name, not email.

Comment 15 David Brown 2012-08-16 15:46:58 UTC
Package Change Request
======================
Package Name: AntTweakBar
New Branches: el5 el6
Owners: dmlb2000

I thought I did this once before but I can't find it.

Comment 16 David Brown 2012-08-16 15:47:59 UTC
I was reading docs here http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure#other and it didn't mention to use the FAS account name.

Comment 17 David Brown 2012-08-16 15:49:18 UTC
Ah found it its up higher in the page under the New Package title.

Comment 18 Gwyn Ciesla 2012-08-16 15:58:25 UTC
Git done (by process-git-requests).

Comment 19 David Brown 2014-07-08 03:47:07 UTC
Package Change Request
======================
Package Name: AntTweakBar
New Branches: el7
Owners: dmlb2000

I'd like to start testing el7


Note You need to log in before you can comment on or make changes to this bug.