CMake: make conan optional

[drewrisinger]

Summary

This allows Conan to be an optional dependency. Conan requires an internet connection to build, and is meant for replicating development environments and not for downstream packaging. It also does not allow using the existing libraries in your system, e.g. muparserx (https://aur.archlinux.org/packages/muparserx/).

Details and comments

This is useful for having a single option for disabling Conan in the build system, instead of patching CMakeLists.txt.

This isn't the prettiest solution, and I'm not super familiar with CMake files, so I'm open to suggestions. The downside of this is that it doesn't do any version checking on the packages that it finds, which is probably not a great practice.

Fixes #757

[atilag]

Thanks for the PR!!
Before starting to review it, can I ask you what's your use case? Where is CONAN a problem?

[mtreinish]

Thanks for the PR!!
Before starting to review it, can I ask you what's your use case? Where is CONAN a problem?

@atilag the use case is in the PR summary. Both distro packaging and assuming an internet connection (both concerns I have outlined before). There are all also platforms where conan doesn't work like emscripten (which is why I opened #757 ), and systems where conan doesn't behave as expected like gcc 10 on my laptop.

[mtreinish]

From a quick glance this lgtm, the only thing I think would be useful here is to add an env var that exposes this option too. There are some build envs (looking at pip mostly) where passing in the cmake flags directly isn't available and enabling this via an env var is useful for those situations.

[drewrisinger]

@mtreinish added the USE_CONAN environment variable. Fixed a minor bug where include(conan_utils) would still run if USE_CONAN=OFF. Rebased on master.

[vvilpas]

@drewrisinger Thanks for the PR!!
Some things:

• Be aware that not all dependencies are needed depending on what you compile. For example muparserx is not needed if you compile the satandalone version, llvm-openmp is only needed for clang on macos if i recall correctly, etc.

• Given a build path, Cmake should fail if some library is not found, to avoid failing later at compilation time.

• We need to think of a mechanism to collect all the dependencies needed (with conan or not) and then use them avoiding if possible all the if(USE_CONAN) constructs.

•  If a package does not provide its own FindLIB.cmake, it's gonna fail, but I think it's ok. The user should be responsible for providing all external things. For example,I think, TBB didn't provide one.

• I think there are also things to change in test/CMakeLists.txt

• Right now, in Linux, I haven't been able to compile any of the Thrust backends. In Cuda I remember some problems with nvcc unable to compile modern nlohmann libraries. I haven't tried yet Windows or Macos

These are things to consider, we have to see how to solve, but again, thanks for contributing with this PR!!!

[vvilpas]

Some of these dependencies are only needed for certain build paths. For example, muparserx is only needed for the python addon, but not for the standalone version. llvm-openmp is only needed in Macos , etc. Ideally we should define which dependencies we really need depending on the build and then find them with conan or without it.

[vvilpas]

Careful, here we are setting some flags and include dirs coming from Conan

[atilag]

I'm concerned about the approach taken here.
In order to support downstream packaging or offline building (which I do agree both are good for the project, even though we should talk about the former with more calm), we have to minimize the impact of having two fundamentally different ways of handling the dependencies in the build system.
I wouldn't want to maintain two different ways of doing builds, also, the build system is complex enough to make sure every change adds value and doesn't add much more complexity (adding some is inevitable).
Having said that, I would tackle this problem this way:

• There's a setup_conan function in cmake/conan_utils.cmake which do all the hard work of downloading the correct dependencies from Conan-Center (the repository), and setting up some variables of the form: CONAN_PKG::library, that are consumed later in the CMakeFiles.txt. I'd try to create a setup_dependencies function instead, creating a new cmake/dependency_utils.cmake and branch there for the CONAN and non-CONAN use cases, and create our own set of AER_DEPENDENCY_PKG::library variables, that would substitute the current CONAN_PKG::library.
• As we support a non-negligible range of platforms and compilers combination, there's a lot of edges-cases covered in the build system, so I'd be specially careful when it comes to change the current logic. On of the pros of the approach I described above, is precisely this -- we don't need to mess with current logic spread all over the CMakeLists.txt files.

@drewrisinger thank you very much for jumping in into this! I really do appreciate it. If you want to try implementing this approach, I'll be glad to mentor you during the process.
If there are any concerns about my what I'm explaining here, I think it's ok to move this conversation to the issue @mtreinish already filed: #757

@vvilpas Thoughts?

[drewrisinger]

Thanks for your comments regarding a better, less hacky approach @atilag.

• Right now, in Linux, I haven't been able to compile any of the Thrust backends. In Cuda I remember some problems with nvcc unable to compile modern nlohmann libraries. I haven't tried yet Windows or Macos

I haven't tried that myself. I've mostly tried to get the base python package compiling.

Based on comments above, I see the TODO list as:

• Change ./tests/CMakeLists.txt
• Use @atilag 's suggestion about refactoring cmake/conan_utils.cmake to something like cmake/find_dependencies.cmake. This will use the USE_CONAN option and provide a single interface (ala CONAN_PKG::...) to all the build packages. This will find & make each of the libraries REQUIRED iff the build system would use them for that build configuration (i.e. platform/compiler/thrust)

[vvilpas]

@vvilpas Thoughts?

I agree. That's what I meant, although I didn't go into detail, with this:

• We need to think of a mechanism to collect all the dependencies needed (with conan or not) and then use them avoiding if possible all the if(USE_CONAN) constructs.

I think we should do it in two steps in that setup_dependencies function:

• Gather the list of required dependencies: A function where, depending on the flags, options, etc, we gather the list of dependencies that we need for the current build. This way, we keep this logic in just one place.
• Find dependencies: Depending on USE_CONAN var, pass that list of dependencies to the appropriate function: setup_conan or setup_system_dependencies (names can be different), so that we do not have a mix of CONAN and Cmake FindPackages stuff. The output of each of these functions would be the set of targets with the dependencies (with the same name as @atilag suggested): AER_DEPENDENCY_PKG::library1, AER_DEPENDENCY_PKG::library2, etc. Then we can use them in the rest of the CMake code independently of where they come.

The build system is kind of complex so maybe a total isolation is not possible, but let's try.
@atilag @drewrisinger What do you think?

[atilag]

Change ./tests/CMakeLists.txt

I think this is the only CMakeLists.txt file that we don't need to change.
These are the important ones:

• CMakeLists.txt
• qiskit/providers/aer/backends/wrappers/CMakeLists.txt

Use @atilag 's suggestion about refactoring cmake/conan_utils.cmake to something like cmake/find_dependencies.cmake.

Yes, just to clarify. We may not need to actually change conan_utils.cmake, but adding two new files:

• cmake/dependencies_utils.txt which is the one being imported in the root CMakeLists.txt, and that will have the USE_CONAN flag to branch to the conan path by importing conan_utils.cmake and calling the setup_conan() function, or local finding path by importing a new files called cmake/find_dependencies.cmake. We will want to set all the AER_DEPENDENCY_PKG::library variables in this last file.

[drewrisinger]

I will rebase and squash before this is ready to merge, but it's mostly done, and I wanted to get some feedback.

TODO:

• Replace remaining references to Conan variables: CONAN_DEFINES, CONAN_INCLUDE_DIRS_THRUST, CONAN_CXX_FLAGS_LLVM-OPENMP, CONAN_INCLUDE_DIRS_LLVM-OPENMP
• (MAYBE): Add conan-less build to test matrix???
• (MAYBE): Change ./test/CMakeLists.txt to use AER_DEPENDENCY_PKG instead.

[vvilpas]

@drewrisinger Thanks for pushing this!
I'll test and fully review when it is finished but I think this looks very good.
One thing I am missing, is a first step gathering all the dependencies needed. Then, use that list to let Conan or Cmake FindPackages find them. That way we'd have the logic to decide which dependencies are needed in just one place. But I think it's maybe too much and we can leave it for another PR.

[vvilpas]

Hi @drewrisinger! I am making some progress on this. Let me know if you want to keep going with this or you are busy with other things.

[drewrisinger]

@vvilpas feel free. I pushed the most recent changes I made (lastest ~1 month ago), I've been busy with other higher-priority projects recently. thanks for taking this up!

[vvilpas]

ok. I'm closing this one and keep working on #999 (with all changes on this PR). Thanks for all the work @drewrisinger !