-
-
Notifications
You must be signed in to change notification settings - Fork 594
fix: move coverage pkg to end of sys.path to avoid collisions #1045
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
Conversation
Signed-off-by: Thulio Ferraz Assis <[email protected]>
Signed-off-by: Thulio Ferraz Assis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This is installed as the script entry point. | ||
|
||
""" | ||
+ sys.path.append(sys.path.pop(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this affect the template that launches the coverage?
I am mainly thinking about this line here: https://github.com/bazelbuild/bazel/blob/0696ba32a789bbf3100f62fa2c1547fc74e36006/tools/python/python_bootstrap_template.txt#L502
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of that. @rickeylev are you familiar with that part of the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with that code in the bootstrap. Looking through blame, it was added in 2020 as part of the initial work to get coverage working. Does coverage still rely on that behavior? It sounds brittle and the sort of thing that would get fixed in the intervening years...
CI was green with this, right? Presubmits are setup to run coverage commands last I saw.
Looking at the code paths -- I'm no expert here -- I think it ends up working out, because I think what happens is:
Python starts up, puts .
at the start of path.
Coverage runs, enters main(), runs this patch, which moves .
to the end of sys.path
Shortly thereafter, coverage enters CoverageScript.command_line, which does sys.path.insert(0, '')
. It says it does this not for itself, but so "plugins can e.g. read django settings". So it doesn't sound related to coverage being able to work in general.
I didn't see the part where coverage undoes the sys.path.insert() call, but I didn't look through all its code. I'll assume its true.
The last step there makes me thing the comment in the bootstrap is outdated or misleading, as it sounds like the current directory being sys.path[0] isn't necessary for coverage itself.
ref: https://github.com/nedbat/coveragepy/blob/master/coverage/cmdline.py#L703
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most importantly: https://github.com/nedbat/coveragepy/blob/a4cd6a01f766359fbe8ff159e0ed87e03e8023ed/doc/whatsnew5x.rst#bugs-fixed.
The coverage run command has always adjusted the first entry in sys.path, to properly emulate how Python runs your program. Now this adjustment is skipped if sys.path[0] is already different than Python's default. This fixes nedbat/coveragepy#715.
When we run tests for the locked requirements files, pip-compile will import pip internally, and eventually, it tries to import
html.entries
, which is part of the standard library. Since thecoverage
package provides anhtml.py
file, it ends up being picked up by pip if thecoverage
package is early on thesys.path
.This patch presents a generic fix for the problem with coverage by moving its own directory to the end of
sys.path
.