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. |