you better drop -devel in package name and add subpackage -devel like you did
for other subpackages -calculator, -pathfinder

make fox-devel sub-package own *.h *.so and .pc files
and main fox package .so* files

In addition to whar Parag said, I thing that calculator should better
be called fox-calculator and pathfinder be called fox-pathfinder
(with corresponding modifications in man pages). The names are much
too common. 

There are dots missing at the end of %descriptions.

Since they are in te same tarball, adie, pathfinder and so on should have
the same version than fox itself. Using specific versions for subpackages
will cause much pain.

Updated.

Hopefully it is fine.

http://mwiriadi.fedorapeople.org/packages/fox-devel/fox.spec
http://mwiriadi.fedorapeople.org/packages/fox-devel/fox-1.7.15-2.fc8.src.rpm

1)Good if you will split BR lines to 80 chars say
2)Missing Requires: pkgconfig on -devel package.
3) missing 
Requires: %{name} = %{version}-%{release}
on -adie, -calculator, -pathfinder, -shutterbug packages.
4) fox library used license LGPLv3+ and other subpackages are using GPLv3+

Fixed.

http://mwiriadi.fedorapeople.org/packages/fox-devel/fox.spec
http://mwiriadi.fedorapeople.org/packages/fox-devel/fox-1.7.15-3.fc8.src.rpm

There are dots missing at the end of %descriptions.

The files in %_bindir also share a common namespace, so in 
there too they should be fox-PathFinder and fox-calculator.
(and corresponding man pages).

Adie.stx looks like a config files. It should be in %_datadir, and
it should be possible to override it with a file in %_sysconfdir
and a file in $HOME/.something.

fox-config should be a wrapper around pkg-config calls to avoid
multilib conflicts.

the pkgconfig file for fox is wrong. There is certainly no need
to link against all those libs to link against fox. Libs: should
only contain link flags for libraries that will be used directly 
by applications linking against fox. Libs.static: should contain
link flags for the remaining libraries linked against the fox library
(but not needed directly by applications linking against the fox library).

You can have an idea about that by doing 

ldd -u -r /usr/bin/adie

it will show all the overlinking in that example. Looking at the 
fox headers, it seems to me that fox completly hides the underlying 
X/GL and image libs, so that Libs should only be:

Libs: -L${libdir} ${FOX_LIBS}

Regarding the include files there are very strange things, namely
there are autoconf conditionals in headers, like in fx3d.h
#ifdef HAVE_GLU_H

This is not right, these conditionals should be in .cpp files, 
not in the API.

The file xincs.h is especially full of these, and also full of 
include files for other APIs. However it doesn't seems to be included
in any other file, so it may certainly be dropped completely from
the fox API.

I'm having a chat to upstream about the issues relating to the code to get feedback.

I have linked the review request in my email he has advised that he will take a
look and comment so I'll await his comment.

ping?

Still waiting for upstream. Going to contact them again sorry about this.

(In reply to comment #6)
> There are dots missing at the end of %descriptions.
> 
> The files in %_bindir also share a common namespace, so in 
> there too they should be fox-PathFinder and fox-calculator.
> (and corresponding man pages).
> 

Upstream has advised me NOT to change it.

> Adie.stx looks like a config files. It should be in %_datadir, and
> it should be possible to override it with a file in %_sysconfdir
> and a file in $HOME/.something.
> 
> fox-config should be a wrapper around pkg-config calls to avoid
> multilib conflicts.
>

fox-config can be dropped in fedora since it is mainly for other distro's.
 
> the pkgconfig file for fox is wrong. There is certainly no need
> to link against all those libs to link against fox. Libs: should
> only contain link flags for libraries that will be used directly 
> by applications linking against fox. Libs.static: should contain
> link flags for the remaining libraries linked against the fox library
> (but not needed directly by applications linking against the fox library).
> 
> You can have an idea about that by doing 
> 
> ldd -u -r /usr/bin/adie
> 
> it will show all the overlinking in that example. Looking at the 
> fox headers, it seems to me that fox completly hides the underlying 
> X/GL and image libs, so that Libs should only be:
> 
> Libs: -L${libdir} ${FOX_LIBS}
> 
> Regarding the include files there are very strange things, namely
> there are autoconf conditionals in headers, like in fx3d.h
> #ifdef HAVE_GLU_H
> 
> This is not right, these conditionals should be in .cpp files, 
> not in the API.
> 

The condition could probably be removed, if the classes not dependent
on OpenGL are moved back to fx.h [FXVec** and so on].  These could
be useful in a 2D environment w/o GL.


> The file xincs.h is especially full of these, and also full of 
> include files for other APIs. However it doesn't seems to be included
> in any other file, so it may certainly be dropped completely from
> the fox API.
> 

This file is needed to write custom API's advised by upstream.

(In reply to comment #6)
> Adie.stx looks like a config files. It should be in %_datadir, and
> it should be possible to override it with a file in %_sysconfdir
> and a file in $HOME/.something.
> 

It is needed to be useful although an RFE will change that in the future not
sure when though.

> fox-config should be a wrapper around pkg-config calls to avoid
> multilib conflicts.

I've removed it now in a newer version thats nearly ready.

> 
> the pkgconfig file for fox is wrong. There is certainly no need
> to link against all those libs to link against fox. Libs: should
> only contain link flags for libraries that will be used directly 
> by applications linking against fox. Libs.static: should contain
> link flags for the remaining libraries linked against the fox library
> (but not needed directly by applications linking against the fox library).
> 
> You can have an idea about that by doing 
> 
> ldd -u -r /usr/bin/adie

I have removed the static libraries using the *.la how do you propose I go about
fixing this?

(In reply to comment #10)
> (In reply to comment #6)

> > The files in %_bindir also share a common namespace, so in 
> > there too they should be fox-PathFinder and fox-calculator.
> > (and corresponding man pages).
> > 
> 
> Upstream has advised me NOT to change it.

What are their arguments? The bindir is shared between all applications, 
the names of commands should be specific.

> fox-config can be dropped in fedora since it is mainly for other distro's.

If other upstream programs use fox-config it should be in fedora.
  
> > Regarding the include files there are very strange things, namely
> > there are autoconf conditionals in headers, like in fx3d.h
> > #ifdef HAVE_GLU_H
> > 
> > This is not right, these conditionals should be in .cpp files, 
> > not in the API.
> 
> The condition could probably be removed, if the classes not dependent
> on OpenGL are moved back to fx.h [FXVec** and so on].  These could
> be useful in a 2D environment w/o GL.

Whatever solution is used, the conditionals should go. An API should
never be conditional. But indeed part of the api could be factored
out in files not installed if that part of the api isn't compiled
in the library.

> > The file xincs.h is especially full of these, and also full of 
> > include files for other APIs. However it doesn't seems to be included
> > in any other file, so it may certainly be dropped completely from
> > the fox API.
> > 
> 
> This file is needed to write custom API's advised by upstream.

How can they advise that? Do they advise relying on autoconf 
conditionals? It is plain wrong.

(In reply to comment #11)
> (In reply to comment #6)
> > Adie.stx looks like a config files. It should be in %_datadir, and
> > it should be possible to override it with a file in %_sysconfdir
> > and a file in $HOME/.something.
> > 
> 
> It is needed to be useful although an RFE will change that in the future not
> sure when though.

In the mean time Adie.stx should be better located.

> > fox-config should be a wrapper around pkg-config calls to avoid
> > multilib conflicts.
> 
> I've removed it now in a newer version thats nearly ready.

I don't think this is the right way. Other packages building against
fox may need it. Better is to propose upstream a 
fox-config-pkgconfig which would be a wrapper around pkgconfig.

> > the pkgconfig file for fox is wrong. There is certainly no need
> > to link against all those libs to link against fox. Libs: should
> > only contain link flags for libraries that will be used directly 
> > by applications linking against fox. Libs.static: should contain
> > link flags for the remaining libraries linked against the fox library
> > (but not needed directly by applications linking against the fox library).
> > 
> > You can have an idea about that by doing 
> > 
> > ldd -u -r /usr/bin/adie
> 
> I have removed the static libraries using the *.la how do you propose I go about
> fixing this?

The .la files are not static libs. They are text files created by 
libtool to help linking/dlopening on platforms with insufficient shared
library support, and are therefore not needed on linux in general 
and in fedora.

The issue of the pkgconfig file is completly different. The content of
the .pc file is wrong. Indeed when linking against shared libraries
you should only link against the library which provides the symbols
you are using in your application. But if you look at the fox .pc file
you'll see that all the libraries that are linked against fox are brought
in:
pkg-config fox --libs
-lFOX-1.7 -lXext -lX11 -lXcursor -lXrender -lXrandr -lXfixes -lXi -lpthread -lrt
-ljpeg -lpng -ltiff -lz -lbz2 -lm -lGLU -lGL

Those are not needed for an application linking against fox .the only 
link flag needed is -lFOX-1.7.


Now when you link statically, you have to precise all the libraries in
the link command. Therefore pkgconfig has a way to distinguish between what
is needed to link dynamically and what is needed to link statically.

in man pkg-config you can look at Libs and Libs.private in the 
METADATA FILE SYNTAX section.

As a side note there is also a Requires.private which has the same
relation ship with Requires than Libs.private with Libs.

I better move away from this review. Patrice, feel free to review this package.

Ok, I'll do.

Created attachment 301361 [details]
fox pc patch

pc patch

Created attachment 301362 [details]
fox-config patch

fox-config.patch it should fix the links.

I have updated the spec file and also the src.rpm file. It is still rough so I
acknowledge that clean up is required.

http://mwiriadi.fedorapeople.org/packages/fox-devel/fox.spec
http://mwiriadi.fedorapeople.org/packages/fox-devel/fox-1.7.15-3.fc9.src.rpm

What I need to know is content wise what needs to change to get it approved.

Adie.stx needs to be done. Is there anything else?

Apologies for the delay in patching stuff I had to read a lot since I've never
patched the pc file or fox-config file so I'm a bit slow.

I won't accept a package that ships binary named PathFinder or calculator.

There should be dots at the end of the %description texts.

Why the 
Requires:       bzip2, zlib
The libs should be auto-detected, so why this Requires?

For the .pc patch, unless I am missing something, there are still 
some libs that are only linked through fox (though the double 
quote seems to confuse pkgconfig, and now the libs needed when 
statically linking are not present anymore. In my opinion, it 
should end up like (also note that the double quotes should not 
be there, I removed them):

....
LIBS=@LIBS@
FOX_LIBS=-lFOX-@FOX_MAJOR_VERSION@.@FOX_MINOR_VERSION@
X_LIBS=@X_LIBS@
X_BASE_LIBS=@X_BASE_LIBS@
X_EXTRA_LIBS=@X_EXTRA_LIBS@
GL_LIBS=@GL_LIBS@

Name: FOX
Description: The FOX Toolkit
URL: www.fox-toolkit.com
Version: @FOX_MAJOR_VERSION@.@FOX_MINOR_VERSION@.@FOX_PATCH_LEVEL@
Libs: ${FOX_LIBS} 
Libs.private: ${X_LIBS} ${X_BASE_LIBS} ${X_EXTRA_LIBS} ${GL_LIBS} ${LIBS}
Cflags: -I${includedir}


Also the fedora opt flags are not used.

With a release, the autoconf call is not needed (and may be harmful).

Regarding the fox-config file, I think it should better be completly rewritten
as a  pkgconfig wrapper. I'll attach it. You can then propose it to upstream to
install it as, for example fox-config-pkgconfig, and then in the fedora package
you ship fox-config-pkgconfig as fox-config.

Created attachment 302678 [details]
pkgconfig config file wrapper

Regarding the license isn't it LGPLv3+ and GPLv3+? In any case, 
the library seems to be covered by LGPL while the binaries are GPL
covered, so it should be along:

For main package
License:        LGPLv3

Nothing for the devel package (same license)

For adie, calculator...

License:        GPLv3

Closing as detailed in bug 429809.

I might have some interest in this package, spec file links seems dead,
any private copy floating around?

Redid the thing with help from a wieers rpm:

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1899111
spec: http://terjeros.fedorapeople.org/fox/fox.spec
srpm: http://terjeros.fedorapeople.org/fox/fox-1.6.37-1.fc12.src.rpm

This is for the current stable version. 

TODO: pkgconfig issue.

I don't see the Adie.stx location as blocker, it's more 
a bug in the app, not a thing to stop approval?

More fixes, should be mostly ok now, built and tested a FOX based application
without issue.

spec: http://terjeros.fedorapeople.org/fox/fox.spec
srpm: http://terjeros.fedorapeople.org/fox/fox-1.6.37-1.fc10.src.rpm