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

Ensure log level is set correctly (and setuptools.logging.set_threshold is called) #3042

Merged
merged 4 commits into from
Jan 24, 2022

Conversation

abravalheri
Copy link
Contributor

Motivation

For some reason distutils.log module is getting cached in distutils.dist and then loaded again when we have the opportunity to patch it.

This implies: id(distutils.log) != id(distutils.dist.log), and therefore the function setuptools.logging.set_threshold never gets called (instead the old version setuptools._distutils.log.set_threshold is used).

Summary of changes

  • Add a test case to ensure the correct verbosity level is set for logging
  • Replace the cached distutils.dist.log with the "correct" patched module object.

Closes #3038

Pull Request Checklist

For some reason `distutils.log` module is getting cached in `distutils.dist`
and then loaded again when we have the opportunity to patch it.
This implies: id(distutils.log) != id(distutils.dist.log).
We need to make sure the same module object is used everywhere.
@dalcinl
Copy link
Contributor

dalcinl commented Jan 22, 2022

@abravalheri Please see #3035. I believe the issue affects any module with a from distutils import log statement. Also consider third-party user code extending setuptools/distutils and using distutils.log. For a proper fix, we should first figure out what's wrong with the current distutils override hack and make it work for distutils submodules. Unfortunately, I'm still unable to catch the root of the problem.

@abravalheri
Copy link
Contributor Author

abravalheri commented Jan 22, 2022

Thank you very much @dalcinl for the pointer.

Indeed that might be problematic (if we find a reproduction for the other modules).

Fortunately the only thing being monkey patched in the distutils.log module is set_threshold which is only meant to be called in distutils.dist, so although not ideal, this patch might be a quick fix. The added tests are definitely something we might consider keeping.

@abravalheri abravalheri marked this pull request as ready for review January 22, 2022 14:40
@abravalheri
Copy link
Contributor Author

Regarding the root cause of the problem in #3035, the only place in the setuptools code setuptools._distutils is being imported is _distutils_hack/__init__.py via importlib.import_module.
I am not an specialist in the importlib machinery, but that might be how these modules end up is sys.modules.

@jaraco might be the best person to advise on that.

@jaraco
Copy link
Member

jaraco commented Jan 22, 2022

I'm unsure about #3035 but as that report is invalid, I'll advise to disregard its concerns for now and focus on the issue and fix at hand, which looks sound. If there are additional concerns, we can address those if/when expressed by a valid issue.

setuptools/logging.py Outdated Show resolved Hide resolved
@abravalheri
Copy link
Contributor Author

abravalheri commented Jan 22, 2022

@jaraco, I managed to write some tests to verify the duplicated import behaviour.

I will follow your suggestion and leave the problem as it is for now until a valid issue is created.

(I will just include the test cases, because it is something that can be useful in the future).

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
@abravalheri
Copy link
Contributor Author

abravalheri commented Jan 24, 2022

UPDATE: I will submit the new tests as a separated PR (#3046), so we can focus on the logging problem.

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.

[BUG] INFO log level is no longer default for some commands
3 participants