Kratos: What is the best way to deal with the Tetgen submodule?

[oberbichler]

Add it to the local .gitignore?

[loumalouomega]

tetgen is not longer in the master. My experience is that if you remove manually (Shift+Delete) the tetgen submodule after merging with the master the pain in the ass will stop

[loumalouomega]

I assume that like you are german you are meaning

the
to Remove tetgen
the

[jginternational]

Caution! It's already in the master for those who need PFEM compiled

https://github.com/KratosMultiphysics/Kratos/tree/master/applications/DelaunayMeshingApplication/external_modules

[oberbichler]

Would it make sense to remove the submodule and include tetgen via CMake? (like e.g. the EigenSolverApplication does with the Eigen library)

[loumalouomega]

https://foonathan.net/blog/2016/07/07/cmake-dependency-handling.html

Maybe we can refine the dependency following this article

[philbucher]

I think it would be a good idea to include it via CMake. It is only used in one application but it continuously causes troubles for everyone.
what do you think @KratosMultiphysics/technical-committee ?

[roigcarlo]

Hi, it is not a technical issue, we can include it via CMake. In fact, the applications which need it do exactly that, but with a known directory ( the submodule folder )

If I remember correctly there is an specific interest in tetgent to be present in the repo, and due to licencing problems we can only include it via submodule. @josep-m-carbonell is that right?

If the problem is having the files because it pollutes the workflow the submodule can be "deinitialized"

https://stackoverflow.com/questions/1260748/how-do-i-remove-a-submodule
https://git-scm.com/docs/git-submodule (deinit)

[josep-m-carbonell]

Hi @roigcarlo

You are right in all of your explanations.
Maybe somebody can explain which is the matter in the submodule that causes problems to everyone, in order to solve it.

[loumalouomega]

The problem is just that it creates conflict in any branch that it is using a different version of the submodule (this is a bug of git, not ours)

[roigcarlo]

That should not happen if the module is not initialized

[loumalouomega]

[roigcarlo]

Can we close this?

[philbucher]

I don't think we reached a conclusion, imo it would be better to include it via cmake as we also do for most other external libs => this way devs not using it are not affected

I added it to my local gitignore to not have to deal with it any longer, but I don't know if we should aim for a more global solution

[RiccardoRossi]

@philbucher i am not sure of what you propose... one issue though is that tetgen is not free so it cannot be with the other libraries

[philbucher]

I mean if a user wants to use Tetgen, then they have to download it first and then in configure.sh give the path to it => the PFEM app would throw an error in case the path is not specified
=> same is done for Metis, Trilinos, MMG, Eigen,...
=> another advantage of this approach is that also other apps could use it if they want to

[RiccardoRossi]

+1 on my side for this solution...

[maceligueta]

Yes, it seems like the standard solution. We also adopted this for using CGAL and Nvidia Flex. Submodules appear to be uncomfortable to work with.

[josep-m-carbonell]

I don't use the standard Tetgen, there are modifications in the code. Then its better to have it in a submodule, because you ensure that the correct version will be downloaded and no extra configuration options must be added, making the compilation easier for the user.

[philbucher]

I don't use the standard Tetgen, there are modifications in the code. Then its better to have it in a submodule, then you ensure that the correct version will be downloaded and no extra configuration options must be added, making the compilation easier for the user.

This is fine, but instead of downloading Tetgen using a submodule the user would have to download it themself from https://github.com/PFEM/tetgen.git and give the path to it through CMake

EDIT @josep-m-carbonell you can add an application-readme to tell the user what to do, similar to what the HDF5-App and the EigenSolvers-App do

[loumalouomega]

Agreed with @philbucher, if everything is perfectly explained is not a problem. We do this with many applications already. Besides, you can provide scripts that simplify that tasks.

[loumalouomega]

We should integrate anyway only code of compatible licenses, as BSD or MIT. Tetgen is GPL, at that's a problem (a legal one)

[loumalouomega]

For example we don't include MMG which is LGPL and we have the permission of the authors

[josep-m-carbonell]

You don't want to deal with submodules:
git submodule deinit --all
Do you want to have and use them:
git submodule update --init

Do you want to convince somebody that downloading libraries, and adding extra lines in the CMakeList.txt its an easier way. I personally don't see it.

[loumalouomega]

It is clearly easier to use submodules if you want to add that application in particular, but if you are not, them the developer should now how to deinitilize the submodule.

[philbucher]

guess the decision is up to @KratosMultiphysics/technical-committee then

[maceligueta]

The @KratosMultiphysics/technical-committee dedicated some time to this issue last Friday, and we come with a proposal.
Considering the facts that:

• people had problems with the submodules and probably will
• the solution git submodule deinit --all will be forgotten soon or unknown for new users
• obliging PFEM users to go from an 'automatic' solution to a more manual solution is disappointing
• we should find a good solution for everyone which can become the standard

The proposal is: if one of the applications using Tetgen is ON in the configure, CMake detects it, looks for Tetgen in a specific folder (external_libraries?), if it is not there CMake downloads Tetgen, then the compilation starts with the desired flags already set in CMake.
@roigcarlo presented himself as a volunteer to code this, as long as it looks good to the community (comments??).
With this proposal, the compilation would be even more automatic than it is now, but it would not have submodules.

[philbucher]

thanks a lot @KratosMultiphysics/technical-committee for dedicating the time to find a good solution for everyone!

[josep-m-carbonell]

Ok @maceligueta, we must try first if this automatic detection and download by cmake works.

[loumalouomega]

https://foonathan.net/blog/2016/07/07/cmake-dependency-handling.html

[maceligueta]

Just to say that @pooyan-dadvand and @roigcarlo are currently working on this.

[pooyan-dadvand]

I made a patch which works for boost:

find_package(Boost)
if(Boost_FOUND)
  message("Boost found in ${BOOST_INCLUDE_DIRS}")
else(Boost_FOUND)
  message("Boost not found in local, downloading boost.zip from boost.org to ${CMAKE_BINARY_DIR} ...")
  file(DOWNLOAD https://dl.bintray.com/boostorg/release/1.67.0/source/boost_1_67_0.zip ${CMAKE_BINARY_DIR}/boost.zip)
  message("boost.zip downloaded. Unpacking in ${KRATOS_ROOT_DIR}/external_libraries...")
  execute_process(
    COMMAND ${CMAKE_COMMAND} -E tar xzf ${CMAKE_BINARY_DIR}/boost.zip  
    WORKING_DIRECTORY ${KRATOS_ROOT_DIR}/external_libraries)
  set(BOOST_ROOT ${KRATOS_ROOT_DIR}/external_libraries/boost_1_67_0)
  message("Finish unpacking. Setting BOOST_ROOT to ${BOOST_ROOT} and calling find package again...")
  find_package(Boost)
endif(Boost_FOUND)

I think we can do the same for other libraries.

Here there wasn't license incompatibility. In case of restriction we should at least prompt about license incompatibilities or activate the download only when some macro is defined.

[pooyan-dadvand]

Is this solved?

[josep-m-carbonell]

solved #4009