Bug 1111294 - Review Request: engrid - Mesh generation tool
Summary: Review Request: engrid - Mesh generation tool
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Dominik 'Rathann' Mierzejewski
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2014-06-19 16:18 UTC by Sandro Mani
Modified: 2017-02-07 04:33 UTC (History)
2 users (show)

Fixed In Version: engrid-1.4.0-3.gite6d55f5.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2014-08-23 01:57:06 UTC
Type: ---
dominik: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)
epel6 patch (1.21 KB, patch)
2014-06-24 13:29 UTC, Dave Love
no flags Details | Diff

Description Sandro Mani 2014-06-19 16:18:59 UTC
Spec URL: http://smani.fedorapeople.org/review/engrid.spec
SRPM URL: http://smani.fedorapeople.org/review/engrid-1.4.0-1.gite6d55f5.fc21.src.rpm
Description: Mesh generation tool
Fedora Account System Username: smani

Note: packaging git snapshot since official 1.4.0 release does not have VTK 6 support.

Comment 1 Dave Love 2014-06-24 13:29:24 UTC
Created attachment 911739 [details]
epel6 patch

I built it on EPEL6 with the attached patch.

Comment 2 Sandro Mani 2014-06-24 22:53:24 UTC

Comment 3 Dominik 'Rathann' Mierzejewski 2014-07-01 08:53:58 UTC
Taking review.

Comment 4 Sandro Mani 2014-07-22 09:04:47 UTC
Hi Dominik, any chance of doing the review? Thanks.

Comment 5 Dominik 'Rathann' Mierzejewski 2014-07-30 00:07:39 UTC
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...

Comment 6 Dominik 'Rathann' Mierzejewski 2014-07-30 00:25:57 UTC

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
   LIBS            += -L$(VTKLIBDIR)
-  LIBS            += -L./netgen_svn -lng
   LIBS            += -L./libengrid -lengrid
+  PKGCONFIG       += netgen-mesher
   brlcad {

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@GLIBC_2.2.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:

Please ask upstream to clarify the licence on those.

Comment 7 Sandro Mani 2014-07-30 23:21:14 UTC
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

* Wed Jul 30 2014 Sandro Mani <manisandro@gmail.com> - 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

Comment 8 Dominik 'Rathann' Mierzejewski 2014-08-06 14:45:50 UTC
$ 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.

Comment 9 Sandro Mani 2014-08-06 21:58:58 UTC
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

Comment 10 Gwyn Ciesla 2014-08-07 12:17:54 UTC
Git done (by process-git-requests).

Added f21.

Comment 11 Fedora Update System 2014-08-10 14:51:45 UTC
engrid-1.4.0-3.gite6d55f5.fc20 has been submitted as an update for Fedora 20.

Comment 12 Fedora Update System 2014-08-15 02:50:28 UTC
engrid-1.4.0-3.gite6d55f5.fc20 has been pushed to the Fedora 20 testing repository.

Comment 13 Fedora Update System 2014-08-23 01:57:06 UTC
engrid-1.4.0-3.gite6d55f5.fc20 has been pushed to the Fedora 20 stable repository.

Note You need to log in before you can comment on or make changes to this bug.