hooks icon indicating copy to clipboard operation
hooks copied to clipboard

Add logging level for conan-center hook

Open uilianries opened this issue 4 years ago • 12 comments

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

uilianries avatar Mar 11 '21 14:03 uilianries

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.

uilianries avatar Mar 12 '21 16:03 uilianries

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?

jgsogo avatar Apr 29 '21 10:04 jgsogo

@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

uilianries avatar Apr 29 '21 14:04 uilianries

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

Croydon avatar Sep 12 '21 19:09 Croydon

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).

uilianries avatar Sep 20 '21 20:09 uilianries

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).

madebr avatar Sep 21 '21 22:09 madebr

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.

uilianries avatar Sep 21 '21 23:09 uilianries

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.

madebr avatar Sep 21 '21 23:09 madebr

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

Croydon avatar Nov 09 '21 09:11 Croydon

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.

SSE4 avatar Nov 09 '21 09:11 SSE4

What is the current status of this?

Croydon avatar Apr 14 '22 09:04 Croydon

Need to re-visit

uilianries avatar Apr 18 '22 07:04 uilianries