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

[bug] conans.model.version can't be imported unless conan is imported first #17493

Open
fredizzimo opened this issue Dec 17, 2024 · 4 comments
Open
Assignees

Comments

@fredizzimo
Copy link
Contributor

Describe the bug

When trying to import conans.model.version without importing anything else first and import error is generated

The bug might be invalid if things under conans is considered private. But in practice for us, it isn't since a lot of the API uses models that only exists there, so I think at least bigger parts of it need to be public.

Tested with Conan 2.9.3 and 2.10.2

How to reproduce it

import conans.model.version
@memsharded memsharded self-assigned this Dec 17, 2024
@memsharded
Copy link
Member

Hi @fredizzimo

There is already a public API for Version, please find it here: https://docs.conan.io/2/reference/tools/scm/version.html#version

Yes, anything under conans or conan.internal, or anything that is not explicitly documented in docs.conan.io is an internal implementation detail and cannot be used, so this wouldn't be a bug, I am marking this ticket as a question. If there are needs for APIs that are not public, please create tickets describing the use case and the need for that specific thing to be part of the public API. Thanks for the feedback.

@fredizzimo
Copy link
Contributor Author

I'll reply on a more general level first, before creating specific issues.

The scm.Version is the same as conans.model.version

from conans.model.version import Version
, but we are not using it for scm stuff, so it feels more logical to use the model directly.

Here's a list of all conans import that we are using, so there's a lot more than just the Version

from conans.client.graph.graph import DepsGraph
from conans.client.graph.graph import Node, RECIPE_EDITABLE
from conans.client.graph.graph_error import GraphError
from conans.client.graph.python_requires import PyRequires
from conans.model.info import ConanInfo
from conans.model.layout import Infos, Conf
from conans.model.options import Options
from conans.model.recipe_ref import RecipeReference
from conans.model.requires import Requirements
from conans.model.settings import Settings
from conans.model.version import Version
from conans.model.version_range import VersionRange

Much of it is to just get typing information, for example our main conanfile looks like this to get basic code completion, which is not necessary entirely correct, nor exhaustive

    def init(self):
        self.name: str
        self.settings: Settings
        self.options: Options
        self.python_requires: PyRequires  # pylint: disable=attribute-defined-outside-init
        self.requires: Requirements
        self.tool_requires: Requirements
        self.info: ConanInfo
        self.cpp: Infos
        self.conf_info: Conf
        self.build_folder: str
        self.package_folder: str
        self.source_folder: str
        self.export_sources_folder: str
        self.recipe_folder: str

Others, like the RecipeReference is directly required in order to properly use the API, for example (although that function is not yet documented)

def recipe_revisions(self, ref: RecipeReference, remote=None):

In some cases, the models are referred to in the documentation like,
https://docs.conan.io/2/reference/extensions/python_api/GraphAPI.html#conan.api.subapi.graph.GraphAPI
Returns: a graph Node, recipe=RECIPE_CONSUMER

@memsharded
Copy link
Member

Thanks for the feedback.

I am afraid that this will be a challenge. We will move the remaining from conans space to an internal from conan.internal namespace in the following months. This has been a long process to reorganize the codebase. So your above code is guaranteed to break in future releases. This will still not be considered a bug and it works as documented.

Even after the move, it will not be allowed to import from conan.internal. Only the documented things in the docs and public API will be allowed to be used. If necessary we will document some special types public interfaces, just exclusively the public interface. But basically everything outside of the public apis, and specially from conan.internal will be guaranteed to break from time to time, as internal implementation details.

Please understand that the surface area of Conan codebase is huge. We cannot just simply expose all of it and allow users to use it directly, that will automatically put the project development and evolution to a halt, because it will be impossible to modify anything without breaking users. So there are no plans to expose that much of the Conan internals in the public API.

Regarding typing, which might help for IDE auto completion and things like that, is also strongly discouraged, except for publicly available and documented models. Because such typing automatically expose to developers all implementation methods and attributes of those classes, even if they are not publicly documented and not intended for being used in recipes or user code in general, which are not only likely to break, but also have risky behavior that can easily can cause bugs and issues when used directly in user code.

Returns: a graph Node, recipe=RECIPE_CONSUMER

Yes, this is generated automatically from code, so some errors are expected, this is why the API is still marked as experimental.

It is very likely that these objects when exposed in the API will either be treated as opaque objects (they can only be returned and passed to other API methods), or they will have a very limited public api, like a serialize() method that will expose its data as json-like, equivalent to the json output that conan ... --format=json of that thing will obtain.

@fredizzimo
Copy link
Contributor Author

Thanks for the reply, and explaining your situation, which we completely understand.

That's indeed a tricky situation, and we are aware of things breaking. So, we are currently treating every update as a potentially breaking one by locking the patch version of Conan, so in that sense it's not the end of the world if things move around and change a bit.

All our code was done before there was any documentation for the API, or at least much less than the current one, so we had no way of knowing what's supposed to be exposed or not. The tests do not help either, for example https://github.com/conan-io/conan/blob/44fe575039d8eb314c41668a056fd3973c6567c7/test/integration/conan_api/list_test.py, since they also import from the conans namespace in order to use the API

That said for a way forward, I suggest that you create interfaces for everything exposed by the API, and completely type annotate it. The type annotation is quite important for us actually, since we have quite much code dealing with Conan, and would like to be able to lint everything properly. It also helps with detecting errors when updating. And the interfaces help with isolating the actual implementation and only expose what's needed. Documentation does not work as well, since it's much harder to check, and when things change, it's easy for things to get unnoticed.

Similarly, the type annotation for the Conan file was required to get even somewhat reasonable output from pylint. Without it, it gets very confused. So, it would be nice to get that officially annotated as well.

Once there's a good specification, we can look at the API, and give more concrete examples of things currently missing, and what we need to do. At least I hope so, because someone else is taking over the maintenance of the conan code on our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants