enable use of system CGAL

[Drew Parsons]

We want to get mshr into Debian. But the Debian archive FTP masters are not permitting the package to enter the archive because of the embedded CGAL. Their position is that mshr "basically consists of a copy of an old cgal library. Please remove that embedded copy and use the cgal library that is already in the archive."

Can the mshr build be reconfigured to allow building against the system CGAL rather than the embedded CGAL source? For the record, Debian's current CGAL is v4.9.

I've logged a Debian "Intent to Package" at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=824716

[Benjamin Dam Kehlet]

Hi! Thanks for your effort!

Would it be sufficient to add an option in the build script so one could decide if one should use the system CGAL or the embedded one?

Another advantage of embedding the source is that we can patch it. We currently have only one small (but crucial) patch. With CGAL 4.9 we could probably work around it without patching CGAL, we have applied bug fixes to CGAL several times. It is nice to not have to wait for new releases to fix issues.

Does it matter that CGAL as of version 4.9 can be used as a header-only library. I guess the binaries will be almost identical regardless of which CGAL (system or embedded) is used.

[Drew Parsons]

Hi Benjamin, a build script option is a good way of doing it I think. The default configuration can use the CGAL code provided with mshr (i.e. the current situation), and Debian can activate the option to use system CGAL instead.

If our ftp masters require it, it's easy enough for me to elide the CGAL subdirectory from the mshr tarball (we have tools like uscan and git-buildpackage which can automate this).

The question of crucial patches could be a touch trickier. Probably I can get the required patch applied to Debian's CGAL. Not quite as simple as just patching it inside mshr, but it will let Debian maintain policy requirements. We use quilt to manage local patches, so it's easy enough to keep track of them.

I can ask about the header-only aspect, but I suspect it won't assuage the ftp masters. The challenge here is not about licence to redistribute CGAL, but rather about trying to avoid duplication of the code, and about ensuring the library code in the archive is up-to-date (e.g. with security patches applied immediately)

[Drew Parsons]

Thinking through the header-only option, that won't fix the ftpmasters' complaint since it's still duplicating the CGAL code. I hope the build script option to use the system library isn't too much trouble to add!

[Benjamin Dam Kehlet]

Yes, that is right (there were other reasons for switching to header only). Introducing a cmake option to use the system cgal should be trivial. I will do that soon. Getting rid of the patch, is a bit more complicated, but probably doable with CGAL 4.9. I'll take a look when I'm done with issue #10 (I hope to finish that this week).

[Johannes Ring]

I started looking into adding a USE_SYSTEM_CGAL option yesterday. It worked but failed to build due to the system CGAL was not patched. I have pushed the changes to a branch (johannr/optional-use-system-packages). @benjamik - feel free to push to that branch. I'm not sure when I can look at this again.

BTW, I think the same issue applies to Tetgen as well.

[Benjamin Dam Kehlet]

Thanks! Yes, we should do the same with Tetgen.

(I would like to make Tetgen optional, btw, but that requires some offers in the code.)

[Benjamin Dam Kehlet]

Thanks! Yes, we should do the same with Tetgen.

(I would like to make Tetgen optional, btw, but that requires some offers in the code.)

Benjamin

[Drew Parsons]

I'm testing johannr/optional-use-system-packages, setting cmake with -D USE_SYSTEM_CGAL:BOOL=ON to access the new functionality.
It's complaining about Polyhedron being declared private, e.g.:

/home/projects/dolfin/build/upstream/mshr/src/Polyhedral_multicomponent_mesh_domain_with_features_3.h:37:37: error: ‘typedef class CGAL::Polyhedron_3<CGAL::Epick, CGAL::Mesh_3::Mesh_polyhedron_items<int>, CGAL::HalfedgeDS_default, std::allocator<int> > CGAL::Polyhedral_mesh_domain_with_features_3<CGAL::Epick, CGAL::Polyhedron_3<CGAL::Epick, CGAL::Mesh_3::Mesh_polyhedron_items<int>, CGAL::HalfedgeDS_default, std::allocator<int> >, CGAL::Triangle_accessor_3<CGAL::Polyhedron_3<CGAL::Epick, CGAL::Mesh_3::Mesh_polyhedron_items<int>, CGAL::HalfedgeDS_default, std::allocator<int> >, CGAL::Epick>, CGAL::Boolean_tag<true>, CGAL::Boolean_tag<true> >::Polyhedron’ is private within this context
   typedef typename Base::Polyhedron Polyhedron;
                                     ^~~~~~~~~~

Also,

/home/projects/dolfin/build/upstream/mshr/src/CSGCGALMeshGenerator3D.cpp:340:49:   required from here
/home/projects/dolfin/build/upstream/mshr/src/Polyhedral_multicomponent_mesh_domain_with_features_3.h:110:36: error: ‘const class Polyhedral_multicomponent_mesh_domain_with_features_3<CGAL::Epick>’ has no member named ‘polyhedron’; did you mean ‘Polyhedron’?
   const Polyhedron& P  = r_domain_.polyhedron();
                          ~~~~~~~~~~^~~~~~~~~~
                          Polyhedron

Should the system CGAL functionality work at the moment? (my debian CGAL is 4.9-1+b2, gcc 7.1.0-13)

[Drew Parsons]

Ah, I think the problem would be the reason for the patch that Johannes mentioned, 3rdparty/cgal.patch.

[Drew Parsons]

Looks like the public typedef has been fixed in CGAL 4.10. Commit 1b8b63c48584fb6574b14fb3ff42757b000a7ad9 (29 Dec 2016).

That leaves the other part of the patch. Polyhedral_mesh_domain_with_features_3 still doesn't provide a getter for poly_ (now called stored_polyhedra).

[Drew Parsons]

I raised the issue for accessing stored_polyhedra upstream at https://github.com/CGAL/cgal/issues/2766 They're marking it for CGAL 4.12 or 4.13.

[Drew Parsons]

The upstream patch is committed (https://github.com/CGAL/cgal/commit/c3ad015f5126f78c07ef992a00bc93c5297c3086) and released now in CGAL-4.12-beta1

mesh. polyhedra() returns the vector<Polyhedron>& of the stored polyhedra.

[Benjamin Dam Kehlet]

Great! Thanks! I'll upgrade to the new cgal version and remove the patch.

[Drew Parsons]

CGAL 4.12 is now released. Fit for patching into mshr's internal copy, and then we can getting it working against the system CGAL.

[Drew Parsons]

I've made the 4.12 update in pull request #13.

[Drew Parsons]

Now we've got CGAL 4.12 in place, what's the best way to proceed? Merge master into Johannes' system_libs branch, or start a new branch and port Johannes' patches over to it?

[Johannes Ring]

Maybe it is easier to start a new branch. Do you want to do it?

[Drew Parsons]

I can do it if you like. I've got a patch for Debian I can apply in one shot.

[Johannes Ring]

Great!

[Drew Parsons]

add compiler definition -DTETLIBRARY

needed when building against external (system) installation of tetgen

debian patch tetgen_define_TETLIBRARY.patch

Addresses Issue #63

→ <<cset 6b81e671b1ae>>

[Drew Parsons]

From 2018.1.0, mshr can use system CGAL and tetgen.