restore python linters #25360
restore python linters #25360
Conversation
ericLemanissier
force-pushed
the
linter-v2
branch
from
3f70e1d
to
7bc7e41
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
can be tested with |
Conan v1 pipeline ❌Sorry, the build is only launched for Access Request users. You can request access writing in this issue. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Sorry, the build is only launched for Access Request users. You can request access writing in this issue. |
|
For full context - the legacy linter, as far as the Conan Center team was concerned, was introduced only with the intention of catching "Conan 2" compatibility of recipes before any level of CI validation of Conan 2 recipes was happening. Any generic pylint checks that happened as a result of it were an added bonus, but never the core of the functionality. It was a mistake from our end to conflate the two things. in the past several weeks and months - we have noticed the warnings coming from the linter to be:
To the point where it is disrupting both contributors and reviewers. The dual Conan v1 and Conan v2 CI process should exercise most code paths (but not all) - so at the very least we know that for the mainline supported configurations there should be no issues. The team is in the process of rewriting the Conan Center CI linter and hooks - one that is more suited the errors we want to catch and fully based on Conan 2. We did perform an extensive reviews of the types of failures, the frequency of them, and the false vs true positives, and deemed that true positives that are not otherwise caught by the regular pipeline are a significantly infrequent - the last time I check, this was in the range of 4-5 in a whole month. I believe it will take us less than that time to have the new linter running, and thus, we don't think that in this period users will be impacted. Our goal is to ensure their experience is not at all degraded. We want to ensure the experience of the users is acceptable (free of errors), and that the experience of both contributors are reviewers is not impeded by noise. |
|
Tagging @perseoGI - as he can provide example output of the new linters that we will integrate in our CI. |
|
Do I understand correctly that you are redevelopping a static analyzer for python code, from scratch ? |
No |
|
Thanks for taking the time to explain your decision. It's reasuring to know that you don't abandon the idea of static analysis. |
|
Hi @ericLemanissier, I'm right now in charge of the new linter. I'm attaching a quick recap of our internal PR just for you as you seem very interested in the tech details: Conan linter to catch up conan specific semantic errors/warnings/notes. This linter combines different tools:
The custom We can now detect conan specific/semantic errors which we hope will be useful for CCI contributors. Usage: $ conan lint [-h] [-f FORMAT] [-c CONFIG_FILE] [-d [DISABLE ...]] [-e [ENABLE ...]] [more options] [path ...]This command allows printing in a beautiful manner different rules (it is easily extensive): $ conan cci:lint tests/assets/cci_lint_repo/recipes/test_recipe/all/conanfile.py
It can also accept multiple input files using glob expression or delegating on terminal expansion: $ conan cci:lint tests/assets/cci_lint_repo/recipes/test_recipe/**/*.yml --format=text
It also support json output: $ conan cci:lint tests/assets/cci_lint_repo/recipes/test_recipe/config.yml --format=json
And GitHub format to be used from actions and allow inline markups: $ conan cci:lint tests/assets/cci_lint_repo/recipes/test_recipe/all/conanfile.py --format=github
Real exampleThe idea of this linter is to be used by contributors before pushing to Conan Center Index. Also, C3I may execute the linter before trying to compile
CustomizationBy creating a By installing the conan configuration, a default A good practice will be to create this file in the Conan Center Index repository. Pylint default rulesUntil now we have discarded the following Our plan is to disable all If you wish to collaborate, it will be helpful to have a list of rules to enable, (apart from our custom rules) I hope this shed some light on your concerns. |
|
Again, thanks a lot for all these details. It's really reassuring to see that is actually a continuation of what was already in place, and not a "throw the baby with the bathwater" situation. This plan sound great. |
|
Yes! It is a continuation of the existing work but with more flexibility. This is the current state of # Class ConanFile doesn't declare all the valid members and functions,
# some are injected by Conan dynamically to the class.
import textwrap
import astroid
from astroid.builder import AstroidBuilder
from astroid.manager import AstroidManager
def _settings_transform():
module = AstroidBuilder(AstroidManager()).string_build(
textwrap.dedent(
"""
class Settings(object):
os = None
arch = None
build_type = None
class Compiler:
def get_safe(self, attr):
pass
compiler = Compiler()
"""
)
)
return module["Settings"]
def _user_info_build_transform():
module = AstroidBuilder(AstroidManager()).string_build(
textwrap.dedent(
"""
class UserInfoBuild(defaultdict):
pass
"""
)
)
return module["UserInfoBuild"]
def _requires_transform():
module = AstroidBuilder(AstroidManager()).string_build(
textwrap.dedent(
"""
class RecipeRequires(OrderedDict):
def __call__(build_require, force_host_context=False):
pass
"""
)
)
return module["RecipeRequires"]
def register(_):
pass
def transform_conanfile(node):
"""Transform definition of ConanFile class so dynamic fields are visible to pylint"""
str_class = astroid.builtin_lookup("str")
dict_class = astroid.builtin_lookup("dict")
info_class = astroid.MANAGER.ast_from_module_name("conans.model.info").lookup(
"ConanInfo"
)
python_requires_class = astroid.MANAGER.ast_from_module_name(
"conans.client.graph.python_requires"
).lookup("PyRequires")
cpp_info_class = astroid.MANAGER.ast_from_module_name(
"conans.model.build_info"
).lookup("CppInfo")
dynamic_fields = {
"conan_data": str_class,
"user_info_build": [_user_info_build_transform()],
"info_build": info_class,
"info": info_class,
"build_requires": [_requires_transform()],
"test_requires": [_requires_transform()],
"tool_requires": [_requires_transform()],
"cpp_info": cpp_info_class,
"python_requires": [str_class, python_requires_class],
"recipe_folder": str_class,
"settings_build": [_settings_transform()],
"settings_target": [_settings_transform()],
"settings_validate": [_settings_transform()],
"conf": dict_class,
}
for f, t in dynamic_fields.items():
node.locals[f] = [i for i in t]
# TODO dont work, filtered on handle_message
# def transform_settings(node):
# if isinstance(node, nodes.ClassDef) and node.name == "ConanFile":
# for method in node.mymethods():
# if method.name == "build":
# settings_class = _settings_transform()
#
# # Iterate over all statements within the function
# for subnode in method.nodes_of_class(nodes.Expr):
# for attr_node in subnode.nodes_of_class(nodes.Attribute):
# if (
# isinstance(attr_node.expr, nodes.Attribute) and
# isinstance(attr_node.expr.expr, nodes.Name) and
# attr_node.expr.expr.name == "self" and
# attr_node.expr.attrname == "settings"
# ):
# # Replace self.settings with an instance of our custom Settings class
# attr_node.expr = settings_class.instantiate_class()
# break # Assuming a single usage per function
astroid.MANAGER.register_transform(
astroid.ClassDef,
transform_conanfile,
lambda node: node.qname() == "conans.model.conan_file.ConanFile",
)
# astroid.MANAGER.register_transform(nodes.FunctionDef, transform_settings)I've to try your solution, but I wanted to share the code with you to see if you can help a bit! |
|
I think the cause of pylint considering self.settings as a list is this line https://github.com/conan-io/conan/blob/28739c763000afc908ebc64eba717b7d01784de1/conans/model/conan_file.py#L98 so if you don't feel my code above is a solution, another way could be to rewrite Appart from that, I don't think we actually need the Also, I think pylint correctly deduces the type of To answer your question above, I'll have a hard time listing you an actual list of "must have" checkers, considering the very long list at https://pylint.readthedocs.io/en/latest/user_guide/checkers/features.html Once again, thanks @perseoGI for having this constructive technical discussion. It's nice to see that collaboration is possible, despite the initial reaction to this PR. By the way, you may want to continue the discussion in a more suitable space than a closed PR on the wrong repo ? |
Summary
Motivation
As explained earlier, I don't think that removing all python linters is a wise move: it is a very serious reduction on the quality of conan-center recipe.
Random finding of the day:
conan-center-index/recipes/libuvc/all/conanfile.py
Line 53 in bcd63d5
I guess that this probably won't add any weigh to this PR, but I have to say that in the context I am, the absence of static code analysis on recipes forces us to question the use of CCI recipes, so basically reconsider our choice of conan as package manager alltogether.
regarding the results of this PR at https://github.com/conan-io/conan-center-index/actions/runs/10991830632?pr=25360
Linter summary (recipes)
src_folderargument which should be tosrc: 1 <= this is a true positiveLinter summary (test_package)