Bug 1758626 - Review Request: octave-iso2mesh - A 3D surface and volumetric mesh generator for MATLAB/Octave
Summary: Review Request: octave-iso2mesh - A 3D surface and volumetric mesh generator ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-neuro, NeuroFedora
TreeView+ depends on / blocked
 
Reported: 2019-10-04 16:03 UTC by Qianqian Fang
Modified: 2019-10-26 17:25 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-10-11 22:14:33 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Qianqian Fang 2019-10-04 16:03:35 UTC
Spec URL: https://github.com/fangq/fedorapkg/blob/iso2mesh/octave-iso2mesh.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/octave-iso2mesh-1.9.1-1.fc30.src.rpm

Description: 

Iso2Mesh is a MATLAB/Octave-based mesh generation toolbox,
designed for easy creation of high quality surface and 
tetrahedral meshes from 3D volumetric images. It contains 
a rich set of mesh processing scripts/programs, working 
either independently or interacting with external free 
meshing utilities. Iso2Mesh toolbox can directly convert
a 3D image stack, including binary, segmented or gray-scale 
images such as MRI or CT scans, into quality volumetric 
meshes. This makes it particularly suitable for multi-modality 
medical imaging data analysis and multi-physics modeling.
Above all, iso2mesh is open-source. You can download it for 
free. You are also allowed to extend the toolbox for your
own research and share with other users. Iso2Mesh is 
cross-platform and is compatible with both MATLAB and GNU Octave 
(a free MATLAB clone).

Fedora Account System Username: fangq

Comment 1 Qianqian Fang 2019-10-04 16:10:32 UTC
rpmlint output for srpm:
------------------------------------------------------------------------------
$rpmlint /home/fangq/rpmbuild/SRPMS/octave-iso2mesh-1.9.1-1.fc30.src.rpm
octave-iso2mesh.src: W: spelling-error Summary(en_US) volumetric -> cliometric
octave-iso2mesh.src: W: spelling-error %description -l en_US volumetric -> cliometric
octave-iso2mesh.src: W: spelling-error %description -l en_US modality -> morality, mortality
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
------------------------------------------------------------------------------


rpmlint output for rpm:
------------------------------------------------------------------------------
rpmlint /home/fangq/rpmbuild/RPMS/noarch/octave-iso2mesh-1.9.1-1.fc30.noarch.rpm
octave-iso2mesh.noarch: W: spelling-error Summary(en_US) volumetric -> cliometric
octave-iso2mesh.noarch: W: spelling-error %description -l en_US volumetric -> cliometric
octave-iso2mesh.noarch: W: spelling-error %description -l en_US modality -> morality, mortality
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalmesh
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalpoly
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsimp2
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsurf
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/cork
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/jmeshlib
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix
octave-iso2mesh.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/octave/packages/iso2mesh-1.9.1/bin/tetgen1.5
octave-iso2mesh.noarch: W: dangerous-command-in-%preun mv
1 packages and 0 specfiles checked; 8 errors, 4 warnings.
------------------------------------------------------------------------------

Comment 2 Qianqian Fang 2019-10-04 16:21:52 UTC
This packaging request can be found in

https://pagure.io/neuro-sig/NeuroFedora/issue/146


This package is configured as a "semi-noarch" package according to the comments in 

http://bugzilla.redhat.com/show_bug.cgi?id=1385257#c5
https://pagure.io/koji/issue/19

> "Sometimes the content in a package can be run anywhere, but some underlying dependency is only 
> available on a subset of arches"

this suites this package quite well - most scripts can function without the binary dependencies in the bin/ folder, thus deserves the noarch designation.

If I set the package BuildArch to anything other than noarch, rpmbuild will fail because something in macros.octave does not seem to support arch dependent files, this seems to be related to this bug

https://bugzilla.redhat.com/show_bug.cgi?id=1413450

Comment 3 Ankur Sinha (FranciscoD) 2019-10-05 17:08:00 UTC
I'm looking at the spec now. Each spec should only build one software. Here, the spec is building 4 different tools? (tetgen is already in Fedora, by the way, so it must be used as a BuildRequires or Requires as required: https://apps.fedoraproject.org/packages/tetgen)

This spec should be limited to iso2mesh, and if the others are dependencies, they must be packaged separately. (I know this means more reviews, but the point is that each tool must be installable on its own also.)

Comment 4 Qianqian Fang 2019-10-05 20:18:28 UTC
@Ankur, thanks for the review. see my replies below.

> Here, the spec is building 4 different tools?

No. the spec is for building iso2mesh only - the tools you saw are used internally by iso2mesh and are not meant for shared by other packages (or directly called by users).

several of these utilities were modified/customized version specifically for iso2mesh (such as the CGAL tools and cork); other utilities were included in iso2mesh because the meshing result is sensitive to these utilities versions, if I let user to call system-installed tools, such as tetgen, the meshing output will not be reproducible across computers.

I also want to mention that these tools are only used by about 5-10% of the functions in iso2mesh. They are dependencies, but weak dependencies.


> tetgen is already in Fedora, by the way, so it must be used as a BuildRequires or Requires as required

I can remove tetgen from iso2mesh package and add a link in its place to use the system installed tetgen, but the risk is that iso2mesh output will not be deterministic.

> if the others are dependencies, they must be packaged separately. (I know this means more reviews, but the point is that each tool must be installable on its own also.)

again, my purpose of including these utilities inside iso2mesh is to ensure that the meshing output is reproducible (many utilities generate very different output from version to version, such as CGAL and tetgen); also such inclusion was permitted under their respective open-source licenses. 

It is fine if someone want to package these tools as separate packages, but 1) the tools that iso2mesh used are either modified, or older versions, 2) I am not the upstream author of these utilities , for example, I am not the author of cork (upstream repo: https://github.com/gilbo/cork), iso2mesh uses a modified version in my fork (https://github.com/fangq/cork). So, if we package my fork as the official package, I am not sure how to accommodate the upstream development in the future.


I do want to mention that iso2mesh currently contains a copy of jsonlab and jnifti (used by a couple of file io functions). Because "pkg load" in octave can't check/load dependencies, AFAIK, so I think including a copy of these toolboxes in iso2mesh is a more portable approach, despite some redundancy.

Comment 5 Ankur Sinha (FranciscoD) 2019-10-06 11:30:50 UTC
(In reply to Qianqian Fang from comment #4)
> @Ankur, thanks for the review. see my replies below.
> 
> > Here, the spec is building 4 different tools?
> 
> No. the spec is for building iso2mesh only - the tools you saw are used
> internally by iso2mesh and are not meant for shared by other packages (or
> directly called by users).

OK, so they're "private". That's fine then. Could we add comments to the spec
to say so please, just for the benefit of other package maintainers who would
probably end up asking the same questions as me otherwise? :)

> 
> several of these utilities were modified/customized version specifically for
> iso2mesh (such as the CGAL tools and cork); other utilities were included in
> iso2mesh because the meshing result is sensitive to these utilities
> versions, if I let user to call system-installed tools, such as tetgen, the
> meshing output will not be reproducible across computers.

OK, but we can specify versioned dependencies, would that work? Of course, it depends on whether or not the Fedora packages are carrying the version you need. Fedora generally carries the latest versions, though---and that in a way helps us remind developers to update their code bases too:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependencies
https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/

Bundling is no longer forbidden in Fedora, but if at all possible, it should be avoided:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

> 
> I also want to mention that these tools are only used by about 5-10% of the
> functions in iso2mesh. They are dependencies, but weak dependencies.

Sure, they can be specified as weak deps if needed also:
https://docs.fedoraproject.org/en-US/packaging-guidelines/WeakDependencies/

> 
> 
> > tetgen is already in Fedora, by the way, so it must be used as a BuildRequires or Requires as required
> 
> I can remove tetgen from iso2mesh package and add a link in its place to use
> the system installed tetgen, but the risk is that iso2mesh output will not
> be deterministic.

Is there a way of checking this maybe? Is there a testsuite, for example? The version of tetgen in Fedora is 1.5.x.

> 
> > if the others are dependencies, they must be packaged separately. (I know this means more reviews, but the point is that each tool must be installable on its own also.)
> 
> again, my purpose of including these utilities inside iso2mesh is to ensure
> that the meshing output is reproducible (many utilities generate very
> different output from version to version, such as CGAL and tetgen); also
> such inclusion was permitted under their respective open-source licenses. 

Sure, I understand that, but please see comments I've made before.

> 
> It is fine if someone want to package these tools as separate packages, but
> 1) the tools that iso2mesh used are either modified, or older versions, 2) I
> am not the upstream author of these utilities , for example, I am not the
> author of cork (upstream repo: https://github.com/gilbo/cork), iso2mesh uses
> a modified version in my fork (https://github.com/fangq/cork). So, if we
> package my fork as the official package, I am not sure how to accommodate
> the upstream development in the future.

I've looked at cork---upstream is inactive, has been for a while, and from the README, it really does not look like they'll actively maintain the project in the future either. I've built it using your patches, and I had to tweak the Makefile to get it to generate shared libraries also---static libraries are discouraged: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries

https://ankursinha.fedorapeople.org//cork/

If that will do, I can submit it for review?

> 
> 
> I do want to mention that iso2mesh currently contains a copy of jsonlab and
> jnifti (used by a couple of file io functions). Because "pkg load" in octave
> can't check/load dependencies, AFAIK, so I think including a copy of these
> toolboxes in iso2mesh is a more portable approach, despite some redundancy.

I'll have to check this up. Maybe we can speak to the Octave maintainer on the sci-tech mailing list to see what the best way of handling these are?
https://lists.fedoraproject.org/admin/lists/scitech.lists.fedoraproject.org/

Comment 6 Robert-André Mauchin 🐧 2019-10-06 21:40:16 UTC
Remove 

ExclusiveArch:  %{ix86} x86_64
BuildArch:      noarch

and:

%global _binaries_in_noarch_packages_terminate_build   0
%global debug_package %{nil}


 - Build errors:


CMake Error at CMakeLists.txt:5 (project):
  No CMAKE_CXX_COMPILER could be found.

  Tell CMake where to find the compiler by setting either the environment
  variable "CXX" or the CMake cache entry CMAKE_CXX_COMPILER to the full path
  to the compiler, or to the compiler name if it is in the PATH.


-- Configuring incomplete, errors occurred!

Add gcc-c++ as a BR

 - Build fails then:

-- Build files have been written to: /builddir/build/BUILD/iso2mesh-1.9.1/tools/tetgen
make -C tetgen --no-print-directory
g++ -O0 -c predicates.cxx
g++ -O3  -o tetgen tetgen.cxx predicates.o -lm
cp: cannot stat 'cgalmesh/mesh_3D_image': No such file or directory
make: *** [commons/Makefile_common.mk:71: copybin] Error 1

Digging around, I found:

-- NOTICE: The examples mesh_3D_image.cpp and mesh_3D_image_variable_size.cpp need CGAL_ImageIO to be configured with ZLIB support, and will not be compiled.

Thus BR zlib-devel

+ %octave_pkg_build
/var/tmp/rpm-tmp.SoTVjv: line 38: fg: no job control

 You need to:

BuildRequires:  octave-devel

 - Use set_build_flags to get debuginfos:

%build
%set_build_flags

Comment 7 Qianqian Fang 2019-10-07 04:25:19 UTC
@Robert-Andre, thanks for the feedback. I updated the spec file with the changes you suggested

https://github.com/fangq/fedorapkg/commit/8f85a65af6a745f8358833aedbbe6c5df7f36eb9

but now the package cannot be built any more on my fc30 vm. I am getting the same error I got previously

https://pagure.io/neuro-sig/NeuroFedora/issue/146#comment-602139

which was previously resolved by forcing BuildArch to noarch.

https://pagure.io/neuro-sig/NeuroFedora/issue/146#comment-602191

are you able to reproduce this error? if not, can you let me know what is your testing environment? thanks

Comment 8 Ankur Sinha (FranciscoD) 2019-10-07 09:27:19 UTC
As I said in the other ticket, please test the build against rawhide instead of f30. (fedpkg --release f32 ... should do it)

I ran a test build, and if succeeds for all architectures other than arm:

https://koji.fedoraproject.org/koji/taskinfo?taskID=38115425

Comment 9 Robert-André Mauchin 🐧 2019-10-07 13:42:15 UTC
Probably related to bug https://bugzilla.redhat.com/show_bug.cgi?id=1733898 and https://src.fedoraproject.org/rpms/octave/c/c32f37ff6c776af9486f4f128b2a87e1055e420a?branch=master

So it should work on F31 and F32 but fail below?

Comment 10 Robert-André Mauchin 🐧 2019-10-07 14:55:12 UTC
It should work on F30 by redefining octave_tar_suffix after octave_pkg_build:

%octave_pkg_build
%global octave_tar_suffix any-none

Comment 11 Qianqian Fang 2019-10-07 17:26:00 UTC
@FranciscoD and @eclipseo,

my spec file is updated according to your feedback

https://github.com/fangq/fedorapkg/commit/72e3319103a36a1d9aa7f99052fb09646ca19863


@FranciscoD: the error on arm was caused by exhausted memory when compiling cgal tools. newer versions of CGAL library seems to require a min memory to build. I previously saw this error on the fc30 vm if I allocate 2GB memory, but it goes away when setting a higher memory. I can either list arm under ExcludeArch or find a flag to increase the memory (if possible).



Regarding cork, if it helps, the merged patch to the upstream inactive repo can be found in this pull request

https://github.com/gilbo/cork/pull/32

For your cork package spec file, it looks good to me. 

let me know if you want me to drop cork/tetgen from my source list and add a link to these packages (or it is ok for my to keep a private copy).



also, I am wondering if there is any policy against using exe packer (such as upx) to compress binary files during %build? the generated cgal binaries are quite bulky using the newer libraries, if I call "upx -9 bin/cgal*", I can drop the bin file sizes by 10x (also make file faster to load). do you want me to add that compression command (and also add upx in BuildRequires)?

Comment 12 Robert-André Mauchin 🐧 2019-10-07 18:40:03 UTC
 - Problem installing the package:

DEBUG util.py:593:  Error: 
DEBUG util.py:593:   Problem: conflicting requests
DEBUG util.py:593:    - nothing provides CGAL needed by octave-iso2mesh-1.9.1-1.fc32.x86_64
DEBUG util.py:595:  (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)

CGAL.spec 5.0 has now this comment:

# CGAL-5.0 is now a header-only library, with dependencies. It no
# longer has any binary to build, but cannot be noarch because of
# arch-specific dependencies

It doesn't provide a CGAL binary anymore.


 - Own these directories:

%{octpkgdir}/doc/
%{octpkgdir}/bin/

[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/octave/packages/iso2mesh-1.9.1/bin,
     /usr/share/octave/packages/iso2mesh-1.9.1/doc

 - Large documentation must go in a -doc subpackage. Large could be  size
  (~1MB) or number of files.
  Note: Documentation size is 1382400 bytes in 31 files.
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_documentation

Make a separate doc noarch subpackage containing the sample/ dir


 - [!]: Rpath absent or only used for internal libs.
     Note: See rpmlint output

Remove Rpath from bin/meshfix

octave-iso2mesh.x86_64: E: binary-or-shlib-defines-rpath /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix ['/builddir/build/BUILD/iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/lib']

I think the best path to do this is to patch out:

link_directories(
    ${LINK_DIRECTORIES}
    ${CMAKE_CURRENT_SOURCE_DIR}/contrib/JMeshLib/lib
)

from meshfix CMakeLists.txt

#meshfix-remove-rpath.patch
diff -up a/CMakeLists.txt.orig b/CMakeLists.txt
--- a/CMakeLists.txt.orig	2019-10-01 18:39:42.000000000 +0200
+++ b/CMakeLists.txt	2019-10-07 20:06:44.072901788 +0200
@@ -8,10 +8,6 @@ include_directories(
     contrib/OpenNL3.2.1/src
     contrib/jrs_predicates
 )
-link_directories(
-    ${LINK_DIRECTORIES}
-    ${CMAKE_CURRENT_SOURCE_DIR}/contrib/JMeshLib/lib
-)
 file(GLOB meshfix_h include/*.h)
 set(meshfix_src
     contrib/jrs_predicates/jrs_predicates.c

Patch0:        meshfix-remove-rpath.patch

[…]

%prep
%setup -q -b 1 -n %{octpkg}-%{version}
%setup -q -T -D -b 2 -n meshfix-1.2.1
%patch0 -p1
%setup -q -T -D -b 3 -n %{octpkg}-%{version}

(See http://ftp.rpm.org/max-rpm/s1-rpm-inside-macros.html for %setup flags, -T -D is necessary to avoid multiple unzipping of Source0)





Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/
- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 1382400 bytes in 31 files.
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_documentation


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[!]: Rpath absent or only used for internal libs.
     Note: See rpmlint output
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "GPL (v3 or later)", "*No copyright* Q
     Public License (v1.0) GNU General Public License GNU Lesser General
     Public License", "BSD (unspecified)", "*No copyright* BSD
     (unspecified)", "*No copyright* GNU Lesser General Public License",
     "AGPL (v3 or later)", "GNU Lesser General Public License (v3 or
     later)", "GPL (v2 or later)", "BSD 3-clause "New" or "Revised"
     License", "Expat License". 306 files have unknown license. Detailed
     output of licensecheck in /home/bob/packaging/review/octave-
     iso2mesh/review-octave-iso2mesh/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/octave/packages/iso2mesh-1.9.1/bin,
     /usr/share/octave/packages/iso2mesh-1.9.1/doc
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Uses parallel make %{?_smp_mflags} macro.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
[!]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 10352640 bytes in /usr/share
     octave-iso2mesh-1.9.1-1.fc32.x86_64.rpm:10352640
     See:
     https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Package_Review_Guidelines
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: octave-iso2mesh-1.9.1-1.fc32.x86_64.rpm
          octave-iso2mesh-debuginfo-1.9.1-1.fc32.x86_64.rpm
          octave-iso2mesh-debugsource-1.9.1-1.fc32.x86_64.rpm
          octave-iso2mesh-1.9.1-1.fc32.src.rpm
octave-iso2mesh.x86_64: W: spelling-error Summary(en_US) volumetric -> cliometric
octave-iso2mesh.x86_64: W: spelling-error %description -l en_US volumetric -> cliometric
octave-iso2mesh.x86_64: W: spelling-error %description -l en_US modality -> morality, mortality
octave-iso2mesh.x86_64: E: binary-or-shlib-defines-rpath /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix ['/builddir/build/BUILD/iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/lib']

octave-iso2mesh.x86_64: W: dangerous-command-in-%preun mv
octave-iso2mesh.src: W: spelling-error Summary(en_US) volumetric -> cliometric
octave-iso2mesh.src: W: spelling-error %description -l en_US volumetric -> cliometric
octave-iso2mesh.src: W: spelling-error %description -l en_US modality -> morality, mortality
4 packages and 0 specfiles checked; 1 errors, 7 warnings.

Comment 13 Qianqian Fang 2019-10-08 15:55:17 UTC
@eclipseo, I made the following changes according to your above comments:

changeset 1: https://github.com/fangq/fedorapkg/commit/c6574e3ec3b41240fb4f3aecd35f19889932a802

this addresses the CGAL header-only mode dependency, meshfix patch that you created, -D -T flags in setup, and ownership of doc/ and bin/

changeset 2: https://github.com/fangq/fedorapkg/commit/bbd84a88cdad40dfd82b7634c27c5943cd9fc9e2

split the samples into a separate sub-package iso2mesh-demos.


the current spec file can be found at

https://github.com/fangq/fedorapkg/tree/iso2mesh


the updated spec file builds fine on f30. I did not test it on rawhide (not sure how to do it) regarding the CGAL-5.0 dependency issue.

let me know if your preference on tetgen1.5 - whether to use it as dependency and create link in my bin/ folder, or use the embedded version for stability.

Comment 14 Robert-André Mauchin 🐧 2019-10-08 17:53:28 UTC
 - Typo: #else → %else

 - Make the new subpackage noarch

Package approved, please fix the aforementioned issue before import.

Comment 15 Qianqian Fang 2019-10-08 19:02:43 UTC
thanks, the mentioned issues are fixed

https://github.com/fangq/fedorapkg/commit/94aa38ef29203635d7f62117f1b660e3e50610d8
https://github.com/fangq/fedorapkg/blob/iso2mesh/octave-iso2mesh.spec


I have two minor questions:

1. I changed the below line

BuildArch:      i686 x86_64 aarch64 ppc64le s390x

to

ExcludeArch:    armv7hl


is this ok? or explicitly setting BuildArch is preferred?

2. Installing the main package, and I noticed the below files/folders, I think these are related to the executables under the the bin folder. are these files desirable? if not, how do I get rid of them?

[fangq@localhost fedorapkg]$ rpm -ql octave-iso2mesh
/usr/lib/.build-id
/usr/lib/.build-id/4d
/usr/lib/.build-id/4d/cba156e0259c317936d0c1ade115ec4d3aba4e
/usr/lib/.build-id/5c
/usr/lib/.build-id/5c/82b58fba63341b50ecb9979c58f6b9d4743d34
/usr/lib/.build-id/88
/usr/lib/.build-id/88/d267d7db7b09c94bb0f24ce5299be052f2a8b4
/usr/lib/.build-id/97
/usr/lib/.build-id/97/27a7c7d6d6a38355950e0f5690fa552d557cda
/usr/lib/.build-id/d8
/usr/lib/.build-id/d8/7848c94ba78111893400d8923783cca5718277
/usr/lib/.build-id/d9
/usr/lib/.build-id/d9/d9f70ad320e7b8e1a2e3b50fc4a6f60c20e286
/usr/lib/.build-id/e7
/usr/lib/.build-id/e7/9bc8e41ef0ab97dc306d128d04de0d0cf97f1a
/usr/lib/.build-id/ef
/usr/lib/.build-id/ef/f3e81fa24fff8c68dd79394962ac7f8c1fc8fe

Comment 16 Gwyn Ciesla 2019-10-08 19:16:15 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/octave-iso2mesh

Comment 17 Qianqian Fang 2019-10-08 22:19:13 UTC
just want to report a problem when building the package (but I have found a solution).

using the above spec file, when I ran fedpkg build, the job failed initially - I tried both rawhide and f30, both failed, here are the logs

https://koji.fedoraproject.org/koji/taskinfo?taskID=38151144
https://koji.fedoraproject.org/koji/taskinfo?taskID=38152397

looking at the build.log file, both seemed to returned an error when cgal is linking the first target, although no explicit error message was printed. I realized that this may be caused by the large memory footprint needed for compiling/linking cgal binaries (which crashed the arm target previously in Ankur's test). But when I ran the spec file in my f30 vm, it did work.

then I realized that this might be a problem due to %make_build, which is basically running multiple make in parallel - because compiling cgal codes needs a lot of memory, running multiple makes might have exhausted the vm's memory and caused the failure.

so, I reverted the %make_build line back to "make" to use sequential compilation, 

https://src.fedoraproject.org/rpms/octave-iso2mesh/c/7971faf3a3dcf4df956474630566a0e150196008?branch=master

this time, the build succeeded

https://koji.fedoraproject.org/koji/buildinfo?buildID=1397854


so, I will leave the make there and added a comment above it.

https://src.fedoraproject.org/rpms/octave-iso2mesh/c/3135659947bea0410ed4a3dd0cc56e3798549094?branch=master


--------------------------------------------

PS: hold on a second, it looks like f31 and f32 worked, but f29 and f30 failed

https://koji.fedoraproject.org/koji/packageinfo?packageID=30110

reading the build.log files, I see two causes

1. on f29, the error appears to be package "gcc-g++" was not available to install, see

https://kojipkgs.fedoraproject.org//work/tasks/4014/38154014/root.log

DEBUG util.py:593:  No matching package to install: 'gcc-g++'


2. on f30, even though the task summary says "see root.log" for more info, but the actual error was in build.log, where the cgal compilation failed as it did initially using %make_build. I am not sure any more.


suggestions are appreciated

Comment 18 Qianqian Fang 2019-10-08 23:00:13 UTC
ok, fixing the typo (gcc-g++ -> gcc-c++) fixed the f29 and f30 build errors. perhaps this was also the cause for the initial error. I am going to revert the %make_build flag and see what happens.

I tried to rebuild f32/f31 packages using "fedpkg build --skip-nvr-check" but both failed and returned an error "Build already exists". How can I force rebuilding? the spec files were patched.

Comment 19 Fedora Update System 2019-10-08 23:18:21 UTC
FEDORA-2019-79f3a89b27 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-79f3a89b27

Comment 20 Fedora Update System 2019-10-09 17:40:01 UTC
octave-iso2mesh-1.9.1-1.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-79f3a89b27

Comment 21 Laurent Rineau 2019-10-10 15:10:02 UTC
I love to see iso2mesh packaged in Fedora, but there have been error in this review. Even rpmlint can see them:

[lrineau@bonnard]~% rpm -q octave-iso2mesh; rpmlint octave-iso2mesh
octave-iso2mesh-1.9.1-1.fc30.x86_64
octave-iso2mesh.x86_64: E: devel-dependency gmp-devel
octave-iso2mesh.x86_64: W: spelling-error Summary(en_US) volumetric -> cliometric
octave-iso2mesh.x86_64: W: spelling-error %description -l en_US volumetric -> cliometric
octave-iso2mesh.x86_64: W: spelling-error %description -l en_US modality -> morality, mortality
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalmesh
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalpoly
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsimp2
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsurf
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/cork
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/jmeshlib
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix
octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/iso2mesh-1.9.1/bin/tetgen1.5
octave-iso2mesh.x86_64: W: dangerous-command-in-%preun mv
1 packages and 0 specfiles checked; 9 errors, 4 warnings.


Plus: tetgen is already packaged in Fedora.

Should I fill new bugs for that?

Qianqian Fang: I am willing to help with those issues, if you agree.

Comment 22 Qianqian Fang 2019-10-10 15:46:18 UTC
@Laurent, thanks for chiming in. 

I just tested octave-iso2mesh on f30 in updates-testing repo, the installation and execution was fine.

regarding your comments

1. the arch-dependent-file-in-usr-share will be triggered as long as the octave package contains a mex file (like in octave-zmat and octave-mcxlab that I just created/pushed), or in this case, bundled executables. I currently don't see other workaround for removing this error; the earlier two reviewers, Ankur Sinha and Robert-André Mauchin seemed to be ok with that too.

2. regarding tetgen, please see Comment 4 and Comment 5 for the discussions. These are internal tools and are not intend to be called outside of iso2mesh. I prefer to bundle these tools as much as possible to allow a mesh reproduced across platforms, and does not reply on the versions of a tool installed on a user's system (wish I could do the same for cgal, but it is too big to be bundled internally). Many of my other tools (such as mmc: http://mcx.space/#mmc) rely on a reproducible mesh to run examples correctly. Also, bundling this utility is allowed by their respective licenses.

if you are ok with these, I am going to push updates to and f29/f31. f30/rawhide already contains the built package.

Comment 23 Qianqian Fang 2019-10-10 15:55:14 UTC
@Laurent,

regarding my question in the cgal-discuss mailing list, it turned out that the large executable sizes were results of debug info in the generated executables. Fortunately, rpm automatically stripped these big executables files and export the extracted the debug info into a separate package called octave-iso2mesh-debuginfo...rpm and octave-iso2mesh-debugsource-...rpm; the final executables after stripping are not that big, so I will leave the building script as is for now.

Comment 24 Ankur Sinha (FranciscoD) 2019-10-10 16:20:13 UTC
(In reply to Laurent Rineau from comment #21)
> I love to see iso2mesh packaged in Fedora, but there have been error in this
> review. Even rpmlint can see them:
> 
> [lrineau@bonnard]~% rpm -q octave-iso2mesh; rpmlint octave-iso2mesh
> octave-iso2mesh-1.9.1-1.fc30.x86_64
> octave-iso2mesh.x86_64: E: devel-dependency gmp-devel
> octave-iso2mesh.x86_64: W: spelling-error Summary(en_US) volumetric ->
> cliometric
> octave-iso2mesh.x86_64: W: spelling-error %description -l en_US volumetric
> -> cliometric
> octave-iso2mesh.x86_64: W: spelling-error %description -l en_US modality ->
> morality, mortality
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalmesh
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalpoly
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsimp2
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsurf
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cork
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/jmeshlib
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/tetgen1.5
> octave-iso2mesh.x86_64: W: dangerous-command-in-%preun mv
> 1 packages and 0 specfiles checked; 9 errors, 4 warnings.
> 
> 
> Plus: tetgen is already packaged in Fedora.
> 
> Should I fill new bugs for that?
> 
> Qianqian Fang: I am willing to help with those issues, if you agree.

This is where bundling gets ugly.  :(

So, bundling tetgen etc if fine if necessary, but the binaries should not be in /usr/share. That violates the FHS as rpmlint points out. Can they be moved to an arch specific directory, preferably %{_libdir}/isomesh/... (so they remain private and do not clash with the system tetgen libraries + binaries), which will expand to /usr/lib/isomesh/ and /usr/lib64/isomesh according to the architecture of the machine?

Please also specify that these are bundled by iso2mesh.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling refers to libraries, so maybe we'll need to speak to the FPC about bundling binaries.

Provides: bundled(tetgen) ...

(In reply to Qianqian Fang from comment #22)
> @Laurent, thanks for chiming in. 
> 
> I just tested octave-iso2mesh on f30 in updates-testing repo, the
> installation and execution was fine.
> 
> regarding your comments
> 
> 1. the arch-dependent-file-in-usr-share will be triggered as long as the
> octave package contains a mex file (like in octave-zmat and octave-mcxlab
> that I just created/pushed), or in this case, bundled executables. I
> currently don't see other workaround for removing this error; the earlier
> two reviewers, Ankur Sinha and Robert-André Mauchin seemed to be ok with
> that too.

Please see above.

> 
> 2. regarding tetgen, please see Comment 4 and Comment 5 for the discussions.
> These are internal tools and are not intend to be called outside of
> iso2mesh. I prefer to bundle these tools as much as possible to allow a mesh
> reproduced across platforms, and does not reply on the versions of a tool
> installed on a user's system (wish I could do the same for cgal, but it is
> too big to be bundled internally). Many of my other tools (such as mmc:
> http://mcx.space/#mmc) rely on a reproducible mesh to run examples
> correctly. Also, bundling this utility is allowed by their respective
> licenses.

I understand this, but as I did say in my comments---do we have an idea of how sensitive iso2mesh is to tetgen? What is required of tetgen to ensure reproducibility? If tetgen on different platforms yields different results, does that mean that all tools that rely on tetgen are always affected and will always bundle it?

Comment 25 Laurent Rineau 2019-10-10 16:22:43 UTC
(In reply to Laurent Rineau from comment #21)
> I love to see iso2mesh packaged in Fedora, but there have been error in this
> review. Even rpmlint can see them:
> 
> [lrineau@bonnard]~% rpm -q octave-iso2mesh; rpmlint octave-iso2mesh
> octave-iso2mesh-1.9.1-1.fc30.x86_64
> octave-iso2mesh.x86_64: E: devel-dependency gmp-devel

You should remove the dependency to 'gmp-devel'. The dependency to 'gmp' will be found automatically once the binaries are correctly installed according to Fedora's rules.

> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalmesh
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalpoly
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsimp2
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsurf
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cork
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/jmeshlib
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/tetgen1.5

All those "tools" should be in /usr/libexec/iso2mesh/ instead of /usr/share/octave/packages/iso2mesh-1.9.1/bin/. The mex files should be able to run the tools. If you want to be lazy, just move the tools into /usr/libexec/iso2mesh/ and package symbolic links in /usr/share/octave/packages/iso2mesh-1.9.1/bin/.


(In reply to Qianqian Fang from comment #22)

> 2. regarding tetgen, please see Comment 4 and Comment 5 for the discussions.
> These are internal tools and are not intend to be called outside of
> iso2mesh. I prefer to bundle these tools as much as possible to allow a mesh
> reproduced across platforms, and does not reply on the versions of a tool
> installed on a user's system (wish I could do the same for cgal, but it is
> too big to be bundled internally). Many of my other tools (such as mmc:
> http://mcx.space/#mmc) rely on a reproducible mesh to run examples
> correctly. Also, bundling this utility is allowed by their respective
> licenses.

All right. The guidelines at https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling says 'SHOULD' and not 'MUST'. As for CGAL, as CGAL is a header-only library of C++ templates, you just have to be careful when the package is rebuilt.

Comment 26 Laurent Rineau 2019-10-10 16:25:52 UTC
And the License declaration of the package is wrong. As the package bundles other software, the licensing is complicated.


(In reply to Robert-André Mauchin from comment #12)

> [x]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses
>      found: "Unknown or generated", "GPL (v3 or later)", "*No copyright* Q
>      Public License (v1.0) GNU General Public License GNU Lesser General
>      Public License", "BSD (unspecified)", "*No copyright* BSD
>      (unspecified)", "*No copyright* GNU Lesser General Public License",
>      "AGPL (v3 or later)", "GNU Lesser General Public License (v3 or
>      later)", "GPL (v2 or later)", "BSD 3-clause "New" or "Revised"
>      License", "Expat License". 306 files have unknown license. Detailed
>      output of licensecheck in /home/bob/packaging/review/octave-
>      iso2mesh/review-octave-iso2mesh/licensecheck.txt

Here we can see that the license checker has detected a lot of licenses (including AGPLv3+ and GPLv2), so the package cannot be just GPLv3+.

Comment 27 Qianqian Fang 2019-10-10 19:02:51 UTC
> So, bundling tetgen etc if fine if necessary, but the binaries should not be in /usr/share. That violates the FHS as rpmlint points out. Can they be moved to an arch specific directory, preferably %{_libdir}/isomesh/... (so they remain private and do not clash with the system tetgen libraries + binaries), which will expand to /usr/lib/isomesh/ and /usr/lib64/isomesh according to the architecture of the machine?


ok, if I want to put those in %{_libdir}/isomesh/ and create symbolic links under iso2mesh/bin, do you mind showing me how to do it? my latest spec file can be found here, would be great if you can send me a pull request, thanks

https://github.com/fangq/fedorapkg/tree/iso2mesh


> Here we can see that the license checker has detected a lot of licenses (including AGPLv3+ and GPLv2), so the package cannot be just GPLv3+.

all the licenses included in this software are GPLv3+ compatible, except AGPLv3+, which is stricter, although it is also listed under GPL compatible licenses

https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses

which means I can declare all of them under the GPLv3+ license as a full package. If I have to distinguish GPLv3+ and AGPLv3+, please suggest how to do it. I want to make sure that AGPLv3+ only covers tetgen, but not other part of the software (which are covered under GPLv3+).

Comment 28 Robert-André Mauchin 🐧 2019-10-10 20:12:21 UTC
You're right, Tetgen is AGPLv3 amd JMeshLib is GPLv2, but I didn't understand it was into the resulting binary:


AGPL (v3 or later)
------------------
iso2mesh-1.9.1/tools/tetgen/LICENSE

GPL (v2 or later)
-----------------
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/binTree.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/clusterGraph.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/dijkstraGraph.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/edge.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/graph.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/heap.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/j_mesh.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/jmesh.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/jqsort.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/list.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/matrix.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/point.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/tin.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/triangle.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/include/vertex.h
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/JMESH/jmesh.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/checkAndRepair.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/edge.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/io.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/point.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/tin.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/triangle.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/MESH_STRUCTURE/vertex.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/binTree.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/clusterGraph.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/dijkstraGraph.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/graph.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/heap.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/jqsort.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/list.cpp
iso2mesh-1.9.1/tools/meshfix/contrib/JMeshLib/src/PRIMITIVES/matrix.cpp
iso2mesh-1.9.1/tools/meshfix/include/detectIntersections.h
iso2mesh-1.9.1/tools/meshfix/include/epsilonSampling.h
iso2mesh-1.9.1/tools/meshfix/include/exttrimesh.h
iso2mesh-1.9.1/tools/meshfix/include/holeFilling.h
iso2mesh-1.9.1/tools/meshfix/include/simplification.h
iso2mesh-1.9.1/tools/meshfix/include/sparseLSystem.h
iso2mesh-1.9.1/tools/meshfix/src/detectIntersections.cpp
iso2mesh-1.9.1/tools/meshfix/src/epsilonSampling.cpp
iso2mesh-1.9.1/tools/meshfix/src/holeFilling.cpp
iso2mesh-1.9.1/tools/meshfix/src/simplification.cpp
iso2mesh-1.9.1/tools/meshfix/src/smoothing.cpp
iso2mesh-1.9.1/tools/meshfix/src/sparseLSystem.cpp
iso2mesh-1.9.1/tools/meshfix/src/uniform.cpp

They should be added to the License: field 


> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalmesh
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalpoly
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsimp2
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cgalsurf
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/cork
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/jmeshlib
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/meshfix
> octave-iso2mesh.x86_64: E: arch-dependent-file-in-usr-share
> /usr/share/octave/packages/iso2mesh-1.9.1/bin/tetgen1.5

>All those "tools" should be in /usr/libexec/iso2mesh/ instead of 
>/usr/share/octave/packages/iso2mesh-1.9.1/bin/. The mex files should be 
>able to run the tools. If you want to be lazy, just move the tools into 
>/usr/libexec/iso2mesh/ and package symbolic links in /usr/share/octave
>/packages/iso2mesh-1.9.1/bin/.

I concir, these error did not show up in my rpmlint output as seen above. They shouldn't be in %_datadir, most likely in bindir or libexecdir.

Comment 29 Robert-André Mauchin 🐧 2019-10-10 20:33:56 UTC
(In reply to Qianqian Fang from comment #27)
> 
> > Here we can see that the license checker has detected a lot of licenses (including AGPLv3+ and GPLv2), so the package cannot be just GPLv3+.
> 
> all the licenses included in this software are GPLv3+ compatible, except
> AGPLv3+, which is stricter, although it is also listed under GPL compatible
> licenses
> 
> https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses
> 
> which means I can declare all of them under the GPLv3+ license as a full
> package. If I have to distinguish GPLv3+ and AGPLv3+, please suggest how to
> do it. I want to make sure that AGPLv3+ only covers tetgen, but not other
> part of the software (which are covered under GPLv3+).

You need to add a comment explaining the breakdown

# Main package: GPLv3+
# JMeshLib: GPLv2
# Tetgen: AGPLv3+ 
License: GPLv3+ and GPLv2 and AGPLv3+

Comment 30 Fedora Update System 2019-10-11 02:38:19 UTC
FEDORA-2019-33bd7a597c has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-33bd7a597c

Comment 31 Fedora Update System 2019-10-11 02:39:39 UTC
FEDORA-2019-fec9adc180 has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-fec9adc180

Comment 32 Laurent Rineau 2019-10-11 07:13:18 UTC
(In reply to Robert-André Mauchin from comment #29)
> You need to add a comment explaining the breakdown
> 
> # Main package: GPLv3+
> # JMeshLib: GPLv2
> # Tetgen: AGPLv3+ 
> License: GPLv3+ and GPLv2 and AGPLv3+

JMeshLib is GPLv2+.

Comment 33 Fedora Update System 2019-10-11 16:53:45 UTC
octave-iso2mesh-1.9.1-2.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-fec9adc180

Comment 34 Qianqian Fang 2019-10-11 22:14:33 UTC
@Ankur, @Robert-Andre and @Laurent, thank you all for the comments and help on creating this package.

the latest spec file can be found on the src git site: https://src.fedoraproject.org/rpms/octave-iso2mesh/blob/master/f/octave-iso2mesh.spec

In the -3 version of this initial package, all the above mentioned issues were addressed, including

- thanks for Rob's patch, the binaries are now moved to /usr/libexec, and some of the license issues found by @Laurent

- per early discussions on setting octave-pkg-level dependency to get better modulation, I was able to get help from Mike Miller from the octave mailing list, and made the following patch https://src.fedoraproject.org/rpms/octave-iso2mesh/c/2a98febb8e720aa03fc2f87f8744a379f4fe69db?branch=master 
  basically, rpm takes care of the rpm-package level dependency, and Depends in DESCRIPTION file also load dependencies in octave; because this is possible now, I have removed the duplicate functions from jsonlab and jnifti from iso2mesh

- the gmp-devel dependency was removed, but I don't understand why rpmlint calls this an error (the binaries clearly needs this library) - per Ankur's comment earlier, it does no harm to explicitly define such dependency despite overlapping, but seems rpmlint treats this as an conflict.

- the licenses of sub-tools are clarified in the comments


the below are no longer an issue any more, but I want to keep a note here

- I changed %make_build to make because it failed building the package on f31/f32; I later on reverted it to "%make_build" again, and that change once again failed the builds - the last line before failing is linking the cgal binaries. So, it is clear there is some issue using %make_build (i.e. SMP parallel building) with cgal targets, @Laurent, is this a known issue for CGAL? The log for one of the failed task can be found here

https://src.fedoraproject.org/rpms/octave-iso2mesh/c/2a98febb8e720aa03fc2f87f8744a379f4fe69db?branch=master 

- now I put arm as ExcludeArch, because @Ankur had a failed build on arm, and the error was "virtual memory exhausted" for CGAL, see https://koji.fedoraproject.org/koji/taskinfo?taskID=38115425 , if we have a way to build on arm, I will drop the ExcludeArch, but this is not a priority

I am going to close this ticket for now, thanks again. feel free to reopen if any issue appears.

Comment 35 Qianqian Fang 2019-10-11 22:18:29 UTC
sorry, for the failed %make_build log, please see the below link instead

https://koji.fedoraproject.org/koji/taskinfo?taskID=38209438

Comment 36 Fedora Update System 2019-10-11 22:20:36 UTC
FEDORA-2019-a94d821b53 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-a94d821b53

Comment 37 Fedora Update System 2019-10-11 22:21:08 UTC
FEDORA-2019-09a3e14f40 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-09a3e14f40

Comment 38 Fedora Update System 2019-10-11 22:21:45 UTC
FEDORA-2019-63ea1f8fbf has been submitted as an update to Fedora 31. https://bodhi.fedoraproject.org/updates/FEDORA-2019-63ea1f8fbf

Comment 39 Fedora Update System 2019-10-12 01:15:01 UTC
octave-iso2mesh-1.9.1-3.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-a94d821b53

Comment 40 Fedora Update System 2019-10-12 02:02:45 UTC
octave-iso2mesh-1.9.1-3.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-09a3e14f40

Comment 41 Fedora Update System 2019-10-13 00:56:30 UTC
octave-iso2mesh-1.9.1-3.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-63ea1f8fbf

Comment 42 Ankur Sinha (FranciscoD) 2019-10-13 14:03:26 UTC
(In reply to Qianqian Fang from comment #34)
> @Ankur, @Robert-Andre and @Laurent, thank you all for the comments and help
> on creating this package.
> 
> the latest spec file can be found on the src git site:
> https://src.fedoraproject.org/rpms/octave-iso2mesh/blob/master/f/octave-
> iso2mesh.spec
> 
> In the -3 version of this initial package, all the above mentioned issues
> were addressed, including
> 
> - thanks for Rob's patch, the binaries are now moved to /usr/libexec, and
> some of the license issues found by @Laurent
> 
> - per early discussions on setting octave-pkg-level dependency to get better
> modulation, I was able to get help from Mike Miller from the octave mailing
> list, and made the following patch
> https://src.fedoraproject.org/rpms/octave-iso2mesh/c/
> 2a98febb8e720aa03fc2f87f8744a379f4fe69db?branch=master 
>   basically, rpm takes care of the rpm-package level dependency, and Depends
> in DESCRIPTION file also load dependencies in octave; because this is
> possible now, I have removed the duplicate functions from jsonlab and jnifti
> from iso2mesh
> 
> - the gmp-devel dependency was removed, but I don't understand why rpmlint
> calls this an error (the binaries clearly needs this library) - per Ankur's
> comment earlier, it does no harm to explicitly define such dependency
> despite overlapping, but seems rpmlint treats this as an conflict.
> 
> - the licenses of sub-tools are clarified in the comments
> 
> 
> the below are no longer an issue any more, but I want to keep a note here
> 
> - I changed %make_build to make because it failed building the package on
> f31/f32; I later on reverted it to "%make_build" again, and that change once
> again failed the builds - the last line before failing is linking the cgal
> binaries. So, it is clear there is some issue using %make_build (i.e. SMP
> parallel building) with cgal targets, @Laurent, is this a known issue for
> CGAL? The log for one of the failed task can be found here
> 
> https://src.fedoraproject.org/rpms/octave-iso2mesh/c/
> 2a98febb8e720aa03fc2f87f8744a379f4fe69db?branch=master 

That is fine. If the tool does not support parallel make, you can use simple make (with -j1).

> 
> - now I put arm as ExcludeArch, because @Ankur had a failed build on arm,
> and the error was "virtual memory exhausted" for CGAL, see
> https://koji.fedoraproject.org/koji/taskinfo?taskID=38115425 , if we have a
> way to build on arm, I will drop the ExcludeArch, but this is not a priority

The guidelines require you to file a bug. Please do that if you haven't done so yet:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_support

> 
> I am going to close this ticket for now, thanks again. feel free to reopen
> if any issue appears.

Comment 43 Laurent Rineau 2019-10-14 09:43:47 UTC
(In reply to Qianqian Fang from comment #34)
> @Ankur, @Robert-Andre and @Laurent, thank you all for the comments and help
> on creating this package.
> 
> the latest spec file can be found on the src git site:
> https://src.fedoraproject.org/rpms/octave-iso2mesh/blob/master/f/octave-
> iso2mesh.spec
> 
> In the -3 version of this initial package, all the above mentioned issues
> were addressed, including
> 
> - thanks for Rob's patch, the binaries are now moved to /usr/libexec, and
> some of the license issues found by @Laurent

Great, thanks!

> - the gmp-devel dependency was removed, but I don't understand why rpmlint
> calls this an error (the binaries clearly needs this library) - per Ankur's
> comment earlier, it does no harm to explicitly define such dependency
> despite overlapping, but seems rpmlint treats this as an conflict.

Actually, I wonder why octave-iso2mesh requires any -devel package at runtime? Does the octave plugin need to build C/C++ sources at runtime?

> - now I put arm as ExcludeArch, because @Ankur had a failed build on arm,
> and the error was "virtual memory exhausted" for CGAL, see
> https://koji.fedoraproject.org/koji/taskinfo?taskID=38115425 , if we have a
> way to build on arm, I will drop the ExcludeArch, but this is not a priority

Oops. The use of the Mesh_3 package from CGAL might need a lot of RAM to build, in debug mode. Maybe if the arch is arm, you could remove `-g` from the compilation flags. That reduces a lot the RAM necessary to build.

Comment 44 Fedora Update System 2019-10-19 17:41:42 UTC
octave-iso2mesh-1.9.1-3.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.

Comment 45 Fedora Update System 2019-10-19 17:45:09 UTC
octave-iso2mesh-1.9.1-3.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 46 Fedora Update System 2019-10-26 17:25:15 UTC
octave-iso2mesh-1.9.1-3.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.


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