Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add GudhUI compilation flag #1001

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

DavidLapous
Copy link
Member

This adds a flag WITH_GUDHI_GUDHUI to disable GudhUI compilation, without disabling everything.
This is very useful as Gudhi doesn't compile on a conda environment without qgl libraries, such as the following one :

channels:
- conda-forge
dependencies:
- auditwheel
- boost
- boost-cpp
- cgal-cpp
- cxx-compiler
- cython
- gudhi
- jupyterlab
- matplotlib
- numpy
- pybind11
- pytest
- python=3.11
- scikit-learn
- scipy
- sympy
- tbb
- tbb-devel
- tqdm

I suppose that cmake doesn't like some of the flags that comes with the cxx-compiler package, which gives the following error

[ 15%] Building CXX object src/GudhUI/CMakeFiles/GudhUI.dir/GudhUI_autogen/mocs_compilation.cpp.o
In file included from /user/dloiseau/home/git/gudhi-devel/build/src/GudhUI/GudhUI_autogen/include/ui_main_window.h:22,
                 from /user/dloiseau/home/git/gudhi-devel/build/src/GudhUI/GudhUI_autogen/DMHXEJ42XS/../../../../../src/GudhUI/gui/MainWindow.h:18,
                 from /user/dloiseau/home/git/gudhi-devel/build/src/GudhUI/GudhUI_autogen/DMHXEJ42XS/moc_MainWindow.cpp:10,
                 from /user/dloiseau/home/git/gudhi-devel/build/src/GudhUI/GudhUI_autogen/mocs_compilation.cpp:2:
/user/dloiseau/home/git/gudhi-devel/src/GudhUI/view/Viewer.h:14:10: fatal error: QGLViewer/qglviewer.h: No such file or directory
   14 | #include <QGLViewer/qglviewer.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

@mglisse
Copy link
Member

mglisse commented Nov 17, 2023

I am not opposed to a flag just for GudhUI, it makes some sense, but the priority should be understanding what is going wrong. CMakeLists.txt has

    find_package(QGLViewer QUIET)
    if ( QGLVIEWER_FOUND)

before the compilation of GudhUI, which seems to imply that QGLViewer was found by CMake. @VincentRouvreau could you try to reproduce?

@mglisse
Copy link
Member

mglisse commented Nov 17, 2023

@DavidLapous what does CMakeCache.txt say about QGLViewer in that failing scenario?

@mglisse
Copy link
Member

mglisse commented Nov 17, 2023

If I try on my laptop in a conda environment created from your .yml, CMake prints

++ GUDHI_MISSING_MODULES list is:"GudhUI;python"

(or python-documentation instead of python depending how I call cmake exactly)
and make does not try to build GudhUI.

Copy link
Member

@mglisse mglisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok if Vincent is ok with it.
But I would really like to understand the failure, don't forget about it just because you have a way to skip it.

@DavidLapous
Copy link
Member Author

Sorry for the delay. Here is my CMakeCache.txt.

@mglisse
Copy link
Member

mglisse commented Nov 20, 2023

It does not reproduce on my computer because cmake finds libGL.so in /usr/lib64 on Fedora but not in /usr/lib/x86_64-linux-gnu on Debian (strange)... and thus it never even looks for QGLViewer on my computer.
The problem seems to come from a discrepancy between the paths that cmake considers and the paths that the compiler considers. 2 things that would go in the right direction, although they may not solve the problem:

  • Use the version of CMake provided by conda (maybe they configure the default paths in a way better suited to the compilers they provide);
  • in FindQGLViewer.cmake, we should remove default things like /usr/include or /usr/lib, normally CMake already knows about default directories to look at.

@DavidLapous
Copy link
Member Author

I've tried with the make and cmake from conda (ie removing build + relaunch cmake), and it doesn't fix the problem.

@mglisse
Copy link
Member

mglisse commented Nov 20, 2023

I managed to reproduce by telling CMake to look in /usr/lib/x86_64-linux-gnu.
My guess is that the following happens:

  • cmake (even the one from conda) looks in /usr/include by default
  • cmake considers /usr/include a default system directory, so it avoids passing -I/usr/include to compilers
  • conda's compiler is configured not to look at /usr/include by default

Assuming my guess is close to the truth, that makes it a big bug in conda's cmake package (maybe related to conda-forge/cmake-feedstock#106 and conda-forge/cmake-feedstock#129 ?).

@mglisse
Copy link
Member

mglisse commented Nov 20, 2023

I can confirm that the cmake in conda has a long CMAKE_SYSTEM_PREFIX_PATH that includes not just the conda paths but also /usr, /usr/local (twice!), /usr/X11R6, /opt, etc).
If I add include_directories("/usr/include") and include_directories("/tmp/y"), I only get -I/tmp/y in the compiler invocation, even though CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES only lists paths in the conda installation.

What conda+cmake does is bound to break...

In the mean time, when something that is installed on your system causes trouble for your conda environment, a natural workaround is to install this in conda as well and hopefully the build will use the right version.

@VincentRouvreau
Copy link
Contributor

Thanks for the proposal @DavidLapous and for the investigation @mglisse !
Quite strange behaviour from cmake + conda to presume that some dependencies are installed in some specific folders...

@VincentRouvreau VincentRouvreau added build The build system (CMake, etc) 3.9.0 GUDHI version 3.9.0 labels Nov 20, 2023
@VincentRouvreau VincentRouvreau merged commit a018ea9 into GUDHI:master Nov 20, 2023
7 checks passed
@DavidLapous DavidLapous deleted the GudhUI_flag branch November 21, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9.0 GUDHI version 3.9.0 build The build system (CMake, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants