This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 714336 - (tetgen) Review Request: tetgen - A Quality Tetrahedral Mesh Generator and a 3D Delaunay Triangulator
Review Request: tetgen - A Quality Tetrahedral Mesh Generator and a 3D Delaun...
Status: CLOSED DUPLICATE of bug 483663
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-Legal
  Show dependency treegraph
 
Reported: 2011-06-18 03:01 EDT by Ankur Sinha (FranciscoD)
Modified: 2011-07-01 12:12 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-06-30 17:49:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Ankur Sinha (FranciscoD) 2011-06-18 03:01:27 EDT
Spec URL: http://ankursinha.fedorapeople.org/tetgen/tetgen.spec
SRPM URL: http://ankursinha.fedorapeople.org/tetgen/tetgen-1.4.3-1.fc15.src.rpm

Description: 
TetGen is a program to generate tetrahedral meshes of any 
3D polyhedral domains. TetGen generates exact constrained 
Delaunay tetrahedralizations, boundary conforming Delaunay 
meshes, and Voronoi partitions. The following pictures 
respectively illustrate a 3D polyhedral domain (left), a 
boundary conforming Delaunay tetrahedral mesh (middle), and 
its dual - a Voronoi partition (right).

TetGen provides various features to generate good quality and 
adaptive tetrahedral meshes suitable for numerical methods, such 
as finite element or finite volume methods. For more information 
of TetGen, please take a look at a list of features.

TetGen is written in C++. It can be compiled into either a 
standalone program invoked from command-line or a library for 
linking with other programs. All major operating systems, 
e.g. Unix/Linux, MacOS, Windows, etc, are supported.

================================================================================

[ankur@ankur rpmbuild]$ rpmlint SPECS/tetgen.spec SRPMS/tetgen-1.4.3-1.fc15.src.rpm RPMS/x86_64/tetgen-*
tetgen.src: W: spelling-error %description -l en_US tetrahedralizations -> rationalizations, liberalizations
tetgen.x86_64: W: spelling-error %description -l en_US tetrahedralizations -> rationalizations, liberalizations
tetgen.x86_64: W: no-manual-page-for-binary tetgen
3 packages and 1 specfiles checked; 0 errors, 3 warnings.
Comment 1 Volker Fröhlich 2011-06-27 17:17:22 EDT
I think that caption doesn't belong into the package description:

TetGen is written in C++. It can be compiled into either a 
standalone program invoked from command-line or a library for 
linking with other programs. All major operating systems, 
e.g. Unix/Linux, MacOS, Windows, etc, are supported.

There are no pictures:

The following pictures respectively illustrate a 3D polyhedral
domain (left), a boundary conforming Delaunay tetrahedral mesh
(middle), and its dual - a Voronoi partition (right).

No need to rm -rf $RPM_BUILD_ROOT.

It might be a good idea to include the documentation you can download from the homepage.

Your CFLAGS and CXXFLAGS are not worshiped:

g++ -O0 -c predicates.cxx
g++ -g -Wall -DSELF_CHECK -o tetgen tetgen.cxx predicates.o -lm
Comment 2 Ankur Sinha (FranciscoD) 2011-06-28 13:02:26 EDT
(In reply to comment #1)
> I think that caption doesn't belong into the package description:
> 
> TetGen is written in C++. It can be compiled into either a 
> standalone program invoked from command-line or a library for 
> linking with other programs. All major operating systems, 
> e.g. Unix/Linux, MacOS, Windows, etc, are supported.
> 
> There are no pictures:
> 
> The following pictures respectively illustrate a 3D polyhedral
> domain (left), a boundary conforming Delaunay tetrahedral mesh
> (middle), and its dual - a Voronoi partition (right).
> 
> No need to rm -rf $RPM_BUILD_ROOT.
> 
> It might be a good idea to include the documentation you can download from the
> homepage.
> 
> Your CFLAGS and CXXFLAGS are not worshiped:
> 
> g++ -O0 -c predicates.cxx
> g++ -g -Wall -DSELF_CHECK -o tetgen tetgen.cxx predicates.o -lm


Hi Volker, 

updated packages:

http://ankursinha.fedorapeople.org/tetgen/tetgen.spec

http://ankursinha.fedorapeople.org/tetgen/tetgen-1.4.3-1.fc15.src.rpm

I'll package the docs as a separate package? Or I can always include them later. 

Thanks,
Ankur
Comment 3 Volker Fröhlich 2011-06-28 17:57:34 EDT
Again: Please don't upload files using the same release number as before. It makes work harder for the reviewer.

g++  -c predicates.cxx is still there.

What do you think of that other paragraph of the description? Does it help the Fedora user? The package already built the binary, so it is pointless, from my point of view. If I wanted to install this package, I wouldn't mind, if it built on Windows too. And if I wanted to see what language it was written in, I'd seek for the source code.

As of the documentation: If it is not very big, include it in the main package.
Comment 4 Ankur Sinha (FranciscoD) 2011-06-29 10:29:07 EDT
(In reply to comment #3)
> Again: Please don't upload files using the same release number as before. It
> makes work harder for the reviewer.
> 

Sorry, will do. 

> g++  -c predicates.cxx is still there.

Added a dirty work around to make this work. Please note that it follows all the optflags other than -O2. The makefile says the file should be compiled with the -O0 flag to work properly (no optimization).

> 
> What do you think of that other paragraph of the description? Does it help the
> Fedora user? The package already built the binary, so it is pointless, from my
> point of view. If I wanted to install this package, I wouldn't mind, if it
> built on Windows too. And if I wanted to see what language it was written in,
> I'd seek for the source code.
> 

There's no harm in letting it be. It only gives the user some information. For example, it tells him that the application is also available for other OSes. I don't see a reason to remove it. :)

> As of the documentation: If it is not very big, include it in the main package.

Included.

[ankur@ankur SPECS]$ rpmlint tetgen.spec ../SRPMS/tetgen-1.4.3-2.fc15.src.rpm ../RPMS/x86_64/tetgen-* 
tetgen.src: W: spelling-error %description -l en_US tetrahedralizations -> rationalizations, liberalizations
tetgen.x86_64: W: spelling-error %description -l en_US tetrahedralizations -> rationalizations, liberalizations
tetgen.x86_64: W: no-manual-page-for-binary tetgen
3 packages and 1 specfiles checked; 0 errors, 3 warnings.


New spec/srpm:

http://ankursinha.fedorapeople.org/tetgen/tetgen-1.4.3-2.fc15.src.rpm

http://ankursinha.fedorapeople.org/tetgen/tetgen.spec
Comment 5 Kevin Kofler 2011-06-30 17:42:34 EDT
> Added a dirty work around to make this work. Please note that it follows all
> the optflags other than -O2. The makefile says the file should be compiled with
> the -O0 flag to work properly (no optimization).

-O0 is not acceptable except at most as a temporary workaround. It generates completely awful code. (If you had ever looked at assembly from -O0, you'd cry!)

Please test the code and figure out what optimization breaks it, and then turn off that particular optimization specifically. (-fno-strict-aliasing is a likely candidate.)


And this paragraph:
> TetGen is written in C++. It can be compiled into either a 
> standalone program invoked from command-line or a library for 
> linking with other programs. All major operating systems, 
> e.g. Unix/Linux, MacOS, Windows, etc, are supported.
definitely has no business being in the package's %description. It brings no value whatsoever to Fedora users, it is just noise.
Comment 6 Kevin Kofler 2011-06-30 17:49:04 EDT
In addition, tetgen.cxx contains this license header:

// TetGen is freely available through the website: http://tetgen.berlios.de. //
//   It may be copied, modified, and redistributed for non-commercial use.   //
//   Please consult the file LICENSE for the detailed copyright notices.     //

And in fact LICENSE contains this "exception" (more like an additional restriction) to the MIT license:

> Distribution of this code for  any  commercial purpose  is permissible
> ONLY BY DIRECT ARRANGEMENT WITH THE COPYRIGHT OWNER.

This is most definitely NOT a Free license. Putting on the legal blocker, and closing as CANTFIX, since there is no way this can go in with that license.
Comment 7 Kevin Kofler 2011-06-30 17:51:45 EDT
The LICENSE file even explicitly says "This means that TetGen is no free software".
Comment 8 Ankur Sinha (FranciscoD) 2011-06-30 22:12:51 EDT
Hi Kevin,

I'll try and get in touch with upstream to request moving to a free license.. but yes..MEH!

Thanks,
Ankur
Comment 9 Kevin Kofler 2011-07-01 05:43:53 EDT
Good luck… Reading the LICENSE file, I get the impression that upstream knows very well what Free Software means, but isn't interested. :-(

Do you have any further review requests which depend on this? If so, it might be worth trying to get them working without this stuff. (AFAICT, qhull can do similar computations, but I have no idea how different the feature set and the API are.)
Comment 10 Ankur Sinha (FranciscoD) 2011-07-01 08:12:17 EDT
(In reply to comment #9)
> Good luck… Reading the LICENSE file, I get the impression that upstream knows
> very well what Free Software means, but isn't interested. :-(
> 
> Do you have any further review requests which depend on this? If so, it might
> be worth trying to get them working without this stuff. (AFAICT, qhull can do
> similar computations, but I have no idea how different the feature set and the
> API are.)

Mailed upstream.

This isn't really a build requires. It was to be an add on tool for one of the fedora-medical packages. It'll lose some minor functionality. Not really a big deal :)

Thanks,
Ankur
Comment 11 Fabian Affolter 2011-07-01 12:12:38 EDT

*** This bug has been marked as a duplicate of bug 483663 ***

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