CoverageScript.command_line wipes out 1st sys.path entry. #715
Comments
|
I'd feel better about changing this if you could show me a bad behavior that the current code causes. |
|
OK. CI is here, but its a wall of text: https://travis-ci.org/pantsbuild/pants/jobs/437335398 The relevant bug presentation is configuring coverage with a plugin, that plugin being on the sys.path (in the 1st slot as it turns out) and then coverage mysteriously not being able to find the plugin:: At the handoff point from executing code, the particular sys.path is: Here the 1st entry, I went down the rabbit hole and things were ok at coverage/cmdline.py line 755: But not ok here: |
|
FWIW - I'm hacking around presently by ensuring the 1st sys.path entry (the user code), is on the sys.path twice. |
|
Also probably a wall of text, but the fix commit downstream is here: pantsbuild/pants@2d95756 |
|
The problem with making the change you suggest is that then "coverage run prog.py" creates a different sys.path than "python prog.py". Making the two the same has been my guiding star, since it is difficult to decide what the "correct" behavior should be other than that. I don't know what to do about your plugin problem.... |
|
I do have a workaround - so, bottom line, no worries. That said, commenting out the |
|
@jsirois Hmm, commenting out that line might be fine: the coverage.py tests all pass. Does it solve your problem? There is another sys.path manipulation later when we exec the file. |
|
I'm not positive, but I don't think it will affects us. Here I assume?: coveragepy/coverage/execfile.py Lines 174 to 176 in 7a099a2 We use pytest-cov to actually collect coverage data under the pytest runner and they appear to use the coverage API (ie: coverage.run(), etc) - not execfile. We only have this issue trying to produce reports after the coverage run using the coverage command reporting tools. |
|
@jsirois Could you try removing the line? |
|
Will do. If I don't get a PR out tonight I will tomorrow. Thanks Ned! |
|
A PR isn't necessary (the change is simple enough), but I'd like to know how your scenario behaves with the change. |
|
Ah, gotcha. I can guaranty it solves our problem. The digging I hinted at above with the injected print statements proved that. That said, I can back up that guaranty with a full test by tomorrow. I'll report back. |
|
Aha, yes I duped #678. I just had a chance to burn a local coverage dist with: $ git diff coverage/cmdline.py
diff --git a/coverage/cmdline.py b/coverage/cmdline.py
index edbc1d25..e6ea6e23 100644
--- a/coverage/cmdline.py
+++ b/coverage/cmdline.py
@@ -465,10 +465,6 @@ class CoverageScript(object):
if self.do_help(options, args, parser):
return OK
- # We need to be able to import from the current directory, because
- # plugins may try to, for example, to read Django settings.
- sys.path[0] = ''
-
# Listify the list options.
source = unshell_list(options.source)
omit = unshell_list(options.omit)That did in fact solve the problem pants was having and coverage now works without hacks. |
|
Fixed in e82d24d. |
|
Thanks @nedbat! |
|
Hmm, wait a minute: looks like this breaks Windows (and my Windows CI is missing some builds!?). Re-opening for the moment. |
|
It wasn't a Windows problem. Coverage.py tests fail if you don't run them with pytest-xdist, reported here: pytest-dev/pytest-xdist#376. I'm still looking for the right solution. |
|
@jsirois Looking into this more, I have a few questions. The sys.path docs (https://docs.python.org/3/library/sys.html#sys.path) say, "As initialized upon program startup, the first item of this list, path[0], is the directory containing the script that was used to invoke the Python interpreter." What is sys.path[0] for you, and why is it different than this? Also, I am just curious about your coverage plugin: what does it do? |
|
@nedbat we run pytest and coverage from within a pex - roughly a zipapp, and so the sys.path[0] entry points to our pex. In other words - in the debug output above snipped and annotated here: The 0th entry is our pex, then a bunch of system python entries, then more bits of our pex (we combine 4) in 12, 13 & 14 - then 3rdparty dependencies of the pex - including coverage down in entry 27. The critical point which applies here afaict is coverage is not guaranteed to be in the 0th slot if some other tool is running coverage. As such coverage should not blow away any sys.path entry it is not sure it owns. In this case the 0th entry is us, the bootstrap code that runs coverage and contains the coverage plugins we wish to activate. More generally, inserting or appending a sys.path entry as is appropriate to the case at hand is almost always what you want to do afaict. If there might be multiple versions of the thing and you need to have your version seen, insert 0, if you know you're unique and just need to be visible, append. The plugin just attempts to remap filenames. The scenario is as follows:
Because the coverage is gathered in step 3, it sees and reports on paths different from the original loose filesystem paths the user expects; and as such we have a coverage plugin that remaps pex pathnames back to user-space. |
|
@jsirois Thanks for the explanation. BTW, your description of your plugin sounds like what the |
|
The situation is a bit more complex. See a comment here: pantsbuild/pants#5426 (comment) and our discussion in #646. The issue boils down to this code here: coveragepy/coverage/control.py Lines 559 to 565 in e6b8839 Pants runs tests for projects that can be split over multiple source trees; ie: As such, a test against the |
|
This is re-fixed in commit b7e0eec, released as part of 5.0a4. |
This can be problematic if the coverage distribution itself is not the first item on the classpath:
coveragepy/coverage/cmdline.py
Lines 468 to 470 in 401471c
Is there any reason this couldn't be a
sys.path.insert(0, '')?I'm happy to do the work but I just wanted to run this by you first.
The text was updated successfully, but these errors were encountered: