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 <mycae@xxxxxxxxx> 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) ====================================== >- MUST: A package must own all directories that it creates. If it does not >create a directory that it uses, then it should require a package which does >create that directory. >OK. But according to the Packaging Guide[2] it would be recommended to >construct the %files section as following: >%files >%defattr(-,root,root,-) >%doc COPYING AUTHORS ChangeLog README TODO >%{_bindir}/%{name} >%dir %{_datadir}/%{name} >%dir %{_datadir}/%{name}/textures >%{_datadir}/%{name}/textures/*.png >%{_datadir}/applications/%{name}.desktop >%{_mandir}/man1/%{name}.1.gz >to avoid unowned/unwanted directories/files. 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. ====================================== >- 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. Done, but already addressed by Comment #5 ====================================== >- MUST: Packages containing GUI applications must include a %{name}.desktop >file, and that file must be properly installed with desktop-file-install in the >%install section. If you feel that your packaged GUI application does not need >a .desktop file, you must put a comment in the spec file with your explanation. >NOT OK. See note about macro consistency. Addressed above. ======================================= >man 3Depict says: 3depict - program to do something >'s/3depict - program to do something/3Depict - 3D point cloud visualization and >analysis/' - Whoops. Fixed. ======================================= >However, the icon "3Depict" referenced in the .desktop file is missing. It >should be added to the package. -- Fixed >Finally, the spec file should get a short comment above Patch0 describing the >purpose of the patch. -- Done -- 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@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review