[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Jussi Lehtola jussi.leht...@iki.fi changed: What|Removed |Added Blocks|505154(FE-SCITECH) | -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #22 from Fedora Update System upda...@fedoraproject.org 2010-10-19 03:04:14 EDT --- 3Depict-0.0.2-3.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #23 from Fedora Update System upda...@fedoraproject.org 2010-10-19 03:10:14 EDT --- 3Depict-0.0.2-3.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Fixed In Version|3Depict-0.0.2-3.fc12|3Depict-0.0.2-3.fc13 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|ON_QA |CLOSED Fixed In Version||3Depict-0.0.2-3.fc14 Resolution||ERRATA Last Closed||2010-10-18 01:40:37 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|MODIFIED|ON_QA --- Comment #20 from Fedora Update System upda...@fedoraproject.org 2010-10-08 16:32:24 EDT --- 3Depict-0.0.2-3.fc12 has been pushed to the Fedora 12 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update 3Depict'. You can provide feedback for this update here: https://admin.fedoraproject.org/updates/3Depict-0.0.2-3.fc12 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Fedora Update System upda...@fedoraproject.org changed: What|Removed |Added Status|ASSIGNED|MODIFIED -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #17 from Fedora Update System upda...@fedoraproject.org 2010-10-07 14:42:11 EDT --- 3Depict-0.0.2-3.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/3Depict-0.0.2-3.fc13 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #19 from Fedora Update System upda...@fedoraproject.org 2010-10-07 14:42:27 EDT --- 3Depict-0.0.2-3.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/3Depict-0.0.2-3.fc14 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #18 from Fedora Update System upda...@fedoraproject.org 2010-10-07 14:42:19 EDT --- 3Depict-0.0.2-3.fc12 has been submitted as an update for Fedora 12. https://admin.fedoraproject.org/updates/3Depict-0.0.2-3.fc12 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #16 from Kevin Fenzi ke...@tummy.com 2010-10-06 19:24:06 EDT --- Git done (by process-git-requests). -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
--- Comment #12 from Martin Gieseking martin.giesek...@uos.de 2010-10-05
03:23:10 EDT ---
OK, thank you for adding the manual.
Sorry for being a bit nit-picking, but I recommend to BR the virtual package
tex(latex) instead of texlive-latex. That way you stay compatible with the
old tetex packages still used in EPEL = 5, and the new package structure of
upcoming TeX Live 2010.
Since the %doc macro puts the PDF file into directory
/usr/share/doc/%{name}-%{version}/, you don't need to add the name and version
info to the filename. IMHO, using %doc docs/manual-latex/manual.pdf would be
sufficient. However, if you prefer the additional filename prefixes, it's fine
too.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #13 from D Haley my...@yahoo.com 2010-10-05 15:09:26 EDT --- I recommend to BR the virtual package tex(latex) instead of texlive-latex. That way you stay compatible with the old tetex packages still used in EPEL = 5, and the new package structure of upcoming TeX Live 2010. OK. I have updated this, but kept the naming in the %doc; I like having it in the filename. SPEC: http://mycae.fedorapeople.org/SPECS/3Depict.spec SRPM: http://mycae.fedorapeople.org/SRPMS/3Depict-0.0.2-3.fc13.src.rpm Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2515056 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 Martin Gieseking martin.giesek...@uos.de changed: What|Removed |Added Flag|fedora-review? |fedora-review+ --- Comment #14 from Martin Gieseking martin.giesek...@uos.de 2010-10-05 15:22:24 EDT --- OK, the package looks fine now. Since I can't find any further issues, we can finish here. :) Package APPROVED -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 D Haley my...@yahoo.com changed: What|Removed |Added Flag||fedora-cvs? --- Comment #15 from D Haley my...@yahoo.com 2010-10-05 18:39:28 EDT --- New Package SCM Request === Package Name: 3Depict Short Description: Valued 3D point cloud visualization and analysis Owners: mycae Branches: f12 f13 f14 InitialCC: -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #11 from D Haley my...@yahoo.com 2010-10-04 14:49:34 EDT --- I have added the PDF, as built from the latex sources. SPEC: http://mycae.fedorapeople.org/SPECS/3Depict.spec SRPM: http://mycae.fedorapeople.org/SRPMS/3Depict-0.0.2-2.fc13.src.rpm Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2511559 -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #8 from Martin Gieseking martin.giesek...@uos.de 2010-10-03 15:07:56 EDT --- File src/tree.hh seems to be licensed under GPLv3 only. Thus, you can't include it in a GPLv3+ program. Could you please clarify with the author whether GPLv3 is actually intended, or if GPLv3+ is also OK? -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
Martin Gieseking martin.giesek...@uos.de changed:
What|Removed |Added
Status|NEW |ASSIGNED
AssignedTo|nob...@fedoraproject.org|martin.giesek...@uos.de
Flag||fedora-review?
--- Comment #10 from Martin Gieseking martin.giesek...@uos.de 2010-10-03
15:52:40 EDT ---
The package looks fine so far. As the tarball contains the LaTeX sources of a
manual, I suggest to create a PDF file from it and add it to the package.
$ rpmlint /var/lib/mock/fedora-13-x86_64/result/*.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
-
key:
[+] OK
[.] OK, not applicable
[X] needs work
-
[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
- GPLv3+ according to source file headers
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: The file containing the text of the license(s) for the package must
be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
$ md5sum 3Depict-0.0.2.tar.gz*
49cb2a46bafcc8afa13889490f341963 3Depict-0.0.2.tar.gz
49cb2a46bafcc8afa13889490f341963 3Depict-0.0.2.tar.gz.1
[+] MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture.
[.] MUST: If the package does not successfully compile, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[.] MUST: The spec file MUST handle locales properly.
[.] MUST: Packages storing shared library files (not just symlinks) must call
ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates.
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix ...
[.] MUST: devel packages must require the base package.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop
file
[+] MUST: .desktop files must be installed with desktop-file-install in the
%install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.
[.] SHOULD: If the source package does not include license text(s) ...
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
- seems to work as expected
[.] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base
package.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin ...
[X] SHOULD: your package should contain man pages for binaries/scripts.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
--- Comment #7 from Martin Gieseking martin.giesek...@uos.de 2010-09-26
14:47:24 EDT ---
(In reply to comment #6)
I have done this, but I was under the impression that %{_datadir}/%{name}
automatically detected directory status, and was recursive? If I am wrong, I
would like clarification, as this may impact other packages I maintain.
Yes, you're right. %{_datadir}/%{name}/ adds the directory and all its contents
recursively including sub-directories. Thus, this single line is sufficient.
The advantage of the more verbose variant is it prevents packaging of unwanted
files/folders that might have been in installed by accident.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
--- Comment #6 from D Haley my...@yahoo.com 2010-09-25 11:33:49 EDT ---
Hello all,
I have uploaded the 0.0.2 spec and SRPM:
SPEC: http://mycae.fedorapeople.org/SPECS/3Depict.spec
SRPM: http://mycae.fedorapeople.org/SRPMS/3Depict-0.0.2-1.fc13.src.rpm
KOJI http://koji.fedoraproject.org/koji/taskinfo?taskID=2488843
Rpmlint clean.
Responses:
-
- MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
Frankly speaking it compiles, but only thanks to the following hack in the spec
file:
export LDFLAGS=-lGL -lpng
which should be removed and the configure itself using appropriately pkg-config
tool should compose compiler/linker command line adequately.
Fixed, though as previously noted this was technically OK. But yes, it is best
to fix this upstream for any other people using the ./configure system.
To automatically check for existance of the libpng, please consider to use to
PKG_CHECK_MODULES macro:
PKG_CHECK_MODULES(PNG, libpng = 1.2)
Then you can obtain linker and compiler options in the PNG_LIBS and PNG_CFLAGS
variables respectively.
This should allows you to remove some of the #ifdefs in the code like the
followings:
#if defined(__WIN32) || defined(__WIN64) || defined(__CYGWIN__)
#include libpng/png.h
#else
#include png.h
#endif
Already addressed between the 0.0.1 and 0.0.2 releases
and
#if PNG_LIBPNG_VER 10200
#error Requires libpng version 1.2.0 or greater!
#endif
The #error is still there, just in case for whatever reason, the configure
check fails (e.g under mac OS, a custom libpng is used by my associate who
makes the .dmg packages, so the configure check is overridden with
--with-libpng-link=). If this fails, then this is pretty much guaranteed to be
pulling you up short on your build env.
Apart from that there are a lot of warnings which is worth to consider to be
corrected. Notably:
a) no return statement in function returning non-void (e.g. scene.h:276:69),
b) variable may be used uninitialized in this function (e.g. uniqueID in
3Depict.cpp:624:16),
c) dereferencing type-punned pointer will break strict-aliasing rules (e.g.
endianTest.h:57:32).
NOT OK
Please note that GCC can produce many false positives, such as (b) (though (c)
was actually a bug). In this case the unitialised value usage is a false
positive, as this condition cannot occur due to the rules of the function call
or the limited data type, which are enforced by ASSERT()s, but GCC is not aware
of this and will complain.
This can be silenced by assigning these an initial value, but this masks errors
when performing memory simulations with valgrind, which show actual logic
errors and this masking is thus highly undesirable.
The same goes for return functions which should never reach the end of the
function, or case statements where there are only a limited number of valid
input values. These usually have an ASSERT(false) at the end, but do not return
a value, or be executed respectively. GCC will complain.
unsigned integer - integer casting problems are inherent when using external
libs (such as wxWidgets), whe you have functions like window-getSize(int,int).
Secondly openMP until recently did not support the unsigned int or size_t data
types as iterators, so compatibility is reduced -- you can either attend to the
warning OR support earlier openMP implementations; but not both.
Other things like:
inline float operator[](unsigned int ui) const { ASSERT(ui 3); return
value[ui];}
generate
basics.h:423: warning: array subscript is above array bounds
etc. etc.
-Wall is a useful tool, and it has picked up many bugs of mine. However, it is
not infallible (more-so as more checks are added). I have addressed those -Wall
complaints I think are valid in 0.0.2. Others I will leave. If you think a
specific warning is valid, feel free to post it, and it will be patched as
needed. I periodically audit these warnings, but may have missed some.
==
- MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line.
Ditto.
NOT OK.
Already addressed by other comment
==
$ [ `grep {buildroot} 3Depict.spec | wc -l` -gt 0 -a `grep RPM_BUILD_ROOT
3Depict.spec | wc -l` -gt 0 ] echo NOT OK
NOT OK
It looks that both %{buildroot} and $RPM_BUILD_ROOT macros are being used.
You should just pick up one and use it consistently throughout your package[3].
Fixed. (cut n paste bug)
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
Damian Wrobel dwro...@ertelnet.rybnik.pl changed:
What|Removed |Added
CC||dwro...@ertelnet.rybnik.pl,
||martin.giesek...@uos.de
--- Comment #1 from Damian Wrobel dwro...@ertelnet.rybnik.pl 2010-09-20
14:45:08 EDT ---
Hi, I did an un-official(informal) package review for the 3Depict, please find
some initial comments.
As it's one of my first package review please be understanding if something is
not perfect.
- MUST: rpmlint must be run on every package. The output should be posted in
the review.
rpmlint 3Depict-0.0.1-1.fc13.src.rpm 3Depict-0.0.1-1.fc13.i686.rpm
3Depict-debuginfo-0.0.1-1.fc13.i686.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
OK
- MUST: The package must be named according to the Package Naming Guidelines.
OK
- MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption.
OK
- MUST: The package must meet the Packaging Guidelines.
$rpmls 3Depict-0.0.1-1.fc13.i686.rpm | grep .so | wc -l
0
a) As the package do not provide any shared libraries[1], it seems to be not
necessary to have in the spec file the following lines:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
b) There is a binary blob (3Depict-0.0.1/src/FreeSans.ttf) which should be
removed from tarball or at least in the %prep section.
c) The configure seems to check for existance of GL and PNG libraries but
there is a following definition in the spec file, which shouldn't be necessary:
export LDFLAGS=-lGL -lpng
NOT OK
- MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
OK, GPLv3+
- MUST: The License field in the package spec file must match the actual
license.
OK.
- MUST: If (and only if) the source package includes the text of the license(s)
in its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.
OK
- MUST: The spec file must be written in American English.
OK. With remark that it's probably reasonable to consider to change the
Summary:
-Summary: Valued 3D point cloud visualization and analysis
with
+Summary: 3D point cloud visualization and analysis program
- MUST: The spec file for the package MUST be legible.
OK
- MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.
$spectool 3Depict.spec | grep Source0 | awk '{print $2}' | wget -i - -O - -o
/dev/null | md5sum
3040bc31eb884c7882fc8c446d1f2a34 -
$ rpmdev-md5 3Depict-0.0.1-1.fc13.src.rpm | grep .tar
3040bc31eb884c7882fc8c446d1f2a34 3Depict-0.0.1.tar.gz
OK
- MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
Frankly speaking it compiles, but only thanks to the following hack in the spec
file:
export LDFLAGS=-lGL -lpng
which should be removed and the configure itself using appropriately pkg-config
tool should compose compiler/linker command line adequately.
To automatically check for existance of the libpng, please consider to use to
PKG_CHECK_MODULES macro:
PKG_CHECK_MODULES(PNG, libpng = 1.2)
Then you can obtain linker and compiler options in the PNG_LIBS and PNG_CFLAGS
variables respectively.
This should allows you to remove some of the #ifdefs in the code like the
followings:
#if defined(__WIN32) || defined(__WIN64) || defined(__CYGWIN__)
#include libpng/png.h
#else
#include png.h
#endif
and
#if PNG_LIBPNG_VER 10200
#error Requires libpng version 1.2.0 or greater!
#endif
Apart from that there are a lot of warnings which is worth to consider to be
corrected. Notably:
a) no return statement in function returning non-void (e.g. scene.h:276:69),
b) variable may be used uninitialized in this function (e.g. uniqueID in
3Depict.cpp:624:16),
c) dereferencing type-punned pointer will break strict-aliasing rules (e.g.
endianTest.h:57:32).
NOT OK
- MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line.
Ditto.
NOT OK.
- MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of the Packaging
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #2 from D Haley my...@yahoo.com 2010-09-20 14:57:33 EDT --- Thanks for the review. You have raised many points which I need to address. However, I am planning on a 0.0.2 release this weekend, so I think it would be best if I address most of these concerns in the next tarball I generate, and upload a .spec thereafter. If you have any additional concerns here, or wish to have a review cycle before I do the next tarball release, please let me know. Just one point concerning the summary comment; the idea is to show the program does not just work on point clouds, but rather point clouds with an associated scalar value (hence valued point clouds) -- any ideas how this could be improved? Thanks -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #3 from D Haley my...@yahoo.com 2010-09-20 15:06:31 EDT --- Sorry for the duplicate posts: Can you please clarify your NOT OK for ExcludeArch? Is there an arch missing? I thought fedora dropped ppc. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 --- Comment #4 from Damian Wrobel dwro...@ertelnet.rybnik.pl 2010-09-20 15:13:59 EDT --- (In reply to comment #3) I've marked it as a NOT OK, just because it almost don't compile for the primary architecture (see PNG BR problem). -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=622314
--- Comment #5 from Martin Gieseking martin.giesek...@uos.de 2010-09-20
17:17:46 EDT ---
Hi D Haley and Damian,
Since I'm sponsoring Damian, here are some comments on his remarks. :)
Just one point concerning the summary comment; the idea is to show the
program does not just work on point clouds, but rather point clouds with an
associated scalar value (hence valued point clouds) -- any ideas how this
could be improved?
I'm not a native English speaker, but maybe something like
Application to visualize and analyze valued 3D point clouds
could be a possible variant.
Damian, I agree with most of your comments. Here are just a couple of
additional notes and corrections.
- MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
Frankly speaking it compiles, but only thanks to the following hack in the
spec
file:
export LDFLAGS=-lGL -lpng
which should be removed and the configure itself using appropriately
pkg-config
tool should compose compiler/linker command line adequately.
NOT OK
These are things to be fixed by upstream, and the packager usually don't need
to address them as long as a simple addition of explicit LDFLAGS leads to a
working binary RPM. But since D Haley is the upstream developer, he might want
to fix these issues anyway.
- MUST: If the package does not successfully compile, build or work ...
Ditto.
NOT OK.
The package compiles, builds and works properly, so this MUST item is OK.
- MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of the Packaging Guidelines ;
inclusion of those as BuildRequires is optional. Apply common sense.
There is no BR: for libpng.
NOT OK.
BR: libpng-devel is not required as it's added as an indirect dependency of
wxGTK-devel. If a package builds in mock/koji, usually all necessary BRs are
present.
%{_mandir}/man1/%{name}.1.gz
The .gz suffix of manpages should be avoided since the compression format
applied by rpmbuild might change. Thus, %{_mandir}/man1/%{name}.1* is more
appropriate.
- MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed ...
NOT OK. See note about macro consistency.
The .desktop file is present and it is installed properly with
desktop-file-install. = OK so far.
However, the icon 3Depict referenced in the .desktop file is missing. It
should be added to the package.
Finally, the spec file should get a short comment above Patch0 describing the
purpose of the patch.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are on the CC list for the bug.
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 622314] Review request: 3Depict- Valued point cloud visualisation and analysis
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=622314 D Haley my...@yahoo.com changed: What|Removed |Added Blocks||505154(FE-SCITECH) -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
