enable use of system CGAL
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
Comments (21)
-
-
reporter 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)
-
reporter 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!
-
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).
-
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.
-
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.)
-
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
-
reporter 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)
-
reporter Ah, I think the problem would be the reason for the patch that Johannes mentioned, 3rdparty/cgal.patch.
-
reporter 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).
-
reporter 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.
-
reporter 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.
-
Great! Thanks! I'll upgrade to the new cgal version and remove the patch.
-
reporter 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.
-
reporter I've made the 4.12 update in pull request #13.
-
reporter 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?
-
Maybe it is easier to start a new branch. Do you want to do it?
-
reporter I can do it if you like. I've got a patch for Debian I can apply in one shot.
-
Great!
-
reporter add compiler definition -DTETLIBRARY
needed when building against external (system) installation of tetgen
debian patch tetgen_define_TETLIBRARY.patch
Addresses Issue
#63→ <<cset 6b81e671b1ae>>
-
reporter - changed status to resolved
From 2018.1.0, mshr can use system CGAL and tetgen.
- Log in to comment
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.