conan icon indicating copy to clipboard operation
conan copied to clipboard

[bug] `required_conan_version` is not evaluated before python imports

Open SpaceIm opened this issue 3 years ago • 6 comments

Environment Details (include every applicable attribute)

  • Operating System+version: macOS Monterey
  • Compiler+version: apple-clang 13.1
  • Conan version: 1.48.0
  • Python version: 3.9.12

Steps to reproduce (Include if Applicable)

see https://github.com/conan-io/conan-center-index/pull/10633#issuecomment-1124783149

  • use conan < 1.45.0, for example 1.44.0
  • conan install yaml-cpp/0.6.3
  • Instead of the meaningful message ERROR: Error loading conanfile at (...): Current Conan version (1.44.0) does not satisfy the defined one (>=1.45.0). we could expect due to required_conan_version = ">=1.45.0", there are python imports error.

Logs (Executed commands with output) (Include/Attach if Applicable)

log from https://github.com/conan-io/conan-center-index/pull/10633#issuecomment-1124783149:

00:00:22  yaml-cpp/0.6.3: Retrieving from server 'conancenter'
00:00:22  yaml-cpp/0.6.3: Trying with 'conancenter'...
00:00:22  Downloading conanmanifest.txt
00:00:22  Downloading conanfile.py
00:00:22  Downloading conan_export.tgz
00:00:22  yaml-cpp/0.6.3: Downloaded recipe revision 0
00:00:22  ERROR: yaml-cpp/0.6.3: Cannot load recipe.
00:00:22  Error loading conanfile at '***/CONAN_CACHE_arm64/.conan/data/yaml-cpp/0.6.3/_/_/export/conanfile.py': Unable to load conanfile in ***/CONAN_CACHE_arm64/.conan/data/yaml-cpp/0.6.3/_/_/export/conanfile.py
00:00:22    File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/imp.py", line 171, in load_source
00:00:22      module = _load(spec)
00:00:22    File "<frozen importlib._bootstrap>", line 711, in _load
00:00:22    File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
00:00:22    File "<frozen importlib._bootstrap_external>", line 850, in exec_module
00:00:22    File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
00:00:22    File "***/CONAN_CACHE_arm64/.conan/data/yaml-cpp/0.6.3/_/_/export/conanfile.py", line 1, in <module>
00:00:22      from conan.tools.microsoft import is_msvc, is_msvc_static_runtime
00:00:22  ImportError: cannot import name 'is_msvc' from 'conan.tools.microsoft' (***/lib/python3.9/site-packages/conan/tools/microsoft/__init__.py)

SpaceIm avatar May 12 '22 14:05 SpaceIm

I dont think this is a bug, this is by design.

Python imports are evaluated when the script is loaded. And we need to load the script to get the required_conan_version variable from it. We are not going to manually parse the .py file looking for the required_conan_version definition, that would be overkill.

memsharded avatar May 12 '22 14:05 memsharded

Maybe it would be overkill, but at same time, that error is not helpful.

uilianries avatar May 12 '22 15:05 uilianries

that error is not helpful.

It is not like an error from a c++ compiler, a missing import gives you a couple of clues.

But it is true that this situation will happen a lot with the migration to 2.0. We might try to introduce something temporarily and accessible to remove in the future?

lasote avatar May 17 '22 07:05 lasote

But it is true that this situation will happen a lot with the migration to 2.0. We might try to introduce something temporarily and accessible to remove in the future?

Maybe, but lets outline the solution:

  • Every conanfile.py should be loaded as text before being used
  • The required_conan_version needs to be manually parsed from that text

That means doubling the conanfile file loading time, for big graphs it could be even noticeable. There are risks of introducing bugs, like the loading of conanfile.py as text doesn't take into account some encoding, the required_conan_version is not located in the conanfile.py but also re-used from other file or inherited from a python_requires, parsing errors of the required_conan_version because of single quotes, double quotes, a trailing comment...

I was not concerned about it being temporary, but about all these little things, that added, could become annoying. But if you think it is doable and it is worth UX-wise, then no problem, lets do it.

memsharded avatar May 17 '22 07:05 memsharded

I was thinking about trying to locate the required_conan_version "manually" only after getting an ImportError (to improve the error message if we locate the required_conan_version, or keep raising otherwise)

lasote avatar May 18 '22 10:05 lasote

I was thinking about trying to locate the required_conan_version "manually" only after getting an ImportError (to improve the error message if we locate the required_conan_version, or keep raising otherwise)

Oh, that could be a better approach, indeed. We could give that a try 👍

memsharded avatar May 18 '22 10:05 memsharded

https://github.com/conan-io/conan/pull/11908 merged, this will be in 1.52

memsharded avatar Aug 23 '22 09:08 memsharded