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.

Sign up for GitHub

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

Jump to bottom

[linter] Transform ConanFile class to match actual one #11162

Merged
merged 14 commits into from
Jun 17, 2022
Merged

[linter] Transform ConanFile class to match actual one #11162

merged 14 commits into from
Jun 17, 2022

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Jun 13, 2022

Conan adds some members and functions dynamically to the ConanFile class.

@ericLemanissier ericLemanissier mentioned this pull request Jun 13, 2022
Closed
4 tasks
jgsogo
jgsogo commented Jun 13, 2022
View reviewed changes
linter/.pylintrc
py-version=3.6
recursive=no
suggestion-mode=yes
unsafe-load-any-extension=no

[MESSAGES CONTROL]
disable=all
disable=fixme,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan is to disable everything needed for all the recipes to work... then, following PRs can try to enable one by one.

ericLemanissier
ericLemanissier reviewed Jun 14, 2022
View reviewed changes
linter/conanfile_transform.py
dict_class = astroid.builtin_lookup("dict")
info_class = astroid.MANAGER.ast_from_module_name("conans.model.info").lookup(
"ConanInfo")
build_requires_class = astroid.MANAGER.ast_from_module_name(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
build_requires_class = astroid.MANAGER.ast_from_module_name(
user_info_build_class = astroid.MANAGER.ast_from_module_name("conans.model.user_info").lookup(
"DepsUserInfo")
build_requires_class = astroid.MANAGER.ast_from_module_name(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why, but using this approach, it fails for odbc recipe. The error is weird and I didn't manage to find a fix on the internet. With the string_build approach, on the other hand, it works. I'm not very happy about how it is looking like, tbh.

linter/conanfile_transform.py Outdated
"build_requires": build_requires_class,
"tool_requires": build_requires_class,
"info_build": info_class,
"user_info_build": info_class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"user_info_build": info_class,
"user_info_build": user_info_build_class,

@jgsogo
Copy link
Contributor Author

jgsogo commented Jun 15, 2022 •
edited
Loading

Just #11213 and #11180 and the linter (python-recipes) will pass for all the recipes in the repository 💪

Linter for YAML still fails, but those errors are really easy to fix and any user creating a PR can do it.

@jgsogo
Copy link
Contributor Author

jgsogo commented Jun 15, 2022

Not related. Do we really want to enabe all the "Convention", "Refactor" and "Information" checks ? They clutter the jobs outputs, make the global test longer, and will probably be ignored 99.9% of the time, because they are not picked up by the problem matcher (which is fine because there are really to much messages of these types on current conan recipes)

The focus (after merging this) should be to help in the migration process to Conan v2. We want to introduce and show in the PR-files errors like: "use CMakeDeps and CMakeToolchain instead of legacy generators (more info here)". This is the kind of information that should appear inline in the PRs.

Convention, refactor, information,... checks might appear in the output of the GH action, but they shouldn't be picked in the problem matcher, they might add too many messages (~noise) and we are not really interested in them (at least while we push the migration to v2).

About time, the global test is only triggered when the linter files are modified, not for PRs that modify recipes, so it is not an issue at all.

@jgsogo
Copy link
Contributor Author

jgsogo commented Jun 16, 2022 •
edited
Loading

New errors introduced in the last hours #11238 and #11239

@jgsogo jgsogo marked this pull request as ready for review June 17, 2022 07:52
ericLemanissier
ericLemanissier approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great !

@jgsogo
Copy link
Contributor Author

jgsogo commented Jun 17, 2022

All recipes are now working (recipe linter) 🎉

image

There are some failures for the YAML linter, but they are easy to fix by contributors (and they are not related to this PR 😄 )

Please, @danimtb , @uilianries , review too. And we can consider merging this.

@jgsogo jgsogo requested review from danimtb and uilianries June 17, 2022 08:34
@conan-center-bot conan-center-bot merged commit 93190d3 into conan-io:master Jun 17, 2022
@jgsogo jgsogo deleted the linter/v2-basic branch June 17, 2022 11:15
hoxnox pushed a commit to hoxnox/conan-center-index that referenced this pull request Jun 30, 2022
@jgsogo @ericLemanissier @hoxnox
(conan-io#11162) [linter] Transform ConanFile class to match actual…
ee74a42 
… one

* [linter] Add member validation

* linter - transform ConanFile to match actual class

* install matching conan version

* add more dynamics

* no settings

* these are failing

* mock settings

* user_info_build

* add fatals to matcher (conan-io#11)

* increase threshold

* touch

Co-authored-by: ericLemanissier <ericLemanissier@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danimtb danimtb danimtb approved these changes

@ericLemanissier ericLemanissier ericLemanissier approved these changes

@uilianries uilianries Awaiting requested review from uilianries

Assignees
No one assigned
Labels
C3I config GitHub config
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jgsogo @ericLemanissier @danimtb @conan-center-bot