This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 431665 - Review Request: fox - A C++ library for GUI development
Review Request: fox - A C++ library for GUI development
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2008-02-06 05:16 EST by Marc Wiriadisastra
Modified: 2010-01-03 16:21 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-02 21:57:49 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
fox pc patch (674 bytes, patch)
2008-04-05 00:36 EDT, Marc Wiriadisastra
no flags Details | Diff
fox-config patch (657 bytes, patch)
2008-04-05 00:37 EDT, Marc Wiriadisastra
no flags Details | Diff
pkgconfig config file wrapper (1.26 KB, text/plain)
2008-04-16 17:56 EDT, Patrice Dumas
no flags Details

  None (edit)
Description Marc Wiriadisastra 2008-02-06 05:16:30 EST
Spec URL: http://mwiriadi.fedorapeople.org/packages/fox-devel/fox-devel.spec
SRPM URL: http://mwiriadi.fedorapeople.org/packages/fox-devel/fox-devel-1.7.15-1.fc8.src.rpm

Description: 
FOX is a C++-based library for graphical user interface development
FOX supports modern GUI features, such as drag-and-drop, tooltips, tab
books, tree lists, icons, multiple document interfaces (MDI), timers,
idle processing, automatic GUI updating, as well as OpenGL/Mesa for
3D graphics. Subclassing of basic FOX widgets allows for easy
extension beyond the built-in widgets by application writers.
The fox-devel package contains the files necessary to develop applications
using the FOX GUI toolkit: the header files, the reswrap resource compiler,
manual pages, and HTML documentation.

I'm not to sure if there should be a separate doc package.  I'm also unsure as to whether to create a separate fox package or rename it libfox?

Only rpmlint issues.

[marc@localhost i386]$ rpmlint fox-devel-1.7.15-1.fc8.i386.rpm 
fox-devel.i386: W: file-not-utf8 /usr/share/doc/fox-1.7/html/styles.css
fox-devel.i386: W: file-not-utf8 /usr/share/doc/fox-1.7/html/menu.css
fox-devel.i386: W: no-dependency-on fox

Other packages built contain no rpmlint errors/warnings
Comment 1 Parag AN(पराग) 2008-02-06 05:53:31 EST
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
Comment 2 Patrice Dumas 2008-02-06 06:07:15 EST
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.
Comment 4 Parag AN(पराग) 2008-02-07 05:41:30 EST
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+
Comment 6 Patrice Dumas 2008-02-08 05:17:25 EST
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.
Comment 7 Marc Wiriadisastra 2008-02-10 05:46:36 EST
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.

Comment 8 Parag AN(पराग) 2008-02-27 21:55:51 EST
ping?
Comment 9 Marc Wiriadisastra 2008-02-28 04:53:03 EST
Still waiting for upstream. Going to contact them again sorry about this.
Comment 10 Marc Wiriadisastra 2008-03-06 06:02:44 EST
(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.
Comment 11 Marc Wiriadisastra 2008-03-08 00:18:27 EST
(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?

Comment 12 Patrice Dumas 2008-03-08 03:46:28 EST
(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.
Comment 13 Patrice Dumas 2008-03-08 04:04:47 EST
(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.
Comment 14 Parag AN(पराग) 2008-04-03 23:40:28 EDT
I better move away from this review. Patrice, feel free to review this package.
Comment 15 Patrice Dumas 2008-04-04 03:57:41 EDT
Ok, I'll do.
Comment 16 Marc Wiriadisastra 2008-04-05 00:36:03 EDT
Created attachment 301361 [details]
fox pc patch

pc patch
Comment 17 Marc Wiriadisastra 2008-04-05 00:37:25 EDT
Created attachment 301362 [details]
fox-config patch

fox-config.patch it should fix the links.
Comment 18 Marc Wiriadisastra 2008-04-05 00:46:11 EDT
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.
Comment 19 Patrice Dumas 2008-04-16 17:55:52 EDT
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.
Comment 20 Patrice Dumas 2008-04-16 17:56:55 EDT
Created attachment 302678 [details]
pkgconfig config file wrapper
Comment 21 Patrice Dumas 2008-04-16 18:07:38 EDT
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
Comment 22 Jason Tibbitts 2008-07-02 21:57:49 EDT
Closing as detailed in bug 429809.
Comment 23 Terje Røsten 2010-01-02 13:30:12 EST
I might have some interest in this package, spec file links seems dead,
any private copy floating around?
Comment 24 Terje Røsten 2010-01-02 17:24:15 EST
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?
Comment 25 Terje Røsten 2010-01-03 16:21:38 EST
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

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