CMake refactor and fix examples.

[vaithak]

• CMake will now download all external dependencies (Eigen, boost, lpsolve) in external/_deps, this prevents multiple downloads of these libraries.
• Combined all cmake files for downloading external dependencies into a single source of truth in external/cmake-files directory - closes Put all .cmake files in one directory  #151.
• Enabled build of individual examples, also updated their README files with description.
• Removed parts of CMakeLists.txt from examples/logconcave and examples/mmcs_method which were already present at the root of examples.
• Added README in hpolytopeVolume and vpolytopeVolume examples.

[vaithak]

@vissarion, @TolisChal It would be great if codecov's app can be integrated with Github - link. This will allow codecov comments on every PR regarding coverage changes.

[vissarion]

@vissarion, @TolisChal It would be great if codecov's app can be integrated with Github - link. This will allow codecov comments on every PR regarding coverage changes.

Sure, is this process needed? Why not simply enabling coverage comments as described in https://docs.codecov.com/docs/pull-request-comments ?

[vaithak]

@vissarion, @TolisChal It would be great if codecov's app can be integrated with Github - link. This will allow codecov comments on every PR regarding coverage changes.

Sure, is this process needed? Why not simply enabling coverage comments as described in https://docs.codecov.com/docs/pull-request-comments ?

Yup, the setup process is needed. The coverage comments are enabled by default in the config, but for the bot to actually post the comments it requires the setup process.

[vissarion]

Thanks for this PR! I am ok with merging in general. But since you did this would it make sense to fix example issues and optionally build them in CI (only in actions is ok). For example, we are still getting errors while compiling them with the new cmake

fatal error: Eigen/Eigen: No such file or directory
    3 | #include "Eigen/Eigen"

in ellipsoid2d-sampling.cpp

EDIT: in particular you are getting the above error if you run cmake . in an example folder and no makefile is generated if you run cmake ..

@vissarion, @TolisChal It would be great if codecov's app can be integrated with Github - link. This will allow codecov comments on every PR regarding coverage changes.

Sure, is this process needed? Why not simply enabling coverage comments as described in https://docs.codecov.com/docs/pull-request-comments ?

Yup, the setup process is needed. The coverage comments are enabled by default in the config, but for the bot to actually post the comments it requires the setup process.

OK thanks, could you open an issue to think/discuss it and put it on schedule.

[vaithak]

EDIT: in particular you are getting the above error if you run cmake . in an example folder and no makefile is generated if you run cmake ..

Please see the new instructions for building examples (present in the README file for each example).
For example, to build ellipsoid-sampling, you will have to run the following steps in the examples/ellipsoid-sampling folder:

cmake .. -DEXAMPLES=ellipsoid-sampling
make

i.e, if you don't pass any example name in the -DEXAMPLES parameter to cmake, then no MakeFile will be generated.

[vissarion]

I see, thanks for the explanations! However, I find the use of -DEXAMPLES variable a bit confusing. Is there a technical reason for that? Since just cmake .. in a directory is simpler and clear.

Another issue I see is the creation of a new directory ellipsoid-sampling inside the examples/ellipsoid-sampling with the executable and some extra makefiles and cmakefiles. Is this needed? Is it simpler to just create one executable file as before?

[vaithak]

I see, thanks for the explanations! However, I find the use of -DEXAMPLES variable a bit confusing. Is there a technical reason for that? Since just cmake .. in a directory is simpler and clear.

Although cmake .. is simpler, but using -DEXAMPLES is useful when the person only wants to test out a particular example and so doesn't have to install dependencies for the successful build of other examples.
For example:
Consider the case when the user just wants to run the hpolytopeVolume example. If we used the earlier method of cmake .., then this will also require the user to have MKL and BLAS installed because examples/logconcave requires it. Thus, the user has to install a dependency which they may never require. By passing -DEXAMPLES=hpolytopeVolume, CMake won't include any other dependencies other than those required by that particular example.

---

Another issue I see is the creation of a new directory ellipsoid-sampling inside the examples/ellipsoid-sampling with the executable and some extra makefiles and cmakefiles. Is this needed? Is it simpler to just create one executable file as before?

I am not facing this on my system and it should not happen, because the whole procedure is exactly the same as earlier other than the exclusion of non required example files.

[vissarion]

Although cmake .. is simpler, but using -DEXAMPLES is useful when the person only wants to test out a particular example and so doesn't have to install dependencies for the successful build of other examples. For example: Consider the case when the user just wants to run the hpolytopeVolume example. If we used the earlier method of cmake .., then this will also require the user to have MKL and BLAS installed because examples/logconcave requires it. Thus, the user has to install a dependency which they may never require. By passing -DEXAMPLES=hpolytopeVolume, CMake won't include any other dependencies other than those required by that particular example.

OK, this makes sense, but then there should be a way to generate all examples in once and the intuitive way of doing this, I think, is cmake .

I am not facing this on my system and it should not happen, because the whole procedure is exactly the same as earlier other than the exclusion of non required example files.

I see, I was running cmake .. inside the ellipsoid-sampling directory. It seems that there exist enough space for the user to create nasty structures.

I think it is cleaner to have one self-contained folder per example, with one CMakeList.txt and the .cpp inside and optionally other useful files.

[vaithak]

Hi @vissarion, I agree with your comment above that there is enough space for user to mess up somehow, also each example should be self contained. So, I am thinking of having a separate and self contained CMakeLists.txt (will depend on external/cmake-files though) in each example's folder, this will enable a user to build each example independently.

Regarding the use case that you mentioned of building all examples together, I think this will be required more for testing purposes (like verifying if all examples are building), which can be done in the future by using a separate bash/python script in the root examples folder.
WDYT?

[vissarion]

Hi @vissarion, I agree with your comment above that there is enough space for user to mess up somehow, also each example should be self contained. So, I am thinking of having a separate and self contained CMakeLists.txt (will depend on external/cmake-files though) in each example's folder, this will enable a user to build each example independently.

I totally agree here, that would be much clearer.

Regarding the use case that you mentioned of building all examples together, I think this will be required more for testing purposes (like verifying if all examples are building), which can be done in the future by using a separate bash/python script in the root examples folder. WDYT?

Yes, that would not be a problem.

[vaithak]

made the changes 👍🏽 as discussed above.

[vissarion]

Thanks I am OK with merging. As a side note, a few examples are not compiling but this is an issue probably not related to this PR. We could open an issue and fix it later. It would also be useful to compile (not run) examples on CI.

[vissarion]

@papachristoumarios @TolisChal what do you think of this PR?