Bug 171039 - Review Request: geos
Review Request: geos
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ralf Corsepius
David Lawrence
http://ftp.intevation.de/freegis/fedo...
:
: 176741 (view as bug list)
Depends On:
Blocks: FE-ACCEPT 171040
  Show dependency treegraph
 
Reported: 2005-10-17 12:37 EDT by Silke Reimer
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-16 04:31:47 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch supposed to resolve FC5 build issues. (3.93 KB, patch)
2006-01-11 02:38 EST, Ralf Corsepius
no flags Details | Diff

  None (edit)
Description Silke Reimer 2005-10-17 12:37:05 EDT
Spec Name or Url: http://ftp.intevation.de/freegis/fedora/4/SPECS/geos.spec
SRPM Name or Url: http://ftp.intevation.de/freegis/fedora/4/SRPMS/geos-2.1.4-1.src.rpm
Description: 
GEOS (Geometry Engine - Open Source) is a C++ port of the
Java Topology Suite (JTS). As such, it aims to contain the complete
functionality of JTS in C++. This includes all the OpenGIS "Simple
Features for SQL" spatial predicate functions and spatial operators,
as well as specific JTS topology functions such as IsValid().

Some additional information:
============================
This package is an updated version of a package provided by David Kaplan last year (see https://bugzilla.fedora.us/show_bug.cgi?id=1394). I asked David wether he wanted to maintain the package. Since he hasn't been interested any more I took over and here it is.
Comment 1 Ralf Corsepius 2005-10-18 00:05:49 EDT
Some random comments:

1. rpmlint

# rpmlint geos-2.1.4-1.i386.rpm
E: geos zero-length /usr/share/doc/geos-2.1.4/ChangeLog

rpmlint is right on this. Simply remove it from the corresponding %doc line.

# rpmlint geos-devel-2.1.4-1.i386.rpm
W: geos-devel no-documentation
Can be ignored.

# rpmlint geos-doc-2.1.4-1.i386.rpm
E: geos-doc arch-dependent-file-in-usr-share
/usr/share/doc/geos-doc-2.1.4/doc/.libs/example
W: geos-doc unstripped-binary-or-object
/usr/share/doc/geos-doc-2.1.4/doc/.libs/example
E: geos-doc arch-dependent-file-in-usr-share
/usr/share/doc/geos-doc-2.1.4/doc/example.o
W: geos-doc hidden-file-or-dir /usr/share/doc/geos-doc-2.1.4/doc/.deps
W: geos-doc hidden-file-or-dir /usr/share/doc/geos-doc-2.1.4/doc/.deps
W: geos-doc hidden-file-or-dir /usr/share/doc/geos-doc-2.1.4/doc/.libs
W: geos-doc hidden-file-or-dir /usr/share/doc/geos-doc-2.1.4/doc/.libs

rpmlint is right on these. These are temporary files which must not be shipped.


2. I'd recommend to merge the *-doc package into the *-devel package.


3. geos-devel contains /usr/bin/XMLTester

IMO, this application's name is unfortunate and could (should?) be considered to
be too general for a devel-package. I'd recommend either not shipping this
binary or to rename it, rsp. to install somewhere else but to /usr/bin.

I know too little about geos rsp. XMLTester to judge if this is possible/feasible.

4. The geos-2.1.4-config.patch seems incomplete:
# geos-config --cflags
-I/usr/include
Comment 2 Ralf Corsepius 2005-12-31 13:10:39 EST
*** Bug 176741 has been marked as a duplicate of this bug. ***
Comment 3 Shawn McCann 2006-01-01 15:39:23 EST
I have updated my spec file to address the comments given in this review. See

http://www.canasoft.ca/fedora/geos.spec
http://www.canasoft.ca/fedora/geos-2.2.1-1.src.rpm

Specifically,
- ChangeLog has been removed
- doxygen files have been included in the devel package
- XMLTester has been omitted as its simply a test driver
- geos-config has been moved to the devel package

I haven't included any patches to geos-config as the baseline version looks OK
and is consistent with the upstream version.

rpmlint generates no messages
Comment 4 Ralf Corsepius 2006-01-01 23:37:52 EST
(In reply to comment #3)

Package looks fine, except

> I haven't included any patches to geos-config as the baseline version looks OK
> and is consistent with the upstream version.
The geos-config scripts is broken:

geos-config --libs reports -L/usr/lib
geos-config --cflags reports -I/usr/include

Using -L/usr/lib and -I/usr/include in compiler calls breaks library rsp.
include search paths, and therefore is never correct.
(cf. pkg-config's behavior. It filters out -I/usr/include and -L/usr/lib).


Possible improvements to the spec:
* Append --disable-static to %configure and remove %exclude %{_libdir}/*.a
--disable-static prevents the package from building static libs and building
shared libs only (Should reduce the time required for building by almost 
factor 2)

* Consider to append --disable-dependency-tracking to %configure
This should speed up building the rpm significantly.
Comment 5 Shawn McCann 2006-01-03 00:56:09 EST
Thanks for the tips. I've made the suggested improvements to the spec file and
have included a patch for geos-config.in to remove -L/usr/lib from libs and
-I/usr/include from cflags
Comment 6 Ralf Corsepius 2006-01-04 23:19:04 EST
Two minor issues:

1. Your patch had been cut in a strange way:
--- tools/geos-config.in
+++ tools/geos-config.in.new

Please change this diff to patch the original file.
[This issue doesn't get exposed when building the rpm, but does when manually
applying the patch.]

2. geos-config --ldflags
still reports -L/usr/lib


Finally, in future submissions, please increment the rpm spec's Release:-tag
when modifying a package during reviews. This helps reviewers to track the
changes between different iterations of package reviews - TIA.

Provided you add the changes mentioned above: APPROVED.
Comment 7 Shawn McCann 2006-01-07 13:14:46 EST
Patch file updated as suggested. No changes to spec.

GEOS has been uploaded into CVS and I'll request a build once the branches are
created.
Comment 8 Shawn McCann 2006-01-11 01:41:11 EST
Well the FC-4 branch built OK and geos now shows up in the FC4 repository, but
the devel build failed with the following errors:

 g++ -DHAVE_CONFIG_H -I. -I. -I../../source/headers -I../../source/headers/geos
-I../../source/headers -DGEOS_VERSION=2.2.1 -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -march=i386 -mtune=pentium4 -fasynchronous-unwind-tables -c Coordinate.cpp
 -fPIC -DPIC -o .libs/Coordinate.o
../../source/headers/geos/geom.h:361: error: extra qualification
'geos::Coordinate::' on member 'setNull'
../../source/headers/geos/geom.h:367: error: extra qualification
'geos::Coordinate::' on member 'getNull'
../../source/headers/geos/geom.h:371: error: extra qualification
'geos::Coordinate::' on member 'Coordinate'
... and so on

Anyone know what would cause problems in devel that don't show in FC4? There are
differences in the compiler flags and I assume that the g++ compiler is a newer
version than in FC4. However I thought I'd see if this is a known issue before I
dig in any try to resolve it.
Comment 9 Ralf Corsepius 2006-01-11 02:36:15 EST
(In reply to comment #8)
> the devel build failed with the following errors:
> Anyone know what would cause problems in devel that don't show in FC4?
gcc-4.1 errors out on bad c++ code that gcc-4.0 had silently swallowed.

> However I thought I'd see if this is a known issue 
It is. 

GCC-4.1 dislikes constructs like this:
class MyClass {
  int MyClass::method(...);
}

and wants
class MyClass {
  int method(...);
}
instead.
Comment 10 Ralf Corsepius 2006-01-11 02:38:48 EST
Created attachment 123041 [details]
Patch supposed to resolve FC5 build issues.

Consider the patch - It's probably incomplete, as I haven't tried it with FC5.
Comment 11 Shawn McCann 2006-01-14 18:35:42 EST
Thanks Ralf. I've applied the patch and geos now builds in devel/FC5.

I also raised this issue with the upstream developers.

This issue can now be closed (I can't change the status)

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