Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 171039 - Review Request: geos
Summary: Review Request: geos
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: David Lawrence
URL: http://ftp.intevation.de/freegis/fedo...
Whiteboard:
: 176741 (view as bug list)
Depends On:
Blocks: FE-ACCEPT 171040
TreeView+ depends on / blocked
 
Reported: 2005-10-17 16:37 UTC by Silke Reimer
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-01-16 09:31:47 UTC
Type: ---
Embargoed:


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

Description Silke Reimer 2005-10-17 16:37:05 UTC
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 04:05:49 UTC
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 18:10:39 UTC
*** Bug 176741 has been marked as a duplicate of this bug. ***

Comment 3 Shawn McCann 2006-01-01 20:39:23 UTC
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-02 04:37:52 UTC
(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 05:56:09 UTC
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-05 04:19:04 UTC
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 18:14:46 UTC
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 06:41:11 UTC
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 07:36:15 UTC
(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 07:38:48 UTC
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 23:35:42 UTC
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.