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 196710
Summary: | Review Request: coldet - 3D Collision Detection Library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Hans de Goede <hdegoede> |
Component: | Package Review | Assignee: | Wart <wart> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | chris.stone, fedora-games-list |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-07-07 18:35:09 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779, 196744 |
Description
Hans de Goede
2006-06-26 16:26:42 UTC
The *-devel packages installs its headers to /usr/include, resulting into this: /usr/include/coldet.h /usr/include/math3d.h To me, /usr/include/math3d.h is too general to justify installing it there[1]. I strongly recommend to install the headers into /usr/include/coldet instead. Also, the package suffers from quite an amount of rather serious GCC warnings (esp. punned pointers), which are not unlikely to break its functionality. [1] I am working on 3d simulations myself and have several packages providing /usr/include/*/math3d.h installed. (In reply to comment #1) > The *-devel packages installs its headers to /usr/include, resulting into this: > /usr/include/coldet.h > /usr/include/math3d.h > > To me, /usr/include/math3d.h is too general to justify installing it there[1]. > I strongly recommend to install the headers into /usr/include/coldet instead. > I was thinking this at first too, then I though it would be ok and that if it wouldn't be ok I would hear so during the review :) > Also, the package suffers from quite an amount of rather serious GCC warnings > (esp. punned pointers), which are not unlikely to break its functionality. > Huh, quite an amount? on x86_64 devel I count 3 "dereferencing type-punned pointer will break strict-aliasing rules" warnings. I've once spend a days fixing all those warnings in Glide3. But I've given up there are simple to many of these warnings and 99.9% is not a problem. I know howto fix these, but I don't see how this package is any different from all the other packages we have with these kind of warnings. Here is a new version soon moving the headers to /usr/include/coldet: Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-2.src.rpm (In reply to comment #2) > (In reply to comment #1) > Huh, quite an amount? on x86_64 devel I count 3 "dereferencing type-punned > pointer will break strict-aliasing rules" warnings. I've once spend a days > fixing all those warnings in Glide3. But I've given up there are simple to > many of these warnings => Crappy, non portable code. > and 99.9% is not a problem. They might currently not be a problem, but they are very likely to become in future, once GCC should start using more agressive optimizations. I know howto fix these, but I > don't see how this package is any different from all the other packages > we have with these kind of warnings. The only difference is this package consisting of very few source files. (In reply to comment #2) Hans, I think, I need to clarify my comment: I would approve this package if you install the headers to /usr/include/coldet. My remark on the "punned pointers" was not meant to be blocker to the review. It was meant as a "BIG FAT WARNING" to you that you will not unlikely be facing "weird runtime errors" related to this package in future. MUST ==== * Package/spec named appropriately * Source matches upstream: 26c2db12ec5ad2d7a0b1d0fe2597ed4a coldet_11.zip * LGPL license ok, license file included * rpmlint output clean * spec file legible and in Am. English * Builds ok in mock on FC4, FC5, and FC6 for both i386 and x86_64 * No excessive BR: (no BR: at all!) * ldconfig called in %post/%postud as needed * headers and unversioned .so properly located in -devel subpackage * Owns all directories that it creates; doesn't own directories that it should not. * No locales * No .desktop file needed * Not relocatable * No duplicate %files * File permissions look ok * No libtool archives MUSTFIX ======= * -devel subpackage missing the -%{release} component of e-v-r when requiring the base package. (In reply to comment #5) > MUSTFIX > ======= > * -devel subpackage missing the -%{release} component of e-v-r when requiring > the base package. Fixed, new version is here: Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-2.src.rpm Correction that should be: Spec URL: http://people.atrpms.net/~hdegoede/coldet.spec SRPM URL: http://people.atrpms.net/~hdegoede/coldet-1.1-3.src.rpm All MUSTFIX items addressed. APPROVED Thanks! Imported & build, closing. |