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

Add logging level for conan-center hook #280

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Mar 11, 2021

This PR is an extension of CONAN_LOGGING_LEVEL, but for Conan Center hook.

For local development we may not want to see all info, only warning and error are enough.

It's related to #279. Unfortunately we don't have a hook post-command yet, but we can silence minor messages at least.

/cc @ericriff

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Do we want to name the variable CONAN_HOOK... when it is something that affects only to the hook named conan-center?

@uilianries
Copy link
Member Author

uilianries commented Mar 12, 2021

Well, we already have CONAN_HOOK_ERROR_LEVEL. But we can re-use them for other hooks too, it's just because we don't have a separated feature for it on Conan. I just want to keep it separated from CONAN_LOGGING_LEVEL. Because if I work developing recipes for my company and cci at same machine, I'll want to mute conan-center hook messages, but keeping regular Conan client messages working.

jgsogo
jgsogo previously approved these changes Mar 12, 2021
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Yeah, the same pattern as CONAN_HOOK_ERROR_LEVEL

hooks/conan-center.py Outdated Show resolved Hide resolved
@jgsogo
Copy link
Contributor

jgsogo commented Apr 29, 2021

In C3i we should probably use WARNING level, right? That way when we tell a user that the recipe is broken because of the hook, they will see only the warnings/errors, but not all the hooks that have passed. Am I right?

@uilianries
Copy link
Member Author

@jgsogo Yes, in CCI it would be nice using WARNING as default level. I don't know if we should enable it by default or CCI should add CONAN_HOOK_LOGGING_LEVEL

hooks/conan-center.py Outdated Show resolved Hide resolved
hooks/conan-center.py Outdated Show resolved Hide resolved
hooks/conan-center.py Outdated Show resolved Hide resolved
hooks/conan-center.py Outdated Show resolved Hide resolved
hooks/conan-center.py Outdated Show resolved Hide resolved
hooks/conan-center.py Show resolved Hide resolved
@Croydon
Copy link
Contributor

Croydon commented Sep 12, 2021

I would like WARNING level per default locally too. I think that at this point all the OKs are kinda spam in the logs 🙈

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
uilianries and others added 4 commits September 14, 2021 12:01
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
hooks/conan-center.py Outdated Show resolved Hide resolved
Comment on lines 1098 to 1100
self.assertNotIn("[FPIC MANAGEMENT (KB-H007)] OK", output)

def test_os_rename_warning(self):
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
self.assertNotIn("[FPIC MANAGEMENT (KB-H007)] OK", output)
def test_os_rename_warning(self):
self.assertNotIn("[FPIC MANAGEMENT (KB-H007)] OK", output)
with tools.environment_append({"CONAN_HOOK_LOGGING_LEVEL": "9001"}): # Over 9000
output = self.conan(['create', '.', 'name/version@user/test'])
self.assertNotIn("ERROR: [PACKAGE LICENSE (KB-H012)]", output)
self.assertNotIn("WARN: [HEADER_ONLY, NO COPY SOURCE (KB-H005)]", output)
self.assertNotIn("[FPIC MANAGEMENT (KB-H007)] OK", output)
def test_os_rename_warning(self):

Copy link
Member Author

Choose a reason for hiding this comment

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

Over_9000!

uilianries and others added 5 commits September 15, 2021 12:03
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

I needed to force Pylint 2.10, because 2.11 changed something which broke hooks. I did a quick read on Pylint repo and there are more issues related to 2.11. My suggestion is using 2.10 for now and wait for a solution (maybe 2.12).

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

About logging levels, I think we want 2 things.

  1. less verbose logs when building locally, which can be accomplished by this pr
  2. increase the severity of an error (aka introduce -Werror):
    I think it would be useful for c3i to mark warnings as errors.
    Right now, we have some checks marked as warnings because marking them as error would cause too many builds to fail.
    To that, I propose to only increase the severity if the name of the recipe is the same as the recipe is being built (and not a dependency).

@uilianries
Copy link
Member Author

I think it would be useful for c3i to mark warnings as errors.

It will break many recipes in Conan Center. You are a Conan expert, know about the errors and warnings, but for new contributors, it can be a block when submitting their first PR.

@madebr
Copy link
Contributor

madebr commented Sep 21, 2021

It's a double edged sword.
I think we should think about ways to give attention to the warnings then.
Right now, c3i hides them.
It would be useful to have them available somewhere such that people/reviewers can propose fixes for them.

@Croydon
Copy link
Contributor

Croydon commented Nov 9, 2021

I would like to see that the CCI bot is outputting all hook warnings as a comment in the PR, but not blocking the PR because of it

@SSE4
Copy link
Contributor

SSE4 commented Nov 9, 2021

I would like to see that the CCI bot is outputting all hook warnings as a comment in the PR, but not blocking the PR because of it

yes, it's possible, we were thinking how to implement it in CI.
but as we run initial export + parallel builds, it has to be gathered in multiple places and then somehow represented in the readable way, which is not too annoying.

@Croydon
Copy link
Contributor

Croydon commented Apr 14, 2022

What is the current status of this?

@uilianries
Copy link
Member Author

Need to re-visit

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

Successfully merging this pull request may close these issues.

None yet

5 participants