Bug 1111294

Created attachment 911739 [details]
epel6 patch

I built it on EPEL6 with the attached patch.

Thanks!

Taking review.

Hi Dominik, any chance of doing the review? Thanks.

Sorry, I was quite busy lately. Some comments while fedora-review is processing:

# Rename licence file...
mv licence.txt license.txt

Why? The guidelines say the spec file must be written in American English, but that doesn't mean the filenames must be, too.

# Desktop file, icon
echo "Icon=engrid.png" >> engrid.desktop
sed -i 's/Education;/Science;/' engrid.desktop

The two lines above can be done with appropriate options in desktop-file-install invocation, please use them (--set-icon and --add-category/--remove-category).

Also, the Icon= option should be set to engrid, not engrid.png, as required by desktop file guidelines. Please also try adding appdata, as the guideline has been approved recently (http://people.freedesktop.org/~hughsient/appdata/),

Out of curiosity, are there any consumers of that shared library apart from this package? I don't see a -devel subpackage...

Additionally:

What's the origin of Source1: engrid.png?
I can see that the included setup_fedora.bash script uses 
src/libengrid/resources/icons/G.png, so why not do the same instead of shipping another copy in the SRPM?

The engrid_build.patch contains whitespace-only change, which should be dropped to improve readability, namely this part (changes to src/engrid.pro.app):
@ -46,11 +46,12 @@ win32-msvc* {
     LIBS        += ../../netCDF/lib/netcdfcxx.lib
   }
 } else {
-  QMAKE_CXXFLAGS += -Wno-deprecated -g
+  CONFIG          += link_pkgconfig
+  QMAKE_CXXFLAGS  += -Wno-deprecated -g
   INCLUDEPATH     += $(VTKINCDIR)
   LIBS            += -L$(VTKLIBDIR)
-  LIBS            += -L./netgen_svn -lng
   LIBS            += -L./libengrid -lengrid
+  PKGCONFIG       += netgen-mesher
   brlcad {
     INCLUDEPATH += $(BRLCADINCDIR)
     INCLUDEPATH += $(BRLCADINCDIR)/openNURBS

You can skip adjusting formatting of the QMAKE_CXXFLAGS line.

rpmlint issues:
engrid.x86_64: W: incoherent-version-in-changelog 1.4-1 ['1.4.0-1.gite6d55f5.fc20', '1.4.0-1.gite6d55f5']

-> easyfix

engrid.x86_64: W: shared-lib-calls-exit /usr/lib64/libengrid.so.1.0.0 exit.5

-> please report upstream

engrid.x86_64: W: undefined-non-weak-symbol /usr/lib64/libengrid.so.1.0.0 nc_put_vara_short
[...]
engrid.x86_64: W: undefined-non-weak-symbol /usr/lib64/libengrid.so.1.0.0 nc_inq_var
engrid.x86_64: W: undefined-non-weak-symbol /usr/lib64/libengrid.so.1.0.0 typeinfo for vtkInteractorStyle
[...]

-> looks like missing libraries during linking stage

engrid.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libengrid.so.1.0.0 /lib64/libQtNetwork.so.4

-> drop from link flags

Source checksums
----------------
https://github.com/enGits/engrid/tarball/e6d55f564c20a8d13bee6bba6280a32320f1bde2/enGits-engrid-1.4.0-264-ge6d55f5.tar.gz :
  CHECKSUM(SHA256) this package     : 27119148fbcefe8654c213bdeb503bec323bd5a72181dd17ae8035492e80cc26
  CHECKSUM(SHA256) upstream package : 27119148fbcefe8654c213bdeb503bec323bd5a72181dd17ae8035492e80cc26

Licence seems to be ok, but there are some files without licence information:
src/dialoglineedit/*

Please ask upstream to clarify the licence on those.

Thank you for the careful review.

Concerning the icon, the reason I include it as a separate source is because the original icon is not a standard sized (46x51). I suppose I could BR imagemagick and resize the image in %prep, I feel it more robust to include it as a separate source but am fine with either method.

Concerning other users of the shared library: none that I know of for sure at the moment, OpenFOAM might be one. But in any case I'd rather keep the upstream build scripts as far as possible.

Concerning the shared-lib-calls-exit and license files, upstream report is here: https://github.com/enGits/engrid/issues/47


Spec URL: http://smani.fedorapeople.org/review/engrid.spec
SRPM URL: http://smani.fedorapeople.org/review/engrid-1.4.0-2.gite6d55f5.fc21.src.rpm

%changelog
* Wed Jul 30 2014 Sandro Mani <manisandro> - 1.4.0-2.gite6d55f5
- Don't rename license file
- Use desktop-file-install to set icon and fix category
- Add doc subpackage
- Add blender subpackage
- Update build patch to also link against vtk libraries, and not QtNetwork

$ rpmlint engrid-debuginfo
engrid-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/enGits-engrid-e6d55f5/src/libengrid/egvtkinteractorstyle.h
engrid-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/enGits-engrid-e6d55f5/src/libengrid/egvtkinteractorstyle.cpp
engrid-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/enGits-engrid-e6d55f5/src/libengrid/createvolumemesh.cpp

Please chmod -x these files in %prep.

Other highlighted rpmlint issues are gone, great.

I noticed that there's a manpage (engrid.1) in debian/ subdirectory, please install it, too.

You can fix these issues upon package import, as they're pretty minor.

This package is APPROVED.

Thanks a lot, issues will be fixed when importing.

New Package SCM Request
=======================
Package Name: engrid
Short Description: Mesh generation tool
Owners: smani
Branches: f20
InitialCC:

Git done (by process-git-requests).

Added f21.

engrid-1.4.0-3.gite6d55f5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/engrid-1.4.0-3.gite6d55f5.fc20

engrid-1.4.0-3.gite6d55f5.fc20 has been pushed to the Fedora 20 testing repository.

engrid-1.4.0-3.gite6d55f5.fc20 has been pushed to the Fedora 20 stable repository.