Bug 221717 - Review Request: agg - C++ rendering framework, move from core to shared
Review Request: agg - C++ rendering framework, move from core to shared
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-06 12:26 EST by Caolan McNamara
Modified: 2010-07-21 01:03 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-19 14:26:42 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Caolan McNamara 2007-01-06 12:26:36 EST
Spec URL: http://people.redhat.com/caolanm/agg/agg.spec
SRPM URL: http://people.redhat.com/caolanm/agg/agg-2.5-1.src.rpm
Description: Move agg a C++ rendering framework from the old core repo to the new shared one
Comment 1 Caolan McNamara 2007-01-06 12:27:26 EST
rpmlint output is...

[root@soulcrusher SPECS]# rpmlint ../RPMS/i386/agg-2.5-1.i386.rpm 
[root@soulcrusher SPECS]# rpmlint ../RPMS/i386/agg-devel-2.5-1.i386.rpm 
W: agg-devel no-documentation
Comment 2 Patrice Dumas 2007-01-06 13:59:30 EST
* I have spotted a licensing issue: gpc is shipped with agg but seems
to be covered by a GPL incompatible license:

           You may not use this software, in whole or in part, in support
           of any commercial product without the express consent of the
           author.

* I think the summary could be ameliorated, for example with
'general purpose 2D graphical library' or 'graphical rendering engine'

* in the patch, I guess the @FREETYPE_LIBS@ should be ion the
libaggfontfreetype_la_LIBADD line. Not a big deal. Has the patch
been submitted to upstream?
Comment 3 Patrice Dumas 2007-01-06 14:04:02 EST
Also a dot should be at the end of the %descriptions.
Comment 4 Jason Tibbitts 2007-01-06 14:22:07 EST
Forgive my intrusion, but mention of gpc perked my interest.  Can I assume that
the gpc referenced here has no relation to the GNU Pascal Compiler?
Comment 5 Patrice Dumas 2007-01-06 16:12:55 EST
It is the General Polygon Clipper, so has no relation with 
the GNU Pascal Compiler... The homepage is at
http://www.cs.man.ac.uk/~toby/alan/software/
Comment 6 Patrice Dumas 2007-01-06 18:57:40 EST
Some other issues I spotted:

Not blockers (personal preferences, in fact ;-):
* the news file could be shipped even though it only points to a web page
* I would personally replace
rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la
by
rm $RPM_BUILD_ROOT/%{_libdir}/*.la
* and in clean I would have used
rm -rf $RPM_BUILD_ROOT
* I tend to add a trailing / to directories in %files to show that these
are directories and not files, like
%{_includedir}/agg2/


Serious issues:
* there are dependencies missing for agg-devel:
freetype-devel (for ft2build.h), pkgconfig

* %{_libdir}/pkgconfig/ and %{_datadir}/aclocal/ are not owned. pkgconfig
owns %{_libdir}/pkgconfig/, automake owns %{_datadir}/aclocal/ (for
the aclocal dir there are more than one way to fix the issue, but it
must be fixed). 

* in libagg.pc there is 
-Wl,-rpath,${exec_prefix}/lib
Is it really needed?

Still in libagg.pc there is no reference to freetype2. Maybe there
should be a
Requires.private: freetype2
?
Comment 7 Patrice Dumas 2007-01-07 06:55:01 EST
The third subpatch for src/platform/X11/Makefile.am in 
agg-2.4-depends.patch is not applied, certainly because it lacks a line 
like 
@@ -5,6 +5,6 @@


Another issue I found is that @x_libraries@ is empty, so
there is a lone -L when linking. I think it should be fixed in 
the autotools build system.

Maybe in configure there could be something along
test -n "$x_libraries" && X_LDFLAGS="-L$x_libraries"
AC_SUBST(X_LDFLAGS)

and then in Makefile.am it could be 
libaggplatformX11_la_LDFLAGS = -version-info @AGG_LIB_VERSION@ @X_LDFLAGS@
Comment 8 Michael Schwendt 2007-01-07 07:14:01 EST
> * I would personally replace
> rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la
> by
> rm $RPM_BUILD_ROOT/%{_libdir}/*.la

The latter returns False if the target files don't exist.
The former returns True in that case and doesn't break the build.
Comment 9 Patrice Dumas 2007-01-07 07:39:18 EST
(In reply to comment #8)
> > * I would personally replace
> > rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la
> > by
> > rm $RPM_BUILD_ROOT/%{_libdir}/*.la
> 
> The latter returns False if the target files don't exist.
> The former returns True in that case and doesn't break the build.

Breaking the build in case those files aren't there anymore is
the point of my change. It helps to detect when something changed.
Comment 10 Michael Schwendt 2007-01-07 08:04:36 EST
%exclude %{_libdir}/*.la in the %files list would achieve the same.
Comment 11 Caolan McNamara 2007-01-07 09:37:46 EST
I've updated the srpm and spec, how about that now...

* Summary: Anti-Grain Geometry graphical rendering engine
* patch fixed, and @FREETYPE_LIBS@ moved, and AC_SUBST(X_LDFLAGS) tweak added
* . at end of descriptions
* news added to docs
* trailing / added to includedir %files
* freetype-devel, pkgconfig and automake added to -devel requires
* Requires.private added
* rpath removed

patches logged as:
http://sourceforge.net/tracker/index.php?func=detail&aid=1629532&group_id=42020&atid=431927
and
http://sourceforge.net/tracker/index.php?func=detail&aid=1629915&group_id=42020&atid=431927

The substantive issue is gpc, and in that case we're building with --disable-gpc
to disable building and including gpc in the final rpm
Comment 12 Patrice Dumas 2007-01-07 12:43:08 EST
Please post the link of the updated srpm...
Comment 13 Patrice Dumas 2007-01-07 14:48:01 EST
Also during review, each time you change the spec file, please 
bump the release and add a changelog entry (except maybe for 
very minor issues).
Comment 14 Patrice Dumas 2007-01-08 12:57:57 EST
(In reply to comment #11)

> The substantive issue is gpc, and in that case we're building with --disable-gpc
> to disable building and including gpc in the final rpm

It isn't enough, the srpm should not contain non free or
legally problematic code. This should be reported upstream 
anyway, upstream is violating gpc license and/or the GPL.
Comment 15 Patrice Dumas 2007-04-25 04:08:23 EDT
(In reply to comment #14)
> (In reply to comment #11)
> 
> > The substantive issue is gpc, and in that case we're building with --disable-gpc
> > to disable building and including gpc in the final rpm
> 
> It isn't enough, the srpm should not contain non free or
> legally problematic code. This should be reported upstream 
> anyway, upstream is violating gpc license and/or the GPL.

In fact it isn't sure that there is a violation as long as no 
binaries are redistributed (with gpc linked in), but still 
there should be no non-free file shipped in fedora srpm.
Comment 16 Jason Tibbitts 2007-07-08 12:45:23 EDT
It's been six months since the last comment by the submitter; setting NEEDINFO.
 I will close this ticket soon if there is no further response.
Comment 17 Patrice Dumas 2007-07-08 13:56:24 EDT
This cannot be closed, this is already in Fedora, was in core.
Comment 18 Jason Tibbitts 2007-07-08 14:10:16 EDT
Oh, joy.  Well, then it's essentially a merge review.  And so it may come down
to this package being pulled from the distro if we can't get any response to
review comments.
Comment 19 Patrice Dumas 2008-02-05 11:45:00 EST
At least gnash depends on agg. 

I can take care of this package, as a gnash dependency. Would you 
accept me as a co-maintainer?
Comment 20 Caolan McNamara 2008-02-05 11:56:03 EST
Sure, you can even be the sole maintainer if you want.
Comment 21 Patrice Dumas 2008-02-05 15:34:57 EST
I would have preferred not to be the sole maintainer, but, if you 
want to I can.
Comment 22 Patrice Dumas 2008-02-05 17:32:00 EST
I have taken care of all my comments (and a bit more).
Comment 23 Jason Tibbitts 2008-02-23 00:40:02 EST
These rpmlint complaints remain:
  agg.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libaggplatformsdl.so.2.0.4 _Z8agg_mainiPPc
  agg.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libaggplatformX11.so.2.0.4 _Z8agg_mainiPPc
I'm not sure what to make of these.

  agg.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libaggplatformsdl.so.2.0.4 /lib64/libpthread.so.0
  agg.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libaggplatformsdl.so.2.0.4 /lib64/libm.so.6
  agg.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libaggplatformX11.so.2.0.4 /lib64/libm.so.6
I don't think these are particularly problematic, but the tweak on the
CommonRpmlintIssues page fixes them up if they bother you.

If I have some free time over the weekend I'll try to finish this off.
Comment 24 Caolan McNamara 2008-02-23 05:46:34 EST
> c++filt _Z8agg_mainiPPc
agg_main(int, char**)

i.e. I believe that idea is that apps using those libraries are supposed to
provide their own "agg_main" entry point
Comment 25 Patrice Dumas 2008-02-23 06:52:50 EST
Indeed, it looks like the apps provide the entry point. Regarding 
the unused-direct-shlib-dependency, they are for very common 
libraries, so I think it is not problematic.
Comment 26 Jason Tibbitts 2008-12-17 13:48:40 EST
Looks like Kevin now owns this package; updating the CC.  I never found the time to get back to this, but I should be able to do so soon.

The rpmlint complaint list is currently:

  agg.x86_64: E: binary-or-shlib-defines-rpath 
   /usr/lib64/libaggplatformsdl.so.2.0.4 ['/usr/lib64']
  agg.x86_64: E: binary-or-shlib-defines-rpath 
   /usr/lib64/libaggplatformX11.so.2.0.4 ['/usr/lib64']
  agg.x86_64: E: binary-or-shlib-defines-rpath 
   /usr/lib64/libaggfontfreetype.so.2.0.4 ['/usr/lib64']
These are new since my last comment, and need tto be fixed up.

  agg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libaggplatformsdl.so.2.0.4 
   _Z8agg_mainiPPc
  agg.x86_64: W: undefined-non-weak-symbol /usr/lib64/libaggplatformX11.so.2.0.4 
   _Z8agg_mainiPPc
These are OK as discussed above.

  agg.x86_64: W: unused-direct-shlib-dependency 
   /usr/lib64/libaggplatformsdl.so.2.0.4 /lib64/libpthread.so.0
  agg.x86_64: W: unused-direct-shlib-dependency 
   /usr/lib64/libaggplatformsdl.so.2.0.4 /lib64/libm.so.6
  agg.x86_64: W: unused-direct-shlib-dependency 
   /usr/lib64/libaggplatformX11.so.2.0.4 /lib64/libm.so.6
These are not a big deal as discussed above, but are easy to get rid of.
Comment 27 Kevin Kofler 2008-12-17 16:52:58 EST
About the rpaths: ugh, darn autocrap. I'll use the sed hack from the guidelines to get rid of those.
Comment 28 Jason Tibbitts 2009-01-15 23:07:08 EST
Any update?  It would really nice to get rid of this ancient review ticket.  Actually, I think I'll just go ahead and do a full review.

I was about to complain about a missing pkgconfig dependency for the -devel package, but then noted that rpm automatically generates a /usr/bin/pkg-config dependency.  That seems to solve the issue, although I'm not quite sure where the dependency comes from.

So really it's just the rpmlint complaints above that could use fixing.


* source files match upstream (verified manually).
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summaries are OK.
* descriptions are OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
X rpmlint has some valid complaints.
* final provides and requires are sane:
  agg-2.5-6.fc11.x86_64.rpm
   libagg.so.2()(64bit)
   libaggfontfreetype.so.2()(64bit)
   libaggplatformX11.so.2()(64bit)
   libaggplatformsdl.so.2()(64bit)
   agg = 2.5-6.fc11
   agg(x86-64) = 2.5-6.fc11
  =
   /sbin/ldconfig
   libSDL-1.2.so.0()(64bit)
   libX11.so.6()(64bit)
   libagg.so.2()(64bit)
   libaggfontfreetype.so.2()(64bit)
   libaggplatformX11.so.2()(64bit)
   libaggplatformsdl.so.2()(64bit)
   libfreetype.so.6()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)

  agg-devel-2.5-6.fc11.x86_64.rpm
   pkgconfig(libagg) = 2.5.0
   agg-devel = 2.5-6.fc11
   agg-devel(x86-64) = 2.5-6.fc11
  =
   /usr/bin/pkg-config
   agg = 2.5-6.fc11
   automake
   freetype-devel
   libagg.so.2()(64bit)
   libaggfontfreetype.so.2()(64bit)
   libaggplatformX11.so.2()(64bit)
   libaggplatformsdl.so.2()(64bit)
   pkgconfig(freetype2)

* shared libraries present.
   ldconfig called properly
   unversioned .so links are in the -devel package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* scriptlets are OK (ldconfig).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* pkgconfig files present; pkgconfig dependency is there (via 
   /usr/bin/pkg-config auto-dep).
* no static libraries.
* no libtool .la files.
Comment 29 Kevin Kofler 2009-01-16 00:02:35 EST
> Any update?  It would really nice to get rid of this ancient review ticket. 
> Actually, I think I'll just go ahead and do a full review.

Oops, sorry, I was busy with more important stuff.

But I see the spec already reruns libtoolize, so IMHO this is a bug in libtool. The libtool 2 upgrade in Rawhide dropped the multilib patch, thinking it is no longer necessary. That turns out to be false. IMHO libtool should get fixed, not every single package running libtoolize.

> I was about to complain about a missing pkgconfig dependency for the -devel
> package, but then noted that rpm automatically generates a /usr/bin/pkg-config
> dependency.  That seems to solve the issue, although I'm not quite sure where
> the dependency comes from.

The new pkgconfig autodeps maybe?
Comment 30 Kevin Kofler 2009-01-16 00:04:09 EST
Hmmm, but it can't be the new libtool, as agg was last built in February 2008. WTF?!
Comment 31 Patrice Dumas 2009-01-17 10:24:03 EST
(In reply to comment #30)
> Hmmm, but it can't be the new libtool, as agg was last built in February 2008.
> WTF?!

Looks like Tibbs rebuilt it locally? pkgconfig(libagg) deps are fairly recent.
Comment 32 Jason Tibbitts 2009-01-17 11:05:03 EST
Yes, I always do a local mock build.  If you want, just think of it as a predictor of problems you'll see when you next rebuild.
Comment 33 Kevin Kofler 2009-01-17 13:16:07 EST
So it's a libtool bug which should be fixed in libtool.

Do you really want me to try to hack around it in agg? I'd much rather it get fixed for everyone in libtool.
Comment 34 Jason Tibbitts 2009-01-20 14:09:42 EST
You're welcome to try and get libtool changed to do what you want.  From my perspective, however, this package fails to meet the packaging guidelines and I cannot approve it until it does.  Were it a new package it would simply stay out of the distribution until it were fixed, but since this is a merge review I can't really do anything about it and you of course have no incentive to fix it.

I guess this ticket will just stay open until something gives.
Comment 35 Kevin Kofler 2009-01-20 14:19:39 EST
The thing is, this always worked previously, it's a regression in libtool (bug 478840), and absolutely not agg's fault. The guidelines explicitly suggest running libtoolize to avoid rpath trouble, agg does exactly that. And the libtool bug is causing rpaths in many packages, not just this one. It makes no sense whatsoever to block the merge review on a bug in another package.
Comment 36 Jason Tibbitts 2009-02-19 14:26:42 EST
I went and whined in that bug and it seems to have been fixed.  I'm not sure why someone who understood the problem didn't do that, but in any case the rpath problems are gone from this package after a rebuild (which will happen automatically next week anyway).

So, APPROVED.  I'll close this out.
Comment 37 Orion Poplawski 2010-07-19 19:00:00 EDT
Looks like agg is missing from RHEL6 so I'd like to see this in EPEL.  Can maintain if needed.
Comment 38 Kevin Kofler 2010-07-19 20:04:54 EDT
As far as I'm concerned (i.e. unless one of the comaintainers wants it), feel free to maintain it in EPEL, I have little to no interest in EPEL.
Comment 39 Orion Poplawski 2010-07-20 16:01:42 EDT
Package Change Request
======================
Package Name: agg
New Branches: EL-5 EL-6
Owners: orion
Comment 40 Kevin Fenzi 2010-07-21 01:03:48 EDT
CVS done (by process-cvs-requests.py).

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