Cmake: make conan optional #999

vvilpas · 2020-10-20T13:36:15Z

Quoting original PR creator (#877):

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
Continues work started on #877 by drewrisinger

Precedence of environment variable is enforced by CMake policy CMP0077.

Env variable usage come after option declaration.

First pass, still has some bugs on non-Conan.

Uses shared AER_DEPENDENCY_PKGs, instead of solely from Conan. Remove explicit USE_CONAN language.

Found it's better to use IMPORTED INTERFACE vs ALIAS. ALIAS would sometimes fail on packages unexpectedly.

Macro makes it a lot clearer to understand what's happening.

Allow disabling conan import checks entirely with environment variable USE_CONAN. Allows conan not to be installed at all.

Required packages for setup should really be handled by pyproject.toml and setup_requirements. This removes the default calls to ``pip install MODULE``, which are unintuitive and potentially undesired for modifying your environment without prompting.

Fix reversion that wasn't 100%

…nan is disabled

…n is the default

vvilpas · 2020-10-26T11:53:42Z

Ready for review. I can build now on linux and macos without conan.

atilag

LGTM!
Haven't seen any blockers, just left some comments that could be addressed later.

atilag · 2020-11-02T10:14:33Z

cmake/conan_utils.cmake

@@ -1,33 +1,46 @@
 include(conan)

+macro(_rename_conan_lib package)
+    add_library(AER_DEPENDENCY_PKG::${package} INTERFACE IMPORTED)


oh! nice hack! :)

atilag · 2020-11-02T10:32:49Z

CMakeLists.txt


 set(AER_COMPILER_DEFINITIONS ${AER_COMPILER_DEFINITIONS} ${CONAN_DEFINES})
 # Cython build is only enabled if building through scikit-build.
 if(SKBUILD) # Terra Addon build
-	set(AER_LIBRARIES ${AER_LIBRARIES} CONAN_PKG::muparserx)
+	set(AER_LIBRARIES ${AER_LIBRARIES} AER_DEPENDENCY_PKG::muparserx)


This is probably on me, but I'm wondering if we should be using list() here instead of set() , we are using list() in other places for appending libraries (see AER_CONAN_LIBS usage), also feels more semantically correct. Thoughts?

BTW, I think I started to use set() instead of list() due to some misbehavior in CMake, I just can't recall exactly which one. If list() works and covers our use-cases then go to list(). Maybe in another PR with a "Good Fist Issue" tag?.

yeah, i think list is the way to go. We can check that it works and and replace all those sets.

atilag · 2020-11-02T10:41:45Z

CMakeLists.txt

@@ -320,9 +331,9 @@ else() # Standalone build
 		PRIVATE ${AER_COMPILER_DEFINITIONS})
 	if(WIN32 AND NOT BLAS_LIB_PATH)


Also wondering if we can simplify the BLAS_LIB_PATH logic now that we have a way to disable Conan.
I'm concerned about the complexity of the build system in general, and having a way to skip over BLAS, and skip over everything else looks confusing, doesn't it?.
What about removing the BLAS_LIB_PATH and if someone wants to build with their own BLAS, force them to go through the disable Conan path?. Certainly it will require them to have not just BLAS but every dependency installed.
Thoughts?

Not sure, because this was requested in order to use the same BLAS library as the one Numpy uses. So probably not in the usual paths where CMake will look for. Not sure if this option is very used, though.
But yeah, I agree, the build system is quite complex.

atilag · 2020-11-02T10:45:33Z

cmake/conan_utils.cmake

+        _rename_conan_lib(${CONAN_LIB})
+    endforeach()
+
+    if(APPLE)


At some point we'll need to review this. Maybe it's been fixed with current versions of Clang?

atilag · 2020-11-02T10:56:05Z

cmake/dependency_utils.cmake

+	endif()
+
+	if(APPLE)
+		# Fix linking. See https://stackoverflow.com/questions/54068035


Sometimes MacOS platform is so unpleasant for system devs :(

Maybe there are better ways to do this, but that's the way that I found. :(

Co-authored-by: Drew Risinger <[email protected]>

drewrisinger and others added 18 commits August 11, 2020 09:37

cmake: make conan optional

0aeecb3

CONTRIBUTING: add USE_CONAN CMake flag

3e192c0

releasenotes: add USE_CONAN CMake flag

647345a

CONTRIBUTING: remove trailing spaces

887fe73

cmake: add USE_CONAN environment variable

33e2f11

Precedence of environment variable is enforced by CMake policy CMP0077.

treewide: fix dependent spelling

7e1c5db

cmake: fix USE_CONAN precedence

f714304

Env variable usage come after option declaration.

cmake: add common dependency imports

75215a0

First pass, still has some bugs on non-Conan.

cmake: convert to AER_DEPENDENCY_PKG

bf9186d

Uses shared AER_DEPENDENCY_PKGs, instead of solely from Conan. Remove explicit USE_CONAN language.

dependencies: fix cmake importing

c7c0e21

Found it's better to use IMPORTED INTERFACE vs ALIAS. ALIAS would sometimes fail on packages unexpectedly.

cmake: refactor dependency load to macro

788d94c

Macro makes it a lot clearer to understand what's happening.

setup.py: use USE_CONAN env variable

1ac5997

Allow disabling conan import checks entirely with environment variable USE_CONAN. Allows conan not to be installed at all.

cmake: linking fixup

7bf17a7

Fix reversion that wasn't 100%

Merge branch 'master' into dr-pr-conan-optional

41dad37

Fixing Coanan build. Some fixes for non conan build

5720e23

Leave Macos out of non conan build for now

4c5f466

Remove not needed thrust include dirs. Fix Thrust name for find package

a64cb2c

vvilpas requested review from atilag and chriseclectic as code owners October 20, 2020 13:36

Merge branch 'master' into conan_optional

4d8fd33

vvilpas mentioned this pull request Oct 20, 2020

CMake: make conan optional #877

Closed

vvilpas and others added 8 commits October 20, 2020 18:44

Revert setup.py to make CI work again

8952f2f

Explicitly add thrust header path for conan

55ef04e

Catch2 package to AER_DEPENDENCY_PKG

44b0deb

Search for any version if required version is not found, only when Co…

b2f193d

…nan is disabled

Remove version when looking for any available library

47d0bd3

Merge branch 'master' into conan_optional

afcf486

Allow no conan on macos

a4d94ea

Fix win build

0c460e6

Change flag to disable instead of use, making more explicit that cona…

e631420

…n is the default

vvilpas changed the title ~~[WIP] CMake: make conan optional~~ Cmake: make conan optional Oct 26, 2020

Merge branch 'master' into conan_optional

691aeb1

chriseclectic added this to the Aer 0.8 milestone Oct 30, 2020

atilag approved these changes Nov 2, 2020

View reviewed changes

Merge branch 'master' into conan_optional

0b12288

chriseclectic merged commit 721fcb4 into Qiskit:master Nov 4, 2020

mtreinish mentioned this pull request Dec 6, 2020

How to disable conan? #1069

Closed

chriseclectic pushed a commit to chriseclectic/qiskit-aer that referenced this pull request Dec 11, 2020

Cmake: make conan optional (Qiskit#999)

83cff3b

Co-authored-by: Drew Risinger <[email protected]>

chriseclectic pushed a commit to chriseclectic/qiskit-aer that referenced this pull request Dec 11, 2020

Cmake: make conan optional (Qiskit#999)

376b61d

Co-authored-by: Drew Risinger <[email protected]>

chriseclectic added the Changelog: New Feature Include in the Added section of the changelog label Dec 11, 2020

drewrisinger mentioned this pull request Dec 11, 2020

Build: DISABLE_CONAN Env Var broken in qiskit-aer v0.7.2 #1079

Closed

chriseclectic pushed a commit to chriseclectic/qiskit-aer that referenced this pull request Jan 25, 2021

Cmake: make conan optional (Qiskit#999)

6cc30fb

Co-authored-by: Drew Risinger <[email protected]>

chriseclectic pushed a commit to chriseclectic/qiskit-aer that referenced this pull request Jan 25, 2021

Cmake: make conan optional (Qiskit#999)

f3c9e56

Co-authored-by: Drew Risinger <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake: make conan optional #999

Cmake: make conan optional #999

vvilpas commented Oct 20, 2020 •

edited

Loading

vvilpas commented Oct 26, 2020

atilag left a comment

atilag Nov 2, 2020

atilag Nov 2, 2020

atilag Nov 2, 2020

vvilpas Nov 3, 2020

atilag Nov 2, 2020

vvilpas Nov 3, 2020

atilag Nov 2, 2020

atilag Nov 2, 2020

atilag Nov 2, 2020

vvilpas Nov 3, 2020

		@@ -320,9 +331,9 @@ else() # Standalone build
		PRIVATE ${AER_COMPILER_DEFINITIONS})
		if(WIN32 AND NOT BLAS_LIB_PATH)

Cmake: make conan optional #999

Cmake: make conan optional #999

Conversation

vvilpas commented Oct 20, 2020 • edited Loading

Summary

Details and comments

vvilpas commented Oct 26, 2020

atilag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvilpas commented Oct 20, 2020 •

edited

Loading